You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "OldTruckDriver (via GitHub)" <gi...@apache.org> on 2026/06/09 12:40:56 UTC
[PR] [CALCITE-7553] Separate TIMESTAMP_TZ comparison semantics [calcite]
OldTruckDriver opened a new pull request, #5007:
URL: https://github.com/apache/calcite/pull/5007
Fixes CALCITE-7553.
This PR separates TIMESTAMP_TZ's Java natural ordering from SQL value comparison semantics.
TimestampWithTimeZoneString.compareTo now compares by instant first, then by the canonical string value as a tie-breaker. This makes compareTo consistent with equals/hashCode, so Java sorted collections do not collapse distinct TIMESTAMP_TZ values that represent the same instant.
SQL comparison paths now use an explicit Rex SQL value comparison helper. For TIMESTAMP_TZ, the helper compares instants; other values keep their existing natural ordering. This preserves SQL semantics for literal folding and RexInterpreter evaluation.
RexBuilder and RexSimplify now avoid constructing Sarg ranges for TIMESTAMP_TZ values in the affected paths, because Sarg still relies on natural Comparable ordering. This keeps IN/BETWEEN and simplification paths from using the wrong ordering. Full TIMESTAMP_TZ SQL-value comparison support inside Sarg/range handling is left for a follow-up PR.
The TIMESTAMP_TZ epoch-millis conversion in RexLiteral now delegates to TimestampWithTimeZoneString.getMillisSinceEpoch(), using the same instant calculation as the comparison helper.
Tests:
- ./gradlew :core:test -Djunit.jupiter.execution.parallel.enabled=false --tests org.apache.calcite.rex.RexProgramTest --tests org.apache.calcite.rex.RexBuilderTest
- ./gradlew :core:check
--
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-7553] Separate TIMESTAMP_TZ comparison semantics [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #5007:
URL: https://github.com/apache/calcite/pull/5007#discussion_r3423080970
##########
core/src/main/java/org/apache/calcite/util/ComparableSqlValue.java:
##########
@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.util;
+
+/**
+ * Value whose SQL comparison semantics differ from its natural ordering.
Review Comment:
This seems to be defined only for constant values (a concept which includes literals, but also perhaps constructors applied to constant arguments). The term "value" you used is very overloaded - it can be applied to runtime values as well.
##########
core/src/main/java/org/apache/calcite/util/SqlValueComparator.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.util;
+
+import java.util.Comparator;
+
+/** Compares values using SQL value semantics. */
+public final class SqlValueComparator implements Comparator<Comparable> {
+ /** Singleton instance. */
+ public static final SqlValueComparator INSTANCE = new SqlValueComparator();
+
+ private SqlValueComparator() {
+ }
+
+ @SuppressWarnings({"rawtypes", "unchecked"})
+ @Override public int compare(Comparable v0, Comparable v1) {
+ if (hasCustomSqlValueComparison(v0, v1)) {
+ return ((ComparableSqlValue) v0).compareSqlValueTo(v1);
+ }
+ return v0.compareTo(v1);
+ }
+
+ /** Returns whether two values should use custom SQL-value comparison. */
+ public boolean hasCustomSqlValueComparison(Comparable v0, Comparable v1) {
Review Comment:
Is the two-argument version of this function really necessary?
Is there a case where the one-argument and the two-argument would return different results for the same first argument?
--
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-7553] Separate TIMESTAMP_TZ comparison semantics [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #5007:
URL: https://github.com/apache/calcite/pull/5007#issuecomment-4726063951
I thought we discussed about making changes in new commits until the review is finished.
--
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-7553] Separate TIMESTAMP_TZ comparison semantics [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5007:
URL: https://github.com/apache/calcite/pull/5007#issuecomment-4726050759
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5007) **Quality Gate passed**
Issues
 [5 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5007&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5007&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5007&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [84.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5007&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5007&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5007)
--
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-7553] Separate TIMESTAMP_TZ comparison semantics [calcite]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5007:
URL: https://github.com/apache/calcite/pull/5007#issuecomment-5009904684
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions.
--
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-7553] Separate TIMESTAMP_TZ comparison semantics [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #5007:
URL: https://github.com/apache/calcite/pull/5007#issuecomment-4664145473
I think this deserves a little design discussion in Jira prior to an implementation.
I would expect to see an interface e.g., ComparableSqlValue implemented by all SqlLiterals.
Alternatively, there could be a Comparator interface implementation for SqlLiterals.
--
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-7553] Separate TIMESTAMP_TZ comparison semantics [calcite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #5007:
URL: https://github.com/apache/calcite/pull/5007#discussion_r3612771374
##########
core/src/main/java/org/apache/calcite/rex/RexBuilder.java:
##########
Review Comment:
it need to have \@Nullable declaration ?
--
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-7553] Separate TIMESTAMP_TZ comparison semantics [calcite]
Posted by "OldTruckDriver (via GitHub)" <gi...@apache.org>.
OldTruckDriver commented on PR #5007:
URL: https://github.com/apache/calcite/pull/5007#issuecomment-4688205892
Thanks. My intent with this PR was to separate Java natural ordering from SQL value comparison semantics, but the current helper is only a local implementation of that idea.
I will move the design proposal to Jira and wait for agreement there before updating this PR. In particular, I will describe the two options you mentioned: a ComparableSqlValue-style interface, or a central Comparator for SQL literal values.
--
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-7553] Separate TIMESTAMP_TZ comparison semantics [calcite]
Posted by "OldTruckDriver (via GitHub)" <gi...@apache.org>.
OldTruckDriver commented on code in PR #5007:
URL: https://github.com/apache/calcite/pull/5007#discussion_r3425499738
##########
core/src/main/java/org/apache/calcite/util/SqlValueComparator.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.util;
+
+import java.util.Comparator;
+
+/** Compares values using SQL value semantics. */
+public final class SqlValueComparator implements Comparator<Comparable> {
+ /** Singleton instance. */
+ public static final SqlValueComparator INSTANCE = new SqlValueComparator();
+
+ private SqlValueComparator() {
+ }
+
+ @SuppressWarnings({"rawtypes", "unchecked"})
+ @Override public int compare(Comparable v0, Comparable v1) {
+ if (hasCustomSqlValueComparison(v0, v1)) {
+ return ((ComparableSqlValue) v0).compareSqlValueTo(v1);
+ }
+ return v0.compareTo(v1);
+ }
+
+ /** Returns whether two values should use custom SQL-value comparison. */
+ public boolean hasCustomSqlValueComparison(Comparable v0, Comparable v1) {
Review Comment:
Thanks, good point. I renamed the interface and comparator to use "constant" rather than "value" (`ComparableSqlConstant`, `SqlConstantComparator`), since this is about constant expression representations rather than runtime values.
I also removed the two-argument public predicate. Call sites now only use the one-argument `hasCustomSqlConstantComparison`; the comparator keeps the same-class check internally before invoking custom SQL constant comparison.
--
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-7553] Separate TIMESTAMP_TZ comparison semantics [calcite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #5007:
URL: https://github.com/apache/calcite/pull/5007#discussion_r3612771374
##########
core/src/main/java/org/apache/calcite/rex/RexBuilder.java:
##########
Review Comment:
it need to have \@Nullable declaration ?
--
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-7553] Separate TIMESTAMP_TZ comparison semantics [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5007:
URL: https://github.com/apache/calcite/pull/5007#issuecomment-4659932145
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5007) **Quality Gate passed**
Issues
 [5 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5007&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5007&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5007&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [84.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5007&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5007&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5007)
--
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-7553] Separate TIMESTAMP_TZ comparison semantics [calcite]
Posted by "OldTruckDriver (via GitHub)" <gi...@apache.org>.
OldTruckDriver commented on PR #5007:
URL: https://github.com/apache/calcite/pull/5007#issuecomment-4718764975
I updated the local version to introduce a `ComparableSqlValue<T>` interface and a shared `SqlValueComparator`. `TimestampWithTimeZoneString` implements the interface, so its Java natural ordering can remain consistent with `equals`, while SQL-value comparison uses the explicit comparator path.
--
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