fix: close P0 sharding holes from the #56 review - #65
Open
renecannao wants to merge 4 commits into
Open
Conversation
- Detect qualified SET t.id as a shard-key assignment and move the row. - Do not fall back to in-place UPDATE when a move SELECT finds no rows. - Empty IN (subquery) rewrites to FALSE instead of per-shard subquery. - Copy subquery string values into the arena (no ResultSet UAF). - Do not mutate the cached logical AGGREGATE when redistributing. - HASH routes integer-looking strings like ints. - Semi-join prune is INNER only; colocated joins emit LEFT/RIGHT/FULL. - Planner-time SELECTs go through TxnRoutingExecutor.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Add cases for qualified non-key UPDATE, same-value and multi-row moves, string/int HASH including negatives and leading zeros, empty IN-subquery DML, string IN-list delete, INSERT...SELECT empty source, unknown-table SELECT/UPDATE/DELETE, plan-cache COUNT/SUM twice, composite qualified move, RIGHT JOIN emit, LIST OR prune, and partial vs full composite HASH prune.
- ResultSet carries ok/error_message so INSERT...SELECT and move SELECTs
no longer treat a failed remote as zero rows.
- Unknown-table SELECT returns ResultSet::fail instead of {}.
- Shard-key move SELECT uses FOR UPDATE; empty/error does not in-place
update. Session refuses a DELETE+INSERT move without a distributed txn.
- shard_index_for_* return SIZE_MAX when unroutable; engine_stress uses try_*.
New tests/test_shard_battery.cpp covers HASH/RANGE/LIST/composite routing, planner prune matrix, INSERT-then-SELECT for ids 0–49, moves, LIST miss, RANGE placement, and Session COUNT cache hits.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the silent-wrong bugs called out on #56:
SET t.id = …is treated as a shard-key assignment and moves the row.WHERE 1=0), not an in-place key UPDATE on shard 0.IN (subquery)becomes0=1instead of shipping the subquery to every shard.'3'and3land together).LEFT/RIGHT/FULL.TxnRoutingExecutor.Stacked on #56 (
fix/shard-dml-routing).Test plan
./run_tests --gtest_brief=1— 1350 passed, 40 skipped