You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "lanky228 (via GitHub)" <gi...@apache.org> on 2026/08/02 17:48:31 UTC
[PR] [CALCITE-7554] NlsString.compareTo inconsistent with equals/hashCode [calcite]
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7554] NlsString.compareTo inconsistent with equals/hashCode [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #5141:
URL: https://github.com/apache/calcite/pull/5141#issuecomment-5169185314
In general please make changes as new commits, so the reviewers can see what's new.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7554] NlsString.compareTo inconsistent with equals/hashCode [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5141:
URL: https://github.com/apache/calcite/pull/5141#issuecomment-5160141552
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5141) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5141&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5141&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5141&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5141&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5141&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5141)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7554] NlsString.compareTo inconsistent with equals/hashCode [calcite]
Posted by "lanky228 (via GitHub)" <gi...@apache.org>.
lanky228 commented on PR #5141:
URL: https://github.com/apache/calcite/pull/5141#issuecomment-5161270096
Thanks, removed the historical comments. Kept the last line because the byte comparison step is non-obvious and needs explanation.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7554] NlsString.compareTo inconsistent with equals/hashCode [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #5141:
URL: https://github.com/apache/calcite/pull/5141#discussion_r3700090321
##########
core/src/main/java/org/apache/calcite/util/NlsString.java:
##########
@@ -185,10 +186,42 @@ private NlsString(@Nullable String stringValue, @Nullable ByteString bytesValue,
}
@Override public int compareTo(NlsString other) {
+ // String content: ("abc",X) vs ("xyz",Y) -> not equal, return
+ int cmp;
if (collation != null && collation.getCollator() != null) {
- return collation.getCollator().compare(getValue(), other.getValue());
+ cmp = collation.getCollator().compare(getValue(), other.getValue());
+ } else {
+ cmp = getValue().compareTo(other.getValue());
+ }
+ if (cmp != 0) {
+ return cmp;
+ }
+
+ // Charset: ("hello","LATIN1") vs ("hello","UTF-8") -> not equal
Review Comment:
Please no such comments about past bugs. They will make no sense to future readers.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7554] NlsString.compareTo inconsistent with equals/hashCode [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5141:
URL: https://github.com/apache/calcite/pull/5141#issuecomment-5160134941
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5141) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5141&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5141&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5141&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5141&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5141&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5141)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7554] NlsString.compareTo inconsistent with equals/hashCode [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5141:
URL: https://github.com/apache/calcite/pull/5141#issuecomment-5159753183
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5141) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5141&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5141&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5141&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5141&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5141&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5141)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7554] NlsString.compareTo inconsistent with equals/hashCode [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu merged PR #5141:
URL: https://github.com/apache/calcite/pull/5141
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org