anxkhn opened a new pull request, #5076: URL: https://github.com/apache/calcite/pull/5076
## Jira Link [CALCITE-7555](https://issues.apache.org/jira/browse/CALCITE-7555) ## Changes Proposed `Sarg` implements `Comparable<Sarg>`, and its `equals`/`hashCode` both fold in `nullAs` (the null-matching semantics) alongside the range set. `compareTo`, however, ordered only by the range set: ```java @Override public int compareTo(Sarg<C> o) { return RangeSets.compare(rangeSet, o.rangeSet); } ``` So two Sargs over identical ranges but with different null semantics, for example `Sarg.of(RexUnknownAs.UNKNOWN, s)` versus `Sarg.of(RexUnknownAs.FALSE, s)`, are unequal as objects yet `compareTo` returns `0`. That breaks the `Comparable`-consistent-with-`equals` contract that `TreeSet`/`TreeMap` rely on, so a sorted collection keyed on `Sarg` silently drops one of two semantically distinct search arguments. This PR tie-breaks on `nullAs` after the range-set comparison, so `compareTo` agrees with `equals`: ```java @Override public int compareTo(Sarg<C> o) { int c = RangeSets.compare(rangeSet, o.rangeSet); if (c != 0) { return c; } // Tie-break on nullAs so that compareTo is consistent with equals and // hashCode, which both account for nullAs. ... return Integer.compare(nullAs.ordinal(), o.nullAs.ordinal()); } ``` Reproduction (added as a unit test in `RexProgramTest`, `testSargCompareToIsConsistentWithEquals`): ```java final ImmutableRangeSet<Integer> singleton = ImmutableRangeSet.of(Range.singleton(1)); final Sarg<Integer> unknown = Sarg.of(RexUnknownAs.UNKNOWN, singleton); final Sarg<Integer> falseSarg = Sarg.of(RexUnknownAs.FALSE, singleton); assertFalse(unknown.equals(falseSarg)); assertThat(unknown.compareTo(falseSarg) == 0, is(false)); // was true before the fix final TreeSet<Sarg<Integer>> values = new TreeSet<>(); values.add(unknown); values.add(falseSarg); assertThat(values, hasSize(2)); // was 1 before the fix ``` The tie-break runs only when `RangeSets.compare` returns `0`, which is exactly when the two range sets are structurally equal, so after the change `compareTo == 0` iff the range sets are equal and `nullAs` is equal, i.e. iff `equals` is `true`. The 6 `SpecialSarg` singletons keep identity `equals` and inherit the new `compareTo`; their `(rangeSet, nullAs)` pairs are all distinct, so the tie-break only makes their ordering finer and never less consistent. No behavior changes where the range sets already differ. Testing: `./gradlew :core:autostyleCheck :core:checkstyleMain :core:checkstyleTest` pass, and `./gradlew :core:test --tests "org.apache.calcite.rex.RexProgramTest"` passes (179 tests, 1 pre-existing skip). The new test fails on `main` without the one-line fix and passes with it. --- ## Files changed - `core/src/main/java/org/apache/calcite/util/Sarg.java` `compareTo` returns the range-set comparison when non-zero, otherwise tie-breaks on `Integer.compare(nullAs.ordinal(), o.nullAs.ordinal())` (with an explanatory comment). - `core/src/test/java/org/apache/calcite/rex/RexProgramTest.java` Adds `testSargCompareToIsConsistentWithEquals` and the `java.util.TreeSet` import; the test is colocated next to the sibling CALCITE-5722 `testSargAntiPoint`. --- -- 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]
