-
Notifications
You must be signed in to change notification settings - Fork 2.5k
[CALCITE-7443] Incorrect simplification for large interval #4845
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,8 +38,8 @@ | |
| public class ConvertToChecked extends RelHomogeneousShuttle { | ||
| final ConvertRexToChecked converter; | ||
|
|
||
| public ConvertToChecked(RexBuilder builder) { | ||
| this.converter = new ConvertRexToChecked(builder); | ||
| public ConvertToChecked(RexBuilder builder, boolean allArithmetic) { | ||
| this.converter = new ConvertRexToChecked(builder, allArithmetic); | ||
| } | ||
|
|
||
| @Override public RelNode visit(RelNode other) { | ||
|
|
@@ -48,14 +48,25 @@ public ConvertToChecked(RexBuilder builder) { | |
| } | ||
|
|
||
| /** | ||
| * Visitor which rewrites an expression tree such that all | ||
| * arithmetic operations that produce numeric values use checked arithmetic. | ||
| * Visitor which rewrites an expression tree such that arithmetic operations | ||
| * use checked arithmetic. | ||
| */ | ||
| class ConvertRexToChecked extends RexShuttle { | ||
| private final RexBuilder builder; | ||
| // If true all arithmetic operations are converted. | ||
| // Otherwise, only arithmetic operations on INTERVAL values is checked. | ||
| private final boolean allArithmetic; | ||
|
|
||
| ConvertRexToChecked(RexBuilder builder) { | ||
| /** Create a converter that replaces arithmetic with checked arithmetic. | ||
| * | ||
| * @param builder RexBuilder to use. | ||
| * @param allArithmetic If true all exact arithmetic operations are converted to checked. | ||
| * If false, only operations that produce INTERVAL-typed results | ||
| * are converted to checked. | ||
| */ | ||
| ConvertRexToChecked(RexBuilder builder, boolean allArithmetic) { | ||
| this.builder = builder; | ||
| this.allArithmetic = allArithmetic; | ||
| } | ||
|
|
||
| @Override public RexNode visitSubQuery(RexSubQuery subQuery) { | ||
|
|
@@ -72,6 +83,22 @@ class ConvertRexToChecked extends RexShuttle { | |
| List<RexNode> clonedOperands = visitList(call.operands, update); | ||
| SqlKind kind = call.getKind(); | ||
| SqlOperator operator = call.getOperator(); | ||
| SqlTypeName resultType = call.getType().getSqlTypeName(); | ||
| boolean anyOperandIsInterval = false; | ||
| for (RexNode op : call.getOperands()) { | ||
| if (SqlTypeName.INTERVAL_TYPES.contains(op.getType().getSqlTypeName())) { | ||
| anyOperandIsInterval = true; | ||
| break; | ||
| } | ||
| } | ||
| boolean resultIsInterval = SqlTypeName.INTERVAL_TYPES.contains(resultType); | ||
| boolean rewrite = | ||
| // 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if the comments are finished, because there's no further content after "for example".
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the example is "it will not rewrite date subtraction" |
||
| || (resultIsInterval && anyOperandIsInterval); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is the condition anyOperandIsInterval needed?🤔
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To avoid rewriting date subtraction |
||
|
|
||
| switch (kind) { | ||
| case PLUS: | ||
| operator = SqlStdOperatorTable.CHECKED_PLUS; | ||
|
|
@@ -91,8 +118,7 @@ class ConvertRexToChecked extends RexShuttle { | |
| default: | ||
| break; | ||
| } | ||
| SqlTypeName resultType = call.getType().getSqlTypeName(); | ||
| if (resultType == SqlTypeName.DECIMAL) { | ||
| if (resultType == SqlTypeName.DECIMAL && this.allArithmetic) { | ||
| // Checked decimal arithmetic is implemented using unchecked | ||
| // arithmetic followed by a CAST, which is always checked | ||
| RexCall result; | ||
|
|
@@ -102,8 +128,9 @@ class ConvertRexToChecked extends RexShuttle { | |
| result = call; | ||
| } | ||
| return builder.makeCast(call.getParserPosition(), call.getType(), result); | ||
| } else if (!SqlTypeName.EXACT_TYPES.contains(resultType)) { | ||
| // Do not rewrite operator if the type is e.g., DOUBLE or DATE | ||
| } | ||
|
|
||
| if (!rewrite) { | ||
| operator = call.getOperator(); | ||
| } | ||
| update[0] = update[0] || operator != call.getOperator(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,32 @@ | |
| !set outputformat mysql | ||
| !use scott | ||
|
|
||
| # 5 test cases for [CALCITE-7443] Incorrect simplification for large interval | ||
| SELECT -(INTERVAL -2147483648 months); | ||
| java.lang.ArithmeticException: integer overflow | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "java.lang.ArithmeticException: INTERVAL MONTH value out of integer range"
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No problem, let's set this aside for now. |
||
|
|
||
| !error | ||
|
|
||
| SELECT INTERVAL 2147483647 years; | ||
| java.lang.ArithmeticException: integer overflow | ||
|
|
||
| !error | ||
|
|
||
| SELECT -(INTERVAL -9223372036854775.808 SECONDS); | ||
| java.lang.ArithmeticException: long overflow | ||
|
|
||
| !error | ||
|
|
||
| SELECT INTERVAL 3000000 months * 1000; | ||
| java.lang.ArithmeticException: integer overflow | ||
|
|
||
| !error | ||
|
|
||
| SELECT INTERVAL 3000000 months / .0001; | ||
| java.lang.ArithmeticException: Overflow | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if this outputs a more complete error message; I'll take a look at the code later.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After reviewing the code, it seems the error message wasn't thrown by Calcite, but rather by the Java API, which makes sense now.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| !error | ||
|
|
||
| select deptno, (select min(empno) from "scott".emp where deptno = dept.deptno) as x from "scott".dept; | ||
| +--------+------+ | ||
| | DEPTNO | X | | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add an
"@deprecated" annotation here to preserve the original method?