lanky228 opened a new pull request, #5141:
URL: https://github.com/apache/calcite/pull/5141
## What
`NlsString.compareTo()` only compared the decoded string value, ignoring
`charsetName`, `collation`, and `bytesValue`. This made `compareTo`
inconsistent with `equals()`/`hashCode()`, which consider all metadata fields.
## Why It Matters
This violates the Java contract: when `equals() == true`, `compareTo() == 0`
must also hold. The bug caused `TreeSet` and `TreeMap` to silently collapse
distinct `NlsString` values that shared the same decoded string but differed in
charset or collation.
Example: `("hello", "LATIN1", null)` and `("hello", "UTF-8", null)` are not
equal per `equals()`, but the old `compareTo()` returned 0 — causing a
`TreeSet` to keep only one of them.
## Changes
**`NlsString.java`** (+32 -2):
- After comparing decoded string values (preserving the original collator
branch), break ties by comparing:
1. `charsetName` — `Comparator.nullsFirst(String::compareTo)` for null
safety
2. `collation` — `SqlCollation` does not implement `Comparable`, so
`toString()` is used as proxy key
3. `bytesValue` — `ByteString` implements `Comparable<ByteString>`, using
`Comparator.naturalOrder()`
- All comparisons use `Comparator.nullsFirst()` for null-safe handling
(fields are `@Nullable`)
- No new helper methods — standard library `Comparator` utilities only
**`UtilTest.java`** (+53):
- `testNlsStringCompareToConsistency()`: 5 cases verifying
`compareTo`/`equals` consistency:
- Same text, different charset → not equal, `compareTo != 0`
- Same text, different collation → not equal, `compareTo != 0`
- Identical values → equal, `compareTo == 0`
- Both null charset → equal, `compareTo == 0`
- One null, one non-null charset → not equal, `compareTo != 0`
- `testNlsStringTreeSetRetainsDistinctValues()`: 4 values with same string
"hello" but different charset — TreeSet must retain all 4 (before fix:
collapsed to 1)
## Verification
- `./gradlew :core:compileJava` — BUILD SUCCESSFUL
- `./gradlew :core:checkstyleMain` — passed
- `./gradlew :core:test --tests "org.apache.calcite.util.UtilTest"` — 7
tests passed
## Note
This is a behavior change: `TreeSet<NlsString>` ordering results may differ
from before. This is the intended fix — the old behavior was a bug.
## Jira
[CALCITE-7554](https://issues.apache.org/jira/browse/CALCITE-7554)
--
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]
