rishi-rana commented on PR #39855:
URL: https://github.com/apache/shardingsphere/pull/39855#issuecomment-5737604175
### Result
**Review Result: Mergeable**
Feedback Mode: Approve
The fix is correct, minimal, and consistent with the established handling of
negated predicates in this package. No blocking issues found.
**Blocking Issues: 0**
### Evidence
- Applied the two changed Java files onto a local tree whose
`features/sharding` contents are identical to this PR's base commit
`101c3d56ccd7ee3c4a390a8f37550bed58a4751c`.
- **The defect is real.** At the base commit,
`ConditionValueBetweenOperatorGenerator.generate()` never reads
`predicate.isNot()`; it builds `Range.closed(betweenValue, andValue)` whenever
both endpoints resolve, so `NOT BETWEEN a AND b` yields the exact range the
predicate excludes.
- **The fix matches in-repo precedent.** `ConditionValueInOperatorGenerator`
already opens with the same guard, returning `Optional.empty()` for a negated
predicate. The change applies that identical shape to `BetweenExpression`, so
the two generators now agree on negation handling.
- **No sibling case is left unfixed.** The third generator in this package,
`ConditionValueCompareOperatorGenerator`, admits operators through an allowlist
of `=`, `>`, `<`, `>=`, `<=`, `IS`, so `!=` and `<>` already fall through to
`Optional.empty()`. After this change all three generators exclude negated
predicates from sharding conditions.
- **The flag is genuinely populated**, so the guard is not dead code: the
dialect visitors construct `BetweenExpression` with the parsed `NOT` token, for
example `MySQLStatementVisitor` line 648, `PostgreSQLStatementVisitor` line
639, and `ClickHouseStatementVisitor` line 315 (`null != ctx.NOT()`).
- **The new test is a real regression guard.** I removed the three added
production lines and re-ran the class:
`assertGenerateConditionValueWithNotBetween` failed with `AssertionFailedError:
expected: <false> but was: <true>`, while the other 7 tests still passed.
Restoring the guard returned the class to 8/8.
- **Falling back to `Optional.empty()` is the safe direction.** Dropping the
condition widens the route rather than narrowing it, so a negated BETWEEN can
over-route but cannot skip a shard holding matching rows, which is the opposite
of the data-loss path described in #39852.
- Tests run with the patch applied:
- `./mvnw -pl features/sharding/core -DskipITs
-Dtest=ConditionValueBetweenOperatorGeneratorTest test` - 8 tests, 0 failures.
- `./mvnw -pl features/sharding/core -DskipITs test` - 878 tests, 0
failures, 1 skipped, so widening the route for negated BETWEEN regresses no
existing routing expectation.
- `./mvnw -pl features/sharding/core spotless:check checkstyle:check
-Pcheck` - clean.
### Coverage
- All 3 changed files accounted for: 1 production, 1 test, 1 release note.
The release note entry is present and points at this PR.
- Behaviour clusters covered: sharding condition generation for negated
predicates, and consistency across the three `ConditionValueGenerator`
implementations.
- Reviewed as Code Correctness. CI checks were not reviewed.
- Not verified: I did not exercise the end-to-end route for `account_id = 11
AND account_id NOT BETWEEN 1 AND 5` from the linked issue against a running
proxy; the evidence above covers the generator contract only.
- No changes were made to the repository; the local working tree was
restored.
--
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]
