[CALCITE-7443] Incorrect simplification for large interval#4845
[CALCITE-7443] Incorrect simplification for large interval#4845mihaibudiu wants to merge 1 commit intoapache:mainfrom
Conversation
Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
| // Do not rewrite operator if the type is e.g., DOUBLE or DATE | ||
| (this.allArithmetic && SqlTypeName.EXACT_TYPES.contains(resultType)) | ||
| // But always rewrite if the type is an INTERVAL and any operand is INTERVAL | ||
| // This will not rewrite date subtraction, for example |
There was a problem hiding this comment.
I'm not sure if the comments are finished, because there's no further content after "for example".
There was a problem hiding this comment.
Yes, the example is "it will not rewrite date subtraction"
| !error | ||
|
|
||
| SELECT INTERVAL 3000000 months / .0001; | ||
| java.lang.ArithmeticException: Overflow |
There was a problem hiding this comment.
I'm not sure if this outputs a more complete error message; I'll take a look at the code later.
There was a problem hiding this comment.
After reviewing the code, it seems the error message wasn't thrown by Calcite, but rather by the Java API, which makes sense now.
There was a problem hiding this comment.
The error message is not informative, it is the innermost exception, which is not very helpful. But this is the best we can do now.
| (this.allArithmetic && SqlTypeName.EXACT_TYPES.contains(resultType)) | ||
| // But always rewrite if the type is an INTERVAL and any operand is INTERVAL | ||
| // This will not rewrite date subtraction, for example | ||
| || (resultIsInterval && anyOperandIsInterval); |
There was a problem hiding this comment.
Why is the condition anyOperandIsInterval needed?🤔
There was a problem hiding this comment.
To avoid rewriting date subtraction
caicancai
left a comment
caicancai
left a comment
There was a problem hiding this comment.
After the comments for "for example" were improved, I had no further questions.
2 participants
Jira Link
https://issues.apache.org/jira/browse/CALCITE-7443
Changes Proposed
This PR changes all arithmetic on values of type INTERVAL to use checked arithmetic.
Currently, using the default conformance, Calcite's arithmetic obeys the Java rules, i.e., overflow is just wrap-around. There is a conformance flag which allows you to change this behavior; checked arithmetic will throw on overflow (at runtime).
Values such as intervals are encoded into long/integer values, and arithmetic on intervals is compiled into suitably scaled computations in Java. However, I can argue that there is no reason for interval arithmetic to wrap-around; in fact, if it does, it can produce very surprising results, since the encoding of intervals into integers is not part of any spec (it is an implementation detail).
We already had a visitor to convert unchecked arithmetic into checked arithmetic. Previously the visitor was only invoked when the conformance required checked arithmetic. Now the visitor is always invoked, and it performs the following rewrites: