This is an automated email from the ASF dual-hosted git repository.
mihaibudiu pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/calcite.git
The following commit(s) were added to refs/heads/main by this push:
new 546a2b2e57 [CALCITE-7725] Review safety of checked arithmetic operators
546a2b2e57 is described below
commit 546a2b2e576ee51a92acba6467308a4d20088d5b
Author: Mihai Budiu <[email protected]>
AuthorDate: Wed Aug 19 21:20:14 2026 -0700
[CALCITE-7725] Review safety of checked arithmetic operators
Signed-off-by: Mihai Budiu <[email protected]>
---
.../java/org/apache/calcite/prepare/Prepare.java | 6 ++++++
.../java/org/apache/calcite/rex/RexAnalyzer.java | 7 ++++++
.../main/java/org/apache/calcite/rex/RexCall.java | 14 ++++++++----
.../java/org/apache/calcite/rex/RexSimplify.java | 20 +++++++++++------
.../apache/calcite/sql2rel/SqlToRelConverter.java | 16 ++++++++++++++
.../org/apache/calcite/rex/RexProgramTest.java | 24 +++++++++++++++++++++
core/src/test/resources/sql/cast.iq | 25 ++++++++++++++++++++++
7 files changed, 102 insertions(+), 10 deletions(-)
diff --git a/core/src/main/java/org/apache/calcite/prepare/Prepare.java
b/core/src/main/java/org/apache/calcite/prepare/Prepare.java
index 1f2e96688a..eac4fa6dd3 100644
--- a/core/src/main/java/org/apache/calcite/prepare/Prepare.java
+++ b/core/src/main/java/org/apache/calcite/prepare/Prepare.java
@@ -260,6 +260,12 @@ public PreparedResult prepareSql(
// Convert some operations to use checked arithmetic:
// - all arithmetic operations on exact types if the conformance requires
checked arithmetic
// - all arithmetic that produces INTERVAL results, regardless of the
conformance
+ //
+ // SqlToRelConverter already runs ConvertToChecked. This second conversion
is needed for:
+ // - INTERVAL arithmetic under a conformance without checked
+ // arithmetic (SqlToRelConverter installs no converter at all)
+ // - expressions that SqlToRelConverter does not build through
+ // Blackboard#convertExpression
ConvertToChecked checkedConv =
new ConvertToChecked(root.rel.getCluster().getRexBuilder(),
convertToChecked);
RelNode rel = checkedConv.visit(root.rel);
diff --git a/core/src/main/java/org/apache/calcite/rex/RexAnalyzer.java
b/core/src/main/java/org/apache/calcite/rex/RexAnalyzer.java
index a124168137..5ca25da265 100644
--- a/core/src/main/java/org/apache/calcite/rex/RexAnalyzer.java
+++ b/core/src/main/java/org/apache/calcite/rex/RexAnalyzer.java
@@ -19,6 +19,7 @@
import org.apache.calcite.linq4j.Linq4j;
import org.apache.calcite.plan.RelOptPredicateList;
import org.apache.calcite.rel.metadata.NullSentinel;
+import org.apache.calcite.sql.SqlKind;
import org.apache.calcite.util.NlsString;
import org.apache.calcite.util.Pair;
import org.apache.calcite.util.Util;
@@ -136,6 +137,12 @@ private static class VariableCollector extends
RexVisitorImpl<Void> {
}
@Override public Void visitCall(RexCall call) {
+ if (SqlKind.CHECKED_ARITHMETIC.contains(call.getKind())) {
+ // RexInterpreter computes with unbounded values, so it cannot tell
+ // whether checked arithmetic overflows
+ ++unsupportedCount;
+ return null;
+ }
switch (call.getKind()) {
case CAST:
case M2V:
diff --git a/core/src/main/java/org/apache/calcite/rex/RexCall.java
b/core/src/main/java/org/apache/calcite/rex/RexCall.java
index deac203607..27d93189c5 100644
--- a/core/src/main/java/org/apache/calcite/rex/RexCall.java
+++ b/core/src/main/java/org/apache/calcite/rex/RexCall.java
@@ -222,12 +222,15 @@ private boolean digestWithType() {
// Only boolean-valued calls can be always-true; e.g. CAST(TRUE AS INTEGER)
// evaluates to 1 (INTEGER), not a boolean, even though its operand is
// always true.
+ // An expression that may throw is never always-true: "1 / 0 IS NOT NULL"
+ // raises an error rather than returning TRUE.
if (getType().getSqlTypeName() != SqlTypeName.BOOLEAN) {
return false;
}
switch (getKind()) {
case IS_NOT_NULL:
- return !operands.get(0).getType().isNullable();
+ return !operands.get(0).getType().isNullable()
+ && RexSimplify.isSafeExpression(operands.get(0));
case IS_NOT_TRUE:
case IS_FALSE:
case NOT:
@@ -240,7 +243,8 @@ private boolean digestWithType() {
final Sarg<?> sarg = ((RexLiteral)
operands.get(1)).getValueAs(Sarg.class);
return requireNonNull(sarg, "sarg").isAll()
&& (sarg.nullAs == RexUnknownAs.TRUE
- || !operands.get(0).getType().isNullable());
+ || !operands.get(0).getType().isNullable())
+ && RexSimplify.isSafeExpression(operands.get(0));
default:
return false;
}
@@ -253,7 +257,8 @@ private boolean digestWithType() {
}
switch (getKind()) {
case IS_NULL:
- return !operands.get(0).getType().isNullable();
+ return !operands.get(0).getType().isNullable()
+ && RexSimplify.isSafeExpression(operands.get(0));
case IS_NOT_TRUE:
case IS_FALSE:
case NOT:
@@ -266,7 +271,8 @@ private boolean digestWithType() {
final Sarg<?> sarg = ((RexLiteral)
operands.get(1)).getValueAs(Sarg.class);
return requireNonNull(sarg, "sarg").isNone()
&& (sarg.nullAs == RexUnknownAs.FALSE
- || !operands.get(0).getType().isNullable());
+ || !operands.get(0).getType().isNullable())
+ && RexSimplify.isSafeExpression(operands.get(0));
default:
return false;
}
diff --git a/core/src/main/java/org/apache/calcite/rex/RexSimplify.java
b/core/src/main/java/org/apache/calcite/rex/RexSimplify.java
index 386964963c..fe9f7b5ca0 100644
--- a/core/src/main/java/org/apache/calcite/rex/RexSimplify.java
+++ b/core/src/main/java/org/apache/calcite/rex/RexSimplify.java
@@ -1034,8 +1034,11 @@ private RexNode simplifyNot(RexCall call, RexUnknownAs
unknownAs) {
private RexNode simplifyUnaryMinus(RexCall call, RexUnknownAs unknownAs) {
final RexNode a = call.getOperands().get(0);
- if (a.getKind() == SqlKind.MINUS_PREFIX) {
- // -(-(x)) ==> x
+ if (call.getKind() == SqlKind.MINUS_PREFIX
+ && a.getKind() == SqlKind.MINUS_PREFIX) {
+ // -(-(x)) ==> x.
+ // Not valid for checked arithmetic, where negation of the minimum value
+ // of the type throws.
return simplify(((RexCall) a).getOperands().get(0), unknownAs);
}
return simplifyGenericNode(call);
@@ -1548,13 +1551,9 @@ enum SafeRexVisitor implements RexVisitor<Boolean> {
safeOps.add(SqlKind.ARRAY_VALUE_CONSTRUCTOR);
safeOps.add(SqlKind.PLUS_PREFIX);
safeOps.add(SqlKind.MINUS_PREFIX);
- safeOps.add(SqlKind.CHECKED_MINUS_PREFIX);
safeOps.add(SqlKind.PLUS);
safeOps.add(SqlKind.MINUS);
safeOps.add(SqlKind.TIMES);
- safeOps.add(SqlKind.CHECKED_PLUS);
- safeOps.add(SqlKind.CHECKED_MINUS);
- safeOps.add(SqlKind.CHECKED_TIMES);
safeOps.add(SqlKind.IS_FALSE);
safeOps.add(SqlKind.IS_NOT_FALSE);
safeOps.add(SqlKind.IS_TRUE);
@@ -1599,6 +1598,13 @@ enum SafeRexVisitor implements RexVisitor<Boolean> {
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.
+ return RexVisitorImpl.visitArrayAnd(this, call.operands)
+ && call.operands.stream().anyMatch(o -> RexUtil.isNullLiteral(o,
true));
+ }
+
switch (sqlKind) {
case DIVIDE:
case MOD:
@@ -1683,6 +1689,8 @@ enum SafeRexVisitor implements RexVisitor<Boolean> {
*
* <p>Division is an unsafe operator; consider the following:
* <pre>case when a > 0 then 1 / a else null end</pre>
+ *
+ * <p>Checked arithmetic is unsafe too, because it throws on overflow
*/
static boolean isSafeExpression(RexNode r) {
return r.accept(SafeRexVisitor.INSTANCE);
diff --git
a/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
b/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
index 357441d316..c486e8ccde 100644
--- a/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
+++ b/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
@@ -291,6 +291,9 @@ public class SqlToRelConverter {
private int explainParamCount;
public final SqlToRelConverter.Config config;
private final RelBuilder relBuilder;
+ /** Rewrites arithmetic into checked arithmetic; null if the conformance
+ * does not require checked arithmetic. */
+ private final @Nullable RexShuttle checkedConverter;
/**
* Fields used in name resolution for correlated sub-queries.
@@ -377,6 +380,13 @@ public SqlToRelConverter(
config.getRelBuilderFactory().create(cluster,
validator != null ?
validator.getCatalogReader().unwrap(RelOptSchema.class) : null)
.transform(config.getRelBuilderConfigTransform());
+ // Simplification assumes that arithmetic never throws, which is wrong for
+ // checked arithmetic; so every expression is converted to checked
+ // arithmetic as soon as it is built, before anything can simplify it
+ this.checkedConverter =
+ validator != null &&
validator.config().conformance().checkedArithmetic()
+ ? new ConvertToChecked(rexBuilder, true).converter
+ : null;
this.hintStrategies = config.getHintStrategyTable();
cluster.setHintStrategies(this.hintStrategies);
@@ -5938,6 +5948,12 @@ ImmutableList<RelNode> retrieveCursors() {
}
@Override public RexNode convertExpression(SqlNode expr) {
+ final RexNode rex = convertExpression0(expr);
+ // Convert arithmetic to checked arithmetic if needed
+ return checkedConverter == null ? rex : rex.accept(checkedConverter);
+ }
+
+ private RexNode convertExpression0(SqlNode expr) {
// If we're in aggregation mode and this is an expression in the
// GROUP BY clause, return a reference to the field.
AggConverter agg = this.agg;
diff --git a/core/src/test/java/org/apache/calcite/rex/RexProgramTest.java
b/core/src/test/java/org/apache/calcite/rex/RexProgramTest.java
index 332f1865a7..ca063c4855 100644
--- a/core/src/test/java/org/apache/calcite/rex/RexProgramTest.java
+++ b/core/src/test/java/org/apache/calcite/rex/RexProgramTest.java
@@ -4788,6 +4788,30 @@ private SqlSpecialOperatorWithPolicy(String name,
SqlKind kind, int prec, boolea
checkSimplify(add(zero, sub(nullInt, nullInt)), "null:INTEGER");
}
+ /** Unit test for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7725";>[CALCITE-7725]
+ * Review safety of checked arithmetic operators</a>. */
+ @Test void testSimplifyCheckedArithmetic() {
+ final RexNode a = vIntNotNull(1);
+ final RexNode b = vIntNotNull(2);
+ final RexNode checkedMul =
+ rexBuilder.makeCall(SqlStdOperatorTable.CHECKED_MULTIPLY, a, b);
+
+ // Unchecked arithmetic wraps around, so it never throws
+ checkSimplify(isNotNull(mul(a, b)), "true");
+ checkSimplify(add(mul(a, b), nullInt), "null:INTEGER");
+
+ // Checked arithmetic throws on overflow
+ checkSimplifyUnchanged(isNotNull(checkedMul));
+ checkSimplifyUnchanged(
+ rexBuilder.makeCall(SqlStdOperatorTable.CHECKED_PLUS, checkedMul,
nullInt));
+
+ // A checked operation with a NULL operand is never performed, hence safe
+ checkSimplify(
+ rexBuilder.makeCall(SqlStdOperatorTable.CHECKED_PLUS, a, nullInt),
+ "null:INTEGER");
+ }
+
@Test void testSimplifyCastWithConstantReduction() {
RexNode dateStr = literal("2020-10-30");
RelDataType nullableDateType =
diff --git a/core/src/test/resources/sql/cast.iq
b/core/src/test/resources/sql/cast.iq
index ce7b13b8b9..832c99e4d2 100644
--- a/core/src/test/resources/sql/cast.iq
+++ b/core/src/test/resources/sql/cast.iq
@@ -65,6 +65,31 @@ select 2147483647 * 2147483647;
Caused by: java.lang.ArithmeticException
!error
+# Test cases for [CALCITE-7725] Review safety of checked arithmetic operators
+# https://issues.apache.org/jira/browse/CALCITE-7725
+
+# A checked operation with a NULL operand is never performed, so the whole
+# expression can still be simplified to NULL
+select cast(null as integer) + empno as c from emp where empno = 7369;
++---+
+| C |
++---+
+| |
++---+
+(1 row)
+
+!ok
+
+# "empno * 100000000" overflows, so "IS NOT NULL" must not become TRUE
+select empno from emp where empno = 7369 and empno * 100000000 is not null;
+integer overflow
+!error
+
+# and "x + NULL" must not become NULL without computing x
+select empno * 100000000 + cast(null as integer) as c from emp where empno =
7369;
+integer overflow
+!error
+
!use scott
# Cast a character literal to a timestamp; note: the plan does not contain CAST
