From 715fbcbc464e03307b2304d2c8f20158214b3584 Mon Sep 17 00:00:00 2001 From: Ruben Quesada Lopez Date: Thu, 27 Aug 2026 11:39:39 +0100 Subject: [PATCH 1/5] [CALCITE-7746] Review operation safety on arithmetic on dates and intervals --- .../java/org/apache/calcite/rex/RexCall.java | 8 +-- .../org/apache/calcite/rex/RexSimplify.java | 49 +++++++++++++++---- .../apache/calcite/rex/RexProgramTest.java | 1 - 3 files changed, 43 insertions(+), 15 deletions(-) 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 27d93189c52..794013d38e6 100644 --- a/core/src/main/java/org/apache/calcite/rex/RexCall.java +++ b/core/src/main/java/org/apache/calcite/rex/RexCall.java @@ -230,7 +230,7 @@ private boolean digestWithType() { switch (getKind()) { case IS_NOT_NULL: return !operands.get(0).getType().isNullable() - && RexSimplify.isSafeExpression(operands.get(0)); + && RexSimplify.isSafe(operands.get(0)); case IS_NOT_TRUE: case IS_FALSE: case NOT: @@ -244,7 +244,7 @@ private boolean digestWithType() { return requireNonNull(sarg, "sarg").isAll() && (sarg.nullAs == RexUnknownAs.TRUE || !operands.get(0).getType().isNullable()) - && RexSimplify.isSafeExpression(operands.get(0)); + && RexSimplify.isSafe(operands.get(0)); default: return false; } @@ -258,7 +258,7 @@ private boolean digestWithType() { switch (getKind()) { case IS_NULL: return !operands.get(0).getType().isNullable() - && RexSimplify.isSafeExpression(operands.get(0)); + && RexSimplify.isSafe(operands.get(0)); case IS_NOT_TRUE: case IS_FALSE: case NOT: @@ -272,7 +272,7 @@ private boolean digestWithType() { return requireNonNull(sarg, "sarg").isNone() && (sarg.nullAs == RexUnknownAs.FALSE || !operands.get(0).getType().isNullable()) - && RexSimplify.isSafeExpression(operands.get(0)); + && RexSimplify.isSafe(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 98fbbb781b9..5e8fd29d48c 100644 --- a/core/src/main/java/org/apache/calcite/rex/RexSimplify.java +++ b/core/src/main/java/org/apache/calcite/rex/RexSimplify.java @@ -29,6 +29,7 @@ import org.apache.calcite.sql.SqlOperator; import org.apache.calcite.sql.fun.SqlStdOperatorTable; import org.apache.calcite.sql.parser.SqlParserPos; +import org.apache.calcite.sql.type.IntervalSqlType; import org.apache.calcite.sql.type.SqlTypeCoercionRule; import org.apache.calcite.sql.type.SqlTypeFamily; import org.apache.calcite.sql.type.SqlTypeName; @@ -90,6 +91,7 @@ public class RexSimplify { final RexUnknownAs defaultUnknownAs; final boolean predicateElimination; private final RexExecutor executor; + private final SafeRexVisitor safeRexVisitor; private static final Strong STRONG = new Strong(); @@ -118,6 +120,7 @@ private RexSimplify(RexBuilder rexBuilder, RelOptPredicateList predicates, this.predicateElimination = predicateElimination; this.paranoid = paranoid; this.executor = requireNonNull(executor, "executor"); + this.safeRexVisitor = new SafeRexVisitor(this); } @Deprecated // to be removed before 2.0 @@ -1212,7 +1215,7 @@ private RexNode simplifyIs(RexCall call, RexUnknownAs unknownAs) { // simplifies to "operand0 IS NOT NULL AND operand1 IS NOT NULL"; // this branch PRESERVES the operand subtrees, so it only // needs SHALLOW safety of the outer operator - if (!SafeRexVisitor.INSTANCE.isShallowSafe(a)) { + if (!this.safeRexVisitor.isShallowSafe(a)) { return simplifiedResult; } final List operands = new ArrayList<>(); @@ -1279,7 +1282,7 @@ private RexNode simplifyIs(RexCall call, RexUnknownAs unknownAs) { return rexBuilder.makeLiteral(false); case ANY: // See symmetric comment in simplifyIsNotNull - if (!SafeRexVisitor.INSTANCE.isShallowSafe(a)) { + if (!this.safeRexVisitor.isShallowSafe(a)) { return simplifiedResult; } final List operands = new ArrayList<>(); @@ -1545,15 +1548,25 @@ private static List toCaseOperands(List branches) { /** * Decides whether it is safe to flatten the given CASE part into ANDs/ORs. */ - enum SafeRexVisitor implements RexVisitor { - INSTANCE; + private static class SafeRexVisitor implements RexVisitor { + + private static final SafeRexVisitor INSTANCE = new SafeRexVisitor(); @SuppressWarnings("ImmutableEnumChecker") private final Set safeOps; @SuppressWarnings("ImmutableEnumChecker") private final ImmutableSet safeOperators; - SafeRexVisitor() { + // Optional RexSimplify, if present it can be used for simplifications during isSafe computation + private final @Nullable RexSimplify rexSimplify; + + private SafeRexVisitor() { + this(null); + } + + SafeRexVisitor(@Nullable RexSimplify rexSimplify) { + this.rexSimplify = rexSimplify; + ImmutableSet.Builder builder = ImmutableSet.builder(); builder.addAll(SqlStdOperatorTable.QUANTIFY_OPERATORS); safeOperators = builder.build(); @@ -1614,16 +1627,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) + || (SqlKind.BINARY_ARITHMETIC.contains(sqlKind) + && call.operands.stream().anyMatch(o -> o.getType() instanceof IntervalSqlType))) { + // Checked arithmetic and binary arithmetic on intervals can throw at runtime, + // so it is only safe when the arithmetic is never performed, i.e. when an operand is NULL if (deep) { boolean areOperandsSafe = RexVisitorImpl.visitArrayAnd(this, call.operands); if (!areOperandsSafe) { return false; } } - return call.operands.stream().anyMatch(o -> RexUtil.isNullLiteral(o, true)); + return call.operands.stream() + .anyMatch( + o -> RexUtil.isNullLiteral( + // Simplify operand as much as possible + this.rexSimplify == null ? o : this.rexSimplify.simplify(o), + true)); } switch (sqlKind) { @@ -1734,7 +1754,16 @@ boolean isShallowSafe(RexNode node) { * *

Checked arithmetic is unsafe too, because it throws on overflow */ - static boolean isSafeExpression(RexNode r) { + private boolean isSafeExpression(RexNode r) { + return r.accept(this.safeRexVisitor); + } + + /** + * Static equivalent of the method above, without any RexSimplify in the context + * (thus less powerful); to be used by classes on the same package that need to perform + * a "basic" isSafe computation without having a RexSimplify at hand. + */ + static boolean isSafe(RexNode r) { return r.accept(SafeRexVisitor.INSTANCE); } 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 0dbbcd9cdec..c115c07f01d 100644 --- a/core/src/test/java/org/apache/calcite/rex/RexProgramTest.java +++ b/core/src/test/java/org/apache/calcite/rex/RexProgramTest.java @@ -3148,7 +3148,6 @@ trueLiteral, literal(1), "<(CAST(?0.varchar0):INTEGER, 100)"); } - @Disabled("[CALCITE-7746] Review operation safety on arithmetic on dates and intervals") @Test void testSimplifyIsNotNullDistributesAcrossStrongOpWithIntervals() { // Arithmetic on DATE and INTERVAL should be considered "unsafe" (since // it can throw at runtime), so simplification should not be applied From fb3bc2ae2f226bdc0e778d2818cfd175e8332c98 Mon Sep 17 00:00:00 2001 From: Ruben Quesada Lopez Date: Thu, 27 Aug 2026 12:07:33 +0100 Subject: [PATCH 2/5] Add new tests --- .../java/org/apache/calcite/rex/RexProgramTest.java | 11 +++++++++++ 1 file changed, 11 insertions(+) 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 c115c07f01d..39bf4d7cb5b 100644 --- a/core/src/test/java/org/apache/calcite/rex/RexProgramTest.java +++ b/core/src/test/java/org/apache/calcite/rex/RexProgramTest.java @@ -3167,6 +3167,14 @@ trueLiteral, literal(1), plus( plus(cast(vVarchar(), tDate(true)), interval(1, TimeUnit.MONTH)), interval(10, TimeUnit.DAY))))); + + // If one operand is effectively NULL, the simplification can happen even with an INTERVAL + checkSimplify( + isNull(plus(cast(nullVarchar, tDate(true)), interval(1, TimeUnit.MONTH))), + "true"); + checkSimplify( + isNull(plus(sub(vDate(), nullDate), interval(1, TimeUnit.MONTH))), + "true"); } @Test void testPushNotIntoCase() { @@ -4955,6 +4963,9 @@ private SqlSpecialOperatorWithPolicy(String name, SqlKind kind, int prec, boolea checkSimplify( rexBuilder.makeCall(SqlStdOperatorTable.CHECKED_PLUS, a, nullInt), "null:INTEGER"); + checkSimplify( + rexBuilder.makeCall(SqlStdOperatorTable.CHECKED_PLUS, a, plus(literal(1), nullInt)), + "null:INTEGER"); } @Test void testSimplifyCastWithConstantReduction() { From f5c97771ff0b427597aadb4c2aaf7e53a33ead86 Mon Sep 17 00:00:00 2001 From: Ruben Quesada Lopez Date: Thu, 27 Aug 2026 13:59:31 +0100 Subject: [PATCH 3/5] Correct CheckerFramework error --- core/src/main/java/org/apache/calcite/rex/RexSimplify.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 5e8fd29d48c..7b4f4e253c2 100644 --- a/core/src/main/java/org/apache/calcite/rex/RexSimplify.java +++ b/core/src/main/java/org/apache/calcite/rex/RexSimplify.java @@ -54,6 +54,7 @@ import com.google.common.collect.Sets; import com.google.common.collect.TreeRangeSet; +import org.checkerframework.checker.initialization.qual.UnderInitialization; import org.checkerframework.checker.nullness.qual.Nullable; import java.math.BigDecimal; @@ -1558,13 +1559,13 @@ private static class SafeRexVisitor implements RexVisitor { private final ImmutableSet safeOperators; // Optional RexSimplify, if present it can be used for simplifications during isSafe computation - private final @Nullable RexSimplify rexSimplify; + private final @UnderInitialization @Nullable RexSimplify rexSimplify; private SafeRexVisitor() { this(null); } - SafeRexVisitor(@Nullable RexSimplify rexSimplify) { + SafeRexVisitor(@UnderInitialization @Nullable RexSimplify rexSimplify) { this.rexSimplify = rexSimplify; ImmutableSet.Builder builder = ImmutableSet.builder(); From 694dbc9a022129b7055313056374dc3bb1c41cd0 Mon Sep 17 00:00:00 2001 From: Ruben Quesada Lopez Date: Thu, 27 Aug 2026 15:52:48 +0100 Subject: [PATCH 4/5] Move SafeRexVisitor as lazily initialized --- .../org/apache/calcite/rex/RexSimplify.java | 23 ++++++++++++------- 1 file changed, 15 insertions(+), 8 deletions(-) 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 7b4f4e253c2..c76acff1616 100644 --- a/core/src/main/java/org/apache/calcite/rex/RexSimplify.java +++ b/core/src/main/java/org/apache/calcite/rex/RexSimplify.java @@ -54,7 +54,6 @@ import com.google.common.collect.Sets; import com.google.common.collect.TreeRangeSet; -import org.checkerframework.checker.initialization.qual.UnderInitialization; import org.checkerframework.checker.nullness.qual.Nullable; import java.math.BigDecimal; @@ -92,7 +91,8 @@ public class RexSimplify { final RexUnknownAs defaultUnknownAs; final boolean predicateElimination; private final RexExecutor executor; - private final SafeRexVisitor safeRexVisitor; + /** Lazily initialized, to be obtained with {@link #getSafeRexVisitor()}. */ + private @Nullable SafeRexVisitor safeRexVisitor; private static final Strong STRONG = new Strong(); @@ -121,7 +121,6 @@ private RexSimplify(RexBuilder rexBuilder, RelOptPredicateList predicates, this.predicateElimination = predicateElimination; this.paranoid = paranoid; this.executor = requireNonNull(executor, "executor"); - this.safeRexVisitor = new SafeRexVisitor(this); } @Deprecated // to be removed before 2.0 @@ -188,6 +187,14 @@ private RexSimplify withPredicateElimination(boolean predicateElimination) { predicateElimination, paranoid, executor); } + /** Gets the {@link SafeRexVisitor} (lazily initialized). */ + private SafeRexVisitor getSafeRexVisitor() { + if (this.safeRexVisitor == null) { + this.safeRexVisitor = new SafeRexVisitor(this); + } + return this.safeRexVisitor; + } + /** Simplifies a boolean expression, always preserving its type and its * nullability. * @@ -1216,7 +1223,7 @@ private RexNode simplifyIs(RexCall call, RexUnknownAs unknownAs) { // simplifies to "operand0 IS NOT NULL AND operand1 IS NOT NULL"; // this branch PRESERVES the operand subtrees, so it only // needs SHALLOW safety of the outer operator - if (!this.safeRexVisitor.isShallowSafe(a)) { + if (!this.getSafeRexVisitor().isShallowSafe(a)) { return simplifiedResult; } final List operands = new ArrayList<>(); @@ -1283,7 +1290,7 @@ private RexNode simplifyIs(RexCall call, RexUnknownAs unknownAs) { return rexBuilder.makeLiteral(false); case ANY: // See symmetric comment in simplifyIsNotNull - if (!this.safeRexVisitor.isShallowSafe(a)) { + if (!this.getSafeRexVisitor().isShallowSafe(a)) { return simplifiedResult; } final List operands = new ArrayList<>(); @@ -1559,13 +1566,13 @@ private static class SafeRexVisitor implements RexVisitor { private final ImmutableSet safeOperators; // Optional RexSimplify, if present it can be used for simplifications during isSafe computation - private final @UnderInitialization @Nullable RexSimplify rexSimplify; + private final @Nullable RexSimplify rexSimplify; private SafeRexVisitor() { this(null); } - SafeRexVisitor(@UnderInitialization @Nullable RexSimplify rexSimplify) { + SafeRexVisitor(@Nullable RexSimplify rexSimplify) { this.rexSimplify = rexSimplify; ImmutableSet.Builder builder = ImmutableSet.builder(); @@ -1756,7 +1763,7 @@ boolean isShallowSafe(RexNode node) { *

Checked arithmetic is unsafe too, because it throws on overflow */ private boolean isSafeExpression(RexNode r) { - return r.accept(this.safeRexVisitor); + return r.accept(this.getSafeRexVisitor()); } /** From f63d38085847e37aefd53b64f06376aa1d15c125 Mon Sep 17 00:00:00 2001 From: Ruben Quesada Lopez Date: Fri, 28 Aug 2026 08:42:53 +0100 Subject: [PATCH 5/5] Update Javadoc --- core/src/main/java/org/apache/calcite/rex/RexSimplify.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 c76acff1616..8c891f9ab3b 100644 --- a/core/src/main/java/org/apache/calcite/rex/RexSimplify.java +++ b/core/src/main/java/org/apache/calcite/rex/RexSimplify.java @@ -1554,7 +1554,8 @@ private static List toCaseOperands(List branches) { } /** - * Decides whether it is safe to flatten the given CASE part into ANDs/ORs. + * Visitor to analyze a given {@link RexNode} and decide whether + * it is safe to unwind, see {@link #isSafeExpression(RexNode)}. */ private static class SafeRexVisitor implements RexVisitor {