Skip to content

[CALCITE-7746] Review operation safety on arithmetic on dates and intervals #5228

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.

Sign up for GitHub

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

Open
rubenada wants to merge 5 commits into
base: main
Choose a base branch
from
+64 −16
Open

[CALCITE-7746] Review operation safety on arithmetic on dates and intervals #5228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
715fbcb
[CALCITE-7746] Review operation safety on arithmetic on dates and int…
rubenada Aug 27, 2026
fb3bc2a
Add new tests
rubenada Aug 27, 2026
f5c9777
Correct CheckerFramework error
rubenada Aug 27, 2026
694dbc9
Move SafeRexVisitor as lazily initialized
rubenada Aug 27, 2026
f63d380
Update Javadoc
rubenada Aug 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions core/src/main/java/org/apache/calcite/rex/RexCall.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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;
}
Expand All @@ -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:
Expand All @@ -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;
}
Expand Down
60 changes: 49 additions & 11 deletions core/src/main/java/org/apache/calcite/rex/RexSimplify.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -90,6 +91,8 @@ public class RexSimplify {
final RexUnknownAs defaultUnknownAs;
final boolean predicateElimination;
private final RexExecutor executor;
/** Lazily initialized, to be obtained with {@link #getSafeRexVisitor()}. */
private @Nullable SafeRexVisitor safeRexVisitor;

private static final Strong STRONG = new Strong();

Expand Down Expand Up @@ -184,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.
*
Expand Down Expand Up @@ -1212,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 (!SafeRexVisitor.INSTANCE.isShallowSafe(a)) {
if (!this.getSafeRexVisitor().isShallowSafe(a)) {
return simplifiedResult;
}
final List<RexNode> operands = new ArrayList<>();
Expand Down Expand Up @@ -1279,7 +1290,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.getSafeRexVisitor().isShallowSafe(a)) {
return simplifiedResult;
}
final List<RexNode> operands = new ArrayList<>();
Expand Down Expand Up @@ -1543,17 +1554,28 @@ private static List<RexNode> toCaseOperands(List<CaseBranch> 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)}.
*/
enum SafeRexVisitor implements RexVisitor<Boolean> {
INSTANCE;
private static class SafeRexVisitor implements RexVisitor<Boolean> {

@mihaibudiu mihaibudiu Aug 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this javadoc still accurate?

@rubenada rubenada Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked outdated indeed. I've updated it.


private static final SafeRexVisitor INSTANCE = new SafeRexVisitor();

@SuppressWarnings("ImmutableEnumChecker")
private final Set<SqlKind> safeOps;
@SuppressWarnings("ImmutableEnumChecker")
private final ImmutableSet<SqlOperator> 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<SqlOperator> builder = ImmutableSet.builder();
builder.addAll(SqlStdOperatorTable.QUANTIFY_OPERATORS);
safeOperators = builder.build();
Expand Down Expand Up @@ -1614,16 +1636,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)

@mihaibudiu mihaibudiu Aug 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not thrilled about this: we are leaking details about specific operators in a class (RexSimplify) which should be rather general. This was already there in the previous solution, but this makes it worse.

On the other hand I am not sure I have a better proposal.

The safety should really be a property that a RexCall can report - based on inspecting it's argument types.

@mihaibudiu mihaibudiu Aug 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is a third party cannot add new operators without modifying this visitor

@rubenada rubenada Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this system looks a bit ad-hoc, but it is the one we have in place (until we decide a big re-work on this mechanism).

Theoretically a third party can add new operators and override the SqlOperator#isSafeOperator method, which gets evaluated here; but it doesn't seem too powerful since it cannot inspect arguments. Perhaps a new method that would allow to evaluate the whole call (including arguments) would be better (but that would be out of the scope of the current PR).

@mihaibudiu mihaibudiu Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API should take a RexCall which can access types.

@rubenada rubenada Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.... I guess in SqlOperator we could deprecate

public Boolean isSafeOperator() {
    return false;
  }

And start using instead:

  public Boolean isSafeOperator(RexCall call) {
    return false;
  }

|| (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) {
Expand Down Expand Up @@ -1734,7 +1763,16 @@ boolean isShallowSafe(RexNode node) {
*
* <p>Checked arithmetic is unsafe too, because it throws on overflow
*/
static boolean isSafeExpression(RexNode r) {
private boolean isSafeExpression(RexNode r) {
return r.accept(this.getSafeRexVisitor());
}

/**
* 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);
}

Expand Down
12 changes: 11 additions & 1 deletion core/src/test/java/org/apache/calcite/rex/RexProgramTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -3168,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() {
Expand Down Expand Up @@ -4956,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() {
Expand Down
Loading