Skip sharding conditions irrelevant to the routed table#39112
Skip sharding conditions irrelevant to the routed table#39112Develop-KIM wants to merge 3 commits intoapache:masterapache/shardingsphere:masterfrom Develop-KIM:fix-irrelevant-sharding-condition-route-explosionDevelop-KIM/shardingsphere:fix-irrelevant-sharding-condition-route-explosionCopy head branch name to clipboard
Conversation
A subquery over two non-binding sharding tables makes ShardingConditionEngine emit one ShardingCondition per table instead of merging them into a single condition, as it does for the equivalent JOIN. While routing one of those tables, the condition belonging to the other table yields no sharding values, so route0 treats it as "no condition given" and broadcasts to every data node. The accumulated result is a full route: the reporter saw 512 actual SQLs (4 datasources x 128 shards) for a query that should reach a single shard. Skip conditions that carry no value for the routed table or its binding table group, while still recording a placeholder in originalDataNodes so that the positional mapping RouteSQLRewriteEngine#buildRouteParameters relies on stays aligned with the condition list. Conditions without any sharding value keep their previous full-route behaviour, since an empty condition means "no sharding information", not "belongs to another table". The earlier attempt in apache#38527 dropped irrelevant conditions with `continue` and fell back to a bare route0 when all of them were dropped, which left originalDataNodes empty and made RouteSQLRewriteEngine#getParameters take the short path instead of buildRouteParameters. That regression is what got it reverted in apache#38639; assertRouteByShardingConditionWithoutShardingValues now guards it. Fixes apache#38456 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
terrymanu
left a comment
There was a problem hiding this comment.
Summary
Review Result: Not Mergeable
Feedback Mode: Change Request
Reason: The irrelevant-condition filtering direction is sound, but the new fallback conflates “no relevant condition” with “a relevant condition routed to no table,” introducing a full-route regression.
Issues
P1: Preserve legitimate zero-target routing results
Problem: routeByShardingConditionsWithCondition now performs full routing whenever the accumulated result is empty. However, a relevant condition can legitimately produce no table targets: routeTables permits an empty table-sharding result, and IntervalShardingAlgorithm returns no target for a precise value outside its configured interval. StandardShardingStrategy preserves that as an empty result. Before this PR, the route engine returned the empty result; this PR reruns route0 without sharding values and selects every configured table.
Impact: A query known to match no configured shard can be broadcast to every shard, changing both result semantics and routing cost.
Required Change: Track whether at least one condition was relevant independently from whether routing produced nodes. Use full-route fallback only when every condition was irrelevant; preserve an empty result from relevant conditions. Please add a focused regression test where a relevant table condition—such as an out-of-range INTERVAL value—produces no target and assert that no route units are created.
Review Details
- Review Focus: Code Correctness Review; CI not reviewed by request.
- Reviewed Scope: All three changed files at head
451277b53152338bd492d9c9aae34b1ea6f0880a:RELEASE-NOTES.md,ShardingStandardRouteEngine.java, andShardingStandardRouteEngineTest.java. Supporting route, strategy, interval-algorithm, Cartesian-route, and rewrite paths were traced. Local merge-base:7b8178371a5be5cebb11bdbf5b96fbb437953f48. The local triple-dot file list matched GitHub/pulls/39112/files. - Not Reviewed Scope: GitHub Actions/check runs and runtime E2E execution. No changed file was omitted.
- Verification: Public PR metadata, changed files, linked issue #38456, prior fixes/revert #38527/#38639, comments, and reviews were accessible. The bounded review inventory and triple-dot scope checks exited successfully. Maven was not run because the shared checkout is not at the PR head; non-head test results were not used as evidence.
- Release Note / User Docs: The release-note entry is present. Additional user documentation is not required because this is an internal routing correction without configuration or migration changes.
Full-route fallback now fires only when every condition was irrelevant to the routed table. A relevant condition that legitimately routes to no target (e.g. an out-of-range interval value) keeps its empty result instead of being broadcast to every shard. Adds a regression test covering that case.
|
Thanks for the careful review — you're right, the empty-result fallback conflated the two cases. Fixed it: I now track whether any condition was relevant to the routed table ( Added the regression test you asked for: |
Fixes #38456.
Changes proposed in this pull request:
originalDataNodesfor the skipped conditions, keeping it positionally aligned with the condition list.Why the route explodes
For a subquery over two non-binding sharding tables,
ShardingConditionEngineemits oneShardingConditionper table instead of merging them into a single condition the way it does for the equivalent JOIN. While routing one of the tables, the condition that belongs to the other table yields no sharding values, soroute0reads it as "no condition given" and broadcasts. The reporter saw 512 actual SQLs (4 datasources x 128 shards) for a query that should reach one shard.Why this does not repeat #38527
#38527 filtered the same conditions but dropped them with
continue, and fell back to a bareroute0when every condition was dropped. Two things went wrong, and #38639 reverted it:RouteSQLRewriteEngine#buildRouteParameterswalksoriginalDataNodesby index and feeds that index straight toparamBuilder.getParameters(count). Dropping an entry shifts every later index, so parameters can land on the wrong row. This PR appends an empty collection instead of skipping, which leaves the indices intact and reads as "matches every route unit" inisInSameDataNode, exactly as before.originalDataNodes.RouteSQLRewriteEngine#getParametersthen took the short path instead ofbuildRouteParameters. Here such a condition is treated as relevant, so its full-route behaviour is unchanged.assertRouteByShardingConditionWithoutShardingValuescovers the second point: dropping the empty-values guard turnsoriginalDataNodesfrom[4 nodes]into[]and the test fails, so the regression cannot come back unnoticed.Scope
routeByMixedConditionsWithConditionis left alone on purpose, even though the issue mentions the same shape there. That path mixes hint strategies, where a condition that looks irrelevant can still contribute a valid hint value, so the same filter is not obviously safe. Happy to extend it in this PR or a follow-up if you would prefer.Verification
assertRouteByShardingConditionsWithIrrelevantTableConditionfails on master (expected: <1> but: was <4>) and passes with the fix../mvnw -pl features/sharding/core -Drat.skip=true test— 853 tests, checkstyle included../mvnw -pl infra/rewrite/core -Drat.skip=true test— 87 tests.This was written with AI assistance (disclosed in the commit trailer); I have reviewed the reasoning and the diff and can speak to every line.
Before committing this PR, I'm sure that I have checked the following options:
./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.