[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.
mihaibudiu
added
the
LGTM-will-merge-soon
xiedeyantu
left a comment
xiedeyantu
left a comment
There was a problem hiding this comment.
Left two minor comments for reference only.
| public class ConvertToChecked extends RelHomogeneousShuttle { | ||
| final ConvertRexToChecked converter; | ||
|
|
||
| public ConvertToChecked(RexBuilder builder) { |
There was a problem hiding this comment.
Should we add an
"@deprecated" annotation here to preserve the original method?
|
|
||
| # 5 test cases for [CALCITE-7443] Incorrect simplification for large interval | ||
| SELECT -(INTERVAL -2147483648 months); | ||
| java.lang.ArithmeticException: integer overflow |
There was a problem hiding this comment.
"java.lang.ArithmeticException: INTERVAL MONTH value out of integer range"
Can the error message be made more informative? For example, when encountering an error in a complex SQL, it might be difficult to quickly pinpoint the exact location. Could we even include the specific number in the error message?
There was a problem hiding this comment.
That is a very difficult problem. The Enumerable API has not been designed for ergonomic handling of runtime errors. Normally it would (1) install catch blocks and (2) use source-position information to give more meaningful error messages. This would be out of scope in this PR anyway.
There was a problem hiding this comment.
No problem, let's set this aside for now.
3 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: