Fix incorrect range routing for right-side sharding columns

[Gimini-3]

No associated issue.

Changes proposed in this pull request:

• Reverse ordered comparison operators when a sharding column is on the right side of a binary predicate.
• Add parameterized unit coverage for <, <=, >, and >=.

Requested labels: type: bug, in: Kernel.

Verification:

• ./mvnw -pl features/sharding/core -am -DskipITs -Dspotless.skip=true -Dtest=org.apache.shardingsphere.sharding.route.engine.condition.generator.impl.ConditionValueCompareOperatorGeneratorTest -Dsurefire.failIfNoSpecifiedTests=false test: 23 tests passed.
• ./mvnw spotless:apply -Pcheck -T1C: passed with no file changes.
• ./mvnw checkstyle:check -Pcheck -T1C: passed with 0 violations.

AI assistance disclosure:

• Tool: OpenAI Codex.
• Scope: ConditionValueCompareOperatorGenerator.java, ConditionValueCompareOperatorGeneratorTest.java, and WhereClauseShardingConditionEngineTest.java.
• The contributor reviewed and verified the complete change.

---

Before committing this PR, I'm sure that I have checked the following options:

• My code follows the code of conduct of this project.
• I have self-reviewed the commit code.
• I have requested corresponding labels for the pull request.
• I have passed the full Maven check locally: ./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.
• Documentation changes are not required because this does not change public APIs or configuration.
• I have added corresponding unit tests for my changes.
• I have updated the Release Notes of the current development version. For more details, see Update Release Note
• I have disclosed the tool and affected scope for material AI assistance as required by the AI-assisted contribution policy.

[terrymanu]

Result

Review Result: Not Mergeable

Feedback Mode: Change Request

Blocking Issues: 1

The direct-column fix is correct, but a reachable right-side wrapped-column path still produces unsafe shard pruning.

Blocking Issues

1. [P1] Preserve broadcast routing for right-side BINARY columns

   • Evidence: In ConditionValueCompareOperatorGenerator.java:76, 100 < BINARY id selects 100 as the value but does not reverse the operator because the right operand is a UnaryOperationExpression, not a direct ColumnSegment. A head reproduction through the MySQL parser confirmed that column extraction finds id, yet the generator returns Range.lessThan(100). This contradicts the established BINARY-range broadcast contract documented at lines 70–71 and tested in WhereClauseShardingConditionEngineTest.java:120.
   • Impact: The resulting range is passed to the range sharding algorithm by StandardShardingStrategy.java:58, potentially excluding shards containing valid rows.
   • Required Change: Make a right-side BINARY-wrapped sharding column produce no range condition for ordered comparisons, preserving broadcast routing. Add regression coverage for a parsed or condition-engine path such as 100 < BINARY sharding_col.

Coverage

• Reviewed head: 8dae6aed8dd1bdd809d741f0c79d22fbb3ad2609; final refresh confirmed it remained unchanged.
• Accounted for all three authoritative files: generator behavior, parameterized tests, and release note.
• Completed root-cause, blast-radius/contracts, tests/runtime/operations, and convergence passes; no unresolved evidence gaps remain.
• Focused and adjacent Maven verification: 36 tests, 0 failures/errors, 1 skipped; build succeeded.
• No linked issue or public review discussion existed.
• Code Correctness Review only; CI was not reviewed.

[Gimini-3]

Addressed in f9bef17. Right-side BINARY-wrapped sharding columns now preserve broadcast routing, with regression coverage added in WhereClauseShardingConditionEngineTest.

[terrymanu]

Result

Review Result: Not Mergeable

Feedback Mode: Change Request

Blocking Issues: 1

The correctness fix is complete, but a reproducible performance regression affects the equality-condition hot path. The PR should not be merged in its current form.

Blocking Issues

1. [P2] Avoid adding range-specific CPU overhead to every equality condition

   • Evidence: ConditionValueCompareOperatorGenerator.java:72-80 invokes isRangeOperator for equality predicates and then repeats the check that determines whether the left operand is a column. HotSpot C2 compilation confirmed that isRangeOperator is inlined and that the = path executes all four String.equals comparisons.
   • Evidence: With JDK 21.0.12, C2, and Epsilon GC fixed across base and head, the id = ? benchmark produced median results of 25.032 and 27.509 ns/op across six independent JVMs, making the head 9.9% slower. Three independent same-JVM, dual-classloader forks produced median head/base ratios of 1.114, 1.207, and 1.151. Allocation remained identical at 120 B/op.
   • Impact: ConditionValueGeneratorFactory.java:57-61 sends every binary sharding condition through this method. Although the absolute increase is only a few nanoseconds per invocation, it is an avoidable CPU regression in a core hot path and is unrelated to the intended fix.
   • Required Change: Keep the range-specific BINARY checks off the equality path and reuse a single determination of which operand is the column. Repeat the same baseline benchmark and confirm that the median regression does not exceed 5% and that allocation does not increase.

