FlyingZC commented on PR #38772: URL: https://github.com/apache/shardingsphere/pull/38772#issuecomment-4599914512
Decision Merge Verdict: Mergeable Reviewed Scope: PR latest head 56919fca8bee86fa3e07377b8c90630e8e6e627e; base ref master; local merge-base 8b2690b07e2ee434f0cd69f2907b528f2d3d2c8a. GitHub /pulls/16213/ files and local triple-dot diff both contain exactly one file: features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/route/engine/type/standard/ ShardingStandardRouteEngine.java. Not Reviewed Scope: GitHub Actions / CI status, full repository test suite, end-to-end JDBC/Proxy tests, and performance benchmark / JFR rerun. Need Expert Review: No dedicated security, protocol, or concurrency review is required. A performance benchmark rerun is optional if maintainers want quantified allocation delta. Basis The root cause is directly addressed. The previous route-condition filtering created a new CaseInsensitiveSet for every ShardingConditionValue; the latest code now reuses shardingColumns.contains(each.getColumnName()) in features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/route/engine/type/standard/ ShardingStandardRouteEngine.java:232. The case-insensitive behavior is preserved. The changed method receives columns from databaseShardingStrategy.getShardingColumns() / tableShardingStrategy.getShardingColumns() on the standard route path at features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/route/engine/type/standard/ShardingStandardRouteEngine.java:136, :174, and :180; StandardShardingStrategy stores the configured sharding column in a CaseInsensitiveSet and exposes it through an unmodifiable wrapper at features/sharding/core/src/main/java/org/ apache/shardingsphere/sharding/route/strategy/type/standard/StandardShardingStrategy.java:51. Adjacent strategy paths remain compatible. NoneShardingStrategy still exposes an empty column collection at features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/ route/strategy/type/none/NoneShardingStrategy.java:36, and HintShardingStrategy still uses a CaseInsensitiveSet while the mixed hint path bypasses condition-column extraction when hint values are used. The blast radius is acceptable. This is a shared JDBC/Proxy sharding route hot path across database dialects, but the patch does not change parser behavior, identifier interpretation, routing precedence, binding-table lookup, config, API, or SPI contracts. It removes one allocation site without adding loops, cache state, blocking I/O, or ConcurrentHashMap#computeIfAbsent. Verification Boundary check: local git diff --name-status <merge-base>..pr/16213/head matched GitHub /pulls/16213/files. Reviewer-run test command passed with exit code 0: ./mvnw -Dmaven.repo.local=<workspace-local-maven-repo> -pl features/sharding/core -am -DskipITs -Dspotless.skip=true -Dcheckstyle.skip=true -Dtest=ShardingStandardRouteEngineTest,StandardShardingStrategyTest -Dsurefire.failIfNoSpecifiedTests=false test Result: Tests run: 12, Failures: 0, Errors: 0, Skipped: 1. Reviewer-run style command passed with exit code 0: ./mvnw -Dmaven.repo.local=<workspace-local-maven-repo> -pl features/sharding/core -am -DskipTests -DskipITs spotless:check checkstyle:check -Pcheck -T1C Result: BUILD SUCCESS, with 0 Checkstyle violations in the scoped reactor. Adversarial pass checked the risky counterexamples: uppercase / case-preserved column matching, None/Hint adjacent strategy paths, shared JDBC/Proxy route execution, and original allocation symptom. No unresolved merge blocker was found. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
