You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "rubenada (via GitHub)" <gi...@apache.org> on 2026/08/27 10:44:52 UTC
[PR] [CALCITE-7746] Review operation safety on arithmetic on dates and intervals [calcite]
rubenada opened a new pull request, #5228:
URL: https://github.com/apache/calcite/pull/5228
<!--
Thanks for sending a pull request! Here are some critical tips for you:
1. READ THE GUIDE FIRST: https://calcite.apache.org/develop/#contributing
*For significant contributions, please discuss on the dev mailing list or Jira BEFORE coding.*
2. JIRA IS USUALLY MANDATORY: Ensure you have created an issue on the Calcite Jira:
https://issues.apache.org/jira/projects/CALCITE/issues
*Check existing issues first to avoid duplicates.*
*Note: A Jira is NOT required for typos and cosmetic changes (i.e., changes that are neither bugs nor features).*
3. TIMING: Strongly recommended to create the Jira BEFORE you start writing code (e.g., a day or so before posting a PR).
This gives others a chance to weigh in on your specification.
4. CRITICAL CONSISTENCY RULE
The following three items MUST match exactly in wording and meaning:
(A) The Jira Issue Title
(B) This Pull Request Title
(C) Your Git Commit Message
Format: [CALCITE-XXXX] <Description>
Example: [CALCITE-0000] Add IF NOT EXISTS clause to CREATE TABLE
Guidelines for a good Title:
- Illustrate using SQL keywords (in ALL-CAPS) rather than Java method names.
- Focus on the specification and user experience, not the implementation.
- Make it clear whether it is a bug or a feature.
- Use words like "should" to indicate desired behavior vs. current behavior (e.g., "Validator should not close model file").
5. REPRODUCTION: If fixing a bug, please provide a concise SQL example or test case to reproduce the issue for a faster review.
6. TESTING: Ensure `./gradlew build` passes and appropriate tests are added/updated.
-->
## Jira Link
[CALCITE-7746](https://issues.apache.org/jira/browse/CALCITE-7746)
## Changes Proposed
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR.
-->
Review operation safety on arithmetic on dates and intervals: treat them as it happens already with CHECKED_ARITHMETIC (since they can also throw at runtime).
The current check "so it is only safe when the arithmetic is never performed, i.e. when an operand is NULL", needed to be improved, because in same cases we may have an operand that is not a NULL literal, but will become effectively a NULL literal upon simplification (e.g. the operand is another RexCall containing a NULL literal inside). Without this adjustment, we'd get regressions (e.g. SqlOperatorTest.testCeilFuncInterval would fail on the last 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-7746] Review operation safety on arithmetic on dates and intervals [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #5228:
URL: https://github.com/apache/calcite/pull/5228#discussion_r3874747864
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1545,15 +1556,25 @@ private static List<RexNode> toCaseOperands(List<CaseBranch> branches) {
/**
* Decides whether it is safe to flatten the given CASE part into ANDs/ORs.
*/
- enum SafeRexVisitor implements RexVisitor<Boolean> {
- INSTANCE;
+ private static class SafeRexVisitor implements RexVisitor<Boolean> {
Review Comment:
is this javadoc still accurate?
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1614,16 +1635,23 @@ private boolean isSafe(RexCall call, boolean deep) {
SqlKind sqlKind = call.getKind();
SqlOperator sqlOperator = call.getOperator();
- if (SqlKind.CHECKED_ARITHMETIC.contains(sqlKind)) {
- // Checked arithmetic throws on overflow, so it is only safe when the
- // arithmetic is never performed, i.e. when an operand is NULL.
+ if (SqlKind.CHECKED_ARITHMETIC.contains(sqlKind)
Review Comment:
I am not thrilled about this: we are leaking details about specific operators in a class (RexSimplify) which should be rather general. This was already there in the previous solution, but this makes it worse.
On the other hand I am not sure I have a better proposal.
The safety should really be a property that a RexCall can report - based on inspecting it's argument types.
--
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-7746] Review operation safety on arithmetic on dates and intervals [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on code in PR #5228:
URL: https://github.com/apache/calcite/pull/5228#discussion_r3878857278
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1545,15 +1556,25 @@ private static List<RexNode> toCaseOperands(List<CaseBranch> branches) {
/**
* Decides whether it is safe to flatten the given CASE part into ANDs/ORs.
*/
- enum SafeRexVisitor implements RexVisitor<Boolean> {
- INSTANCE;
+ private static class SafeRexVisitor implements RexVisitor<Boolean> {
Review Comment:
Looked outdated indeed. I've updated it.
--
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-7746] Review operation safety on arithmetic on dates and intervals [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5228:
URL: https://github.com/apache/calcite/pull/5228#issuecomment-5439740034
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5228) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5228&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5228&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5228&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [90.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5228&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5228&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5228)
--
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-7746] Review operation safety on arithmetic on dates and intervals [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5228:
URL: https://github.com/apache/calcite/pull/5228#issuecomment-5438090367
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5228) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5228&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5228&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5228&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [90.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5228&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5228&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5228)
--
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-7746] Review operation safety on arithmetic on dates and intervals [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on code in PR #5228:
URL: https://github.com/apache/calcite/pull/5228#discussion_r3882393466
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1614,16 +1635,23 @@ private boolean isSafe(RexCall call, boolean deep) {
SqlKind sqlKind = call.getKind();
SqlOperator sqlOperator = call.getOperator();
- if (SqlKind.CHECKED_ARITHMETIC.contains(sqlKind)) {
- // Checked arithmetic throws on overflow, so it is only safe when the
- // arithmetic is never performed, i.e. when an operand is NULL.
+ if (SqlKind.CHECKED_ARITHMETIC.contains(sqlKind)
Review Comment:
Yes.... I guess in SqlOperator we could deprecate
```
public Boolean isSafeOperator() {
return false;
}
```
And start using instead:
```
public Boolean isSafeOperator(RexCall call) {
return false;
}
```
--
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-7746] Review operation safety on arithmetic on dates and intervals [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on code in PR #5228:
URL: https://github.com/apache/calcite/pull/5228#discussion_r3878890222
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1614,16 +1635,23 @@ private boolean isSafe(RexCall call, boolean deep) {
SqlKind sqlKind = call.getKind();
SqlOperator sqlOperator = call.getOperator();
- if (SqlKind.CHECKED_ARITHMETIC.contains(sqlKind)) {
- // Checked arithmetic throws on overflow, so it is only safe when the
- // arithmetic is never performed, i.e. when an operand is NULL.
+ if (SqlKind.CHECKED_ARITHMETIC.contains(sqlKind)
Review Comment:
I agree this system looks a bit ad-hoc, but it is the one we have in place (until we decide a big re-work on this mechanism).
Theoretically a third party can add new operators and override the `SqlOperator#isSafeOperator` method, which gets evaluated here; but it doesn't seem too powerful since it cannot inspect arguments. Perhaps a new method that would allow to evaluate the whole call (including arguments) would be better (but that would be out of the scope of the current PR).
--
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-7746] Review operation safety on arithmetic on dates and intervals [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5228:
URL: https://github.com/apache/calcite/pull/5228#issuecomment-5441251330
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5228) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5228&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5228&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5228&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [90.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5228&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5228&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5228)
--
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-7746] Review operation safety on arithmetic on dates and intervals [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #5228:
URL: https://github.com/apache/calcite/pull/5228#discussion_r3875396882
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1614,16 +1635,23 @@ private boolean isSafe(RexCall call, boolean deep) {
SqlKind sqlKind = call.getKind();
SqlOperator sqlOperator = call.getOperator();
- if (SqlKind.CHECKED_ARITHMETIC.contains(sqlKind)) {
- // Checked arithmetic throws on overflow, so it is only safe when the
- // arithmetic is never performed, i.e. when an operand is NULL.
+ if (SqlKind.CHECKED_ARITHMETIC.contains(sqlKind)
Review Comment:
The problem is a third party cannot add new operators without modifying this visitor
--
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-7746] Review operation safety on arithmetic on dates and intervals [calcite]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5228:
URL: https://github.com/apache/calcite/pull/5228#issuecomment-6183896789
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-7746] Review operation safety on arithmetic on dates and intervals [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #5228:
URL: https://github.com/apache/calcite/pull/5228#discussion_r3882225879
##########
core/src/main/java/org/apache/calcite/rex/RexSimplify.java:
##########
@@ -1614,16 +1635,23 @@ private boolean isSafe(RexCall call, boolean deep) {
SqlKind sqlKind = call.getKind();
SqlOperator sqlOperator = call.getOperator();
- if (SqlKind.CHECKED_ARITHMETIC.contains(sqlKind)) {
- // Checked arithmetic throws on overflow, so it is only safe when the
- // arithmetic is never performed, i.e. when an operand is NULL.
+ if (SqlKind.CHECKED_ARITHMETIC.contains(sqlKind)
Review Comment:
The API should take a RexCall which can access types.
--
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-7746] Review operation safety on arithmetic on dates and intervals [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5228:
URL: https://github.com/apache/calcite/pull/5228#issuecomment-5450093112
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5228) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5228&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5228&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5228&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [90.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5228&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5228&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5228)
--
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-7746] Review operation safety on arithmetic on dates and intervals [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5228:
URL: https://github.com/apache/calcite/pull/5228#issuecomment-5438321578
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5228) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5228&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5228&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5228&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [92.5% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5228&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5228&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5228)
--
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