Coverage

• Reviewed head: f9bef17d058cb0501622f1e93cd2911ebe8dceb3; base: 65e298aaeea19adb13a0d7ef8b0a56afd38771e9. A final GitHub API refresh confirmed that the head remained unchanged.
• Accounted for all four authoritative files covering operator reversal, BINARY broadcast fallback, generator and condition-engine tests, and the release notes.
• The previous incorrect-pruning issue for 100 < BINARY id is fixed at the current head, and its regression test is effective.
• Focused verification: 27 tests passed. JaCoCo confirmed execution of the new BINARY guard and all four range-operator reversal mappings.
• Full features/sharding/core verification: 867 tests, 0 failures, 0 errors, and 1 skipped.
• Completed root-cause, boundary and contract, runtime and test, performance, and final convergence reviews. Apart from the performance issue above, no functional, concurrency, memory, API/SPI, dependency, compatibility, or allocation regressions were found.
• This was a code-correctness and targeted-performance review; CI status was not reviewed. Absolute zero risk cannot be guaranteed, but no unresolved evidence gap remains that could change the current blocker set.

[Gimini-3]

Addressed in 640df15. Equality predicates now skip range-specific BINARY checks, and the column side is determined once and reused. With JDK 21.0.12.1, C2, Epsilon GC, and six JMH forks, the baseline and candidate medians were 23.585 and 24.033 ns/op (+1.90%), with 120.000 B/op for both. The 27 focused tests and all 867 sharding-core tests passed.

[terrymanu]

Correction

• Previous Finding: The equality-path microbenchmark regression was merge-blocking.
• Current Status: Withdrawn
• Reason: Exact-source JDK 21 measurements reproduced only single-digit-nanosecond method overhead with unchanged allocation. No profile, SLA, repository budget, or end-to-end routing benchmark proves material impact, so the Finding Proof Gate’s necessity condition is not met.

Result

Review Result: Not Mergeable

Feedback Mode: Change Request

Blocking Issues: 1

Direct right-column comparisons and top-level BINARY operands are fixed, but nested right-side sharding expressions can still produce an unsafe range.

Blocking Issues

1. [P1] Do not treat nested right expressions as direct sharding columns

   • Evidence: MySQL permits comparisons against arithmetic expressions such as 100 < id + 1 (MySQL expression grammar). The visitor preserves the arithmetic operand as a nested BinaryOperationExpression (MySQLStatementVisitor.java:547, MySQLStatementVisitor.java:652). ColumnExtractor recursively finds id, and the condition engine passes the original comparison to the generator (ColumnExtractor.java:214, WhereClauseShardingConditionEngine.java:110). Head reverses only when the right operand itself is a ColumnSegment, so it selects literal 100 and emits Range.lessThan(100) (ConditionValueCompareOperatorGenerator.java:72). Exact-source reproduction confirmed one extracted column, the incorrect range, and broadcast behavior for the equivalent left-side expression.
   • Impact: id = 101 satisfies 100 < id + 1, but the generated range excludes it. StandardShardingStrategy forwards that range to the configured algorithm, which may prune the shard containing the matching row (StandardShardingStrategy.java:81).
   • Required Change: Generate a range only when the sharding column is a direct operand after explicitly supported normalization. Return Optional.empty() for unsupported nested or derived operands, and add a condition-engine or parsed-SQL regression covering 100 < sharding_col + 1.

Coverage

• Reviewed head f9bef17d058cb0501622f1e93cd2911ebe8dceb3; merge base 65e298aaeea19adb13a0d7ef8b0a56afd38771e9. Final GitHub refresh confirmed the head and four-file scope were unchanged.
• Accounted for all four authoritative files: operator reversal, BINARY broadcast fallback, both test classes, and the release note.
• Verified that all four direct operator mappings and the prior right-side BINARY finding are fixed. The confirmed nested-expression issue was present earlier and missed by previous reviews, but remains inside this PR’s declared right-side comparison fix boundary.
• Completed root-cause, blast-radius/contracts, tests/runtime/operations, performance, SQL-dialect, and zero-new-finding convergence passes. No unresolved evidence gaps remain.
• Code Correctness Review only; CI was not reviewed.

[Gimini-3]

Addressed in c8d06ae. The generator now requires a direct column operand after supported normalization and returns no range condition for nested or derived operands such as 100 < sharding_col + 1, preserving broadcast routing. Added condition-engine regression coverage. The focused suite passed 28 tests; sharding-core completed 868 tests with 0 failures/errors and 1 skipped. Spotless and Checkstyle also passed.

[terrymanu]

Please fix the GitHub action first

[Gimini-3]

I don’t have permission to rerun the failed jobs. The E2E failure reports MySQL “Too many connections,” and the CI job includes Maven artifact-lock timeouts. Could you please rerun the failed jobs?