[CALCITE-7722] RexSimplify IS [NOT] NULL on a safe operator with Strong policy ANY and unsafe operands can be further simplified by rubenada · Pull Request #5184 · apache/calcite

rubenada · 2026-08-17T14:24:32Z

Jira Link

Changes Proposed

RexSimplify.simplifyIsNotNull / simplifyIsNull currently bail out of the whole simplification when the input RexCall is not fully safe (i.e. isSafeExpression(a) == false). This is stricter than necessary for operators with Strong.Policy.ANY, where IS [NOT] NULL(f(x, y, ...)) is semantically equivalent to IS [NOT] NULL(x) OR/AND IS [NOT] NULL(y) OR/AND ... — the operator itself does not need to be evaluated to compute the result.

Example (regression for downstream projects such as Hive):

Before (≤ 1.34):
IS NOT NULL(CAST(key AS DOUBLE) + 1.0) → IS NOT NULL(CAST(key AS DOUBLE))

After (≥ 1.35):
IS NOT NULL(CAST(key AS DOUBLE) + 1.0) → (unchanged)

The rewrite is dropped because CAST(key AS DOUBLE) + 1.0 is a non-lossless cast wrapped in a +, so isSafeExpression returns false — even though + is Strong.ANY and the distribution is a valid rewrite regardless of the outer call's safety.

Proposed fix:

In the Strong.Policy.ANY branch, replace the full-tree safety requirement with a shallow safety check on the outer call. Because the branch rewraps the input as IS [NOT] NULL(operand_i) and joins the results with OR/AND, add a per-operand guard to prevent RexCall.isAlwaysTrue()/isAlwaysFalse() from silently collapsing a rewrapped IS [NOT] NULL(op) whose operand is typed non-nullable but not fully safe (which would otherwise erase a throwing subexpression such as 1 / 0).

Strong.Policy.NOT_NULL and CUSTOM continue to require full-tree safety, since those branches drop the subtree entirely.

julianhyde

-1

This change should use Strong.

It is frustrating when you log an issue and provide a PR at the same time. It removes the chance to have design discussions.

rubenada · 2026-08-17T17:46:45Z

@julianhyde the patch is actually using Strong. I have update the Jira to make it more accurate with the actual fix proposal

thomasrebele

I find the idea of "shallow-safety" interesting. It would be nice if we could support simplifying ((1/0)+1) IS NOT NULL.

Side note: This PR makes me think that there are some limitations in the current code for representing the properties of the operators (e.g., CALCITE-7264). I think there was a discussion somewhere whether the possibility that a RexNode may throw an exception could be included in the type system. The safety and shallow-safety of an operator would be attached to the operator itself instead of a visitor (see Julian's comment).

The concept of "safety" and "shallow-safety" could be collapsed, i.e., an operator is safe iff its evaluation does not throw an exception. A RexNode expression is safe iff all its operators are safe. Might be a bit easier to understand than "shallow-safety".

thomasrebele · 2026-08-18T09:16:27Z

+
+    // The outer PLUS is shallow-safe, but the div(1, 0) is not, so the peel is therefore suppressed
+    checkSimplifyUnchanged(isNotNull(plus(div(literal(1), literal(0)), vIntNotNull())));
+    checkSimplifyUnchanged(isNull(plus(div(literal(1), literal(0)), vIntNotNull())));


The previous version of the PR stated the reason for not simplifying this: otherwise it would be simplified to true because RexCall#isAlwaysTrue() returns true for IS NULL(1/0). This sounds like a bug in RexCall.isAlwaysTrue() to me. Could we make RexCall#isAlwaysTrue check for the safety of the IS [NOT] NULL operand? Potentially in a follow-up ticket if this turns out to be too complex.

Correct, the problem is that RexCall#isAlwaysTrue/isAlwaysFalse of an IS [NOT] NULL on an expression that is of a non-nullable type will always return a short-circuited true/false, without considering if the expression is safe, so we can hide a rutime issue; e.g. IS NOT NULL of a division by zero of type non-nullable would be collapsed to true. IMO this is a different improvement, I think this should be handled separately, as a follow-up ticket.

UPDATE: on a second thought, maybe if we include it in here we can simplify the current patch, checking....

UPDATE II: actually, fixing the RexCall isAlwaysTrue / isAlwaysFalse simplification for IS [NOT] NULL so that it considers isSafe makes the patch simpler, and enables more simplifications; so I've just applied it on the last commit. Thanks for pointing this out @thomasrebele !

julianhyde · 2026-08-18T17:48:26Z

+1

(This overrides my earlier '-1'. Thank you to @rubenada for explaining in Jira the purpose of this change.)

julianhyde · 2026-08-18T17:55:24Z

@thomasrebele Yes, we could use better terminology. "Safe", "Strict" and "Strong" are parallel concepts, dealing with exceptions (non-termination), whether arguments get evaluated, and null values.

It's necessary to distinguish an operator's propagation characteristics (e.g. whether it returns null if and only if both its arguments are null) from the characteristics of an expression (e.g. whether it may return null, or may throw).

Even if you are able to find a good terminology that the community agrees on, implementing it is a challenge - you would have to modify the source code, potentially renaming classes, and deal with the fact that there are closed issues and commits that cannot be retrospectively changed.

… operations if they do not affect the nullability

…sitor

rubenada · 2026-08-21T17:36:59Z

+
+    // Operators with checked arithmetic: they cannot be peeled because they are not "safe"
+    // (they will throw at runtime in case of overflow)
+    checkSimplifyUnchanged(


@thomasrebele @mihaibudiu I have rebased with the latest main branch (which includes the patch "CALCITE-7725 Review safety of checked arithmetic operators").
As expected, I had to adjust the tests here, since the checked arithmetic expressions are no longer simplified (because they are not considered safe, i.e. it is now working as expected).
When you have a bit of time, could you please take another look at the PR?

sonarqubecloud · 2026-08-21T17:54:36Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
86.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

mihaibudiu · 2026-08-22T00:09:34Z

-    }
    switch (Strong.policy(a)) {
    case NOT_NULL:
+      // Drops the subtree; require full-tree safety so we don't hide runtime errors


should the comment be inside the braces?

Could be. IMO it talks about the general NOT_NULL case behavior, so it doesn't seem out of place currently.

mihaibudiu · 2026-08-22T00:12:07Z

+     * <p>Non-{@link RexCall} nodes are always shallow-safe (they cannot
+     * throw at their own level).
+     */
+    boolean isShallowSafe(RexNode node) {


Can this be called isOperatorSafe? After all, it's a property of the operator.

I'd prefer to leave the current name, since the method theoretically could accept any type of RexNode, and also to avoid confusion with SqlOperator#isSafeOperator

An alternative might be isOuterSafe.

mihaibudiu · 2026-08-22T00:14:39Z

+   */
+  @Test void testSimplifyIsNotNullDistributesAcrossStrongOpWithLossyCast() {
+    // "(CAST(?0.varchar0):INTEGER + 1) IS NOT NULL" ==> "IS NOT NULL(CAST(?0.varchar0):INTEGER)"
+    // The outer PLUS is strong AND shallow-safe; distribution keeps the


what is "distribution"?

It means the IS_NOT_NULL(expr) is "distributed" among the operands of the expression: IS_NOT_NULL(operand0) AND .... IS_NOT_NULL(operandN)

mihaibudiu · 2026-08-22T00:15:35Z

+        isNull(plus(cast(vVarchar(), tInt(true)), literal(1))),
+        "IS NULL(CAST(?0.varchar0):INTEGER)");
+
+    // Confirm this is consistent with same expression without CAST


I don't know which cast this is talking about

It means the CAST of the tests above (which contains similar IS_NOT_NULL on PLUS expression, but with CAST operand)

mihaibudiu · 2026-08-22T00:19:17Z

+        isNotNull(plus(cast(vVarchar(), tDate(true)), interval(10, TimeUnit.DAY))),
+        "IS NOT NULL(CAST(?0.varchar0):DATE)");
+    checkSimplify(
+        isNull(plus(cast(vVarchar(), tDate(true)), interval(1, TimeUnit.MONTH))),


Isn't this wrong? Arithmetic on dates should be checked.
Or maybe this kind of code can never be generated?

This seems to be consistent with how these operations are defined atm.
Is arithmetic on date/interval operands supposed to behave different compared to other operand types?

Yes, arithmetic on dates and intervals should not wrap around. For integers you can argue that wrap around may make sense (e.g., that's the Java semantics), but for dates or intervals it produces no meaningful results. Perhaps this is another bug -- an omission in ConvertToChecked?

Yes, in that case I'd consider that a separate issue, to be handled in a follow-up ticket

I agree with handling this in a follow-up ticket.

thomasrebele

LGTM

thomasrebele · 2026-08-24T06:52:13Z

+     * <p>Non-{@link RexCall} nodes are always shallow-safe (they cannot
+     * throw at their own level).
+     */
+    boolean isShallowSafe(RexNode node) {


An alternative might be isOuterSafe.

thomasrebele · 2026-08-24T07:09:44Z

+        isNotNull(plus(cast(vVarchar(), tDate(true)), interval(10, TimeUnit.DAY))),
+        "IS NOT NULL(CAST(?0.varchar0):DATE)");
+    checkSimplify(
+        isNull(plus(cast(vVarchar(), tDate(true)), interval(1, TimeUnit.MONTH))),


I agree with handling this in a follow-up ticket.

rubenada mentioned this pull request Aug 17, 2026

HIVE-29641: Upgrade Calcite to 1.42.0 apache/hive#6523

Merged

mihaibudiu reviewed Aug 17, 2026

View reviewed changes

julianhyde requested changes Aug 17, 2026

View reviewed changes

rubenada changed the title ~~[CALCITE-7722] Simplify IS NULL / IS NOT NULL expressions by removing operations if they do not affect the nullability~~ [CALCITE-7722] RexSimplify IS [NOT] NULL over a Strong.ANY call is no longer simplified when the call itself is unsafe Aug 17, 2026

rubenada changed the title ~~[CALCITE-7722] RexSimplify IS [NOT] NULL over a Strong.ANY call is no longer simplified when the call itself is unsafe~~ [CALCITE-7722] RexSimplify IS [NOT] NULL on a safe operator with Strong policy ANY and unsafe operands can be further simplified Aug 18, 2026

thomasrebele reviewed Aug 18, 2026

View reviewed changes

Comment thread core/src/test/java/org/apache/calcite/rex/RexProgramTest.java Outdated

rubenada added 2 commits August 21, 2026 18:16

[CALCITE-7722] Simplify IS NULL / IS NOT NULL expressions by removing…

0695e98

… operations if they do not affect the nullability

Adjust tests for checked arithmetic simplifications

6cb9721

rubenada force-pushed the CALCITE-7722 branch from 315d918 to 6cb9721 Compare August 21, 2026 17:24

Add "deep" flag logic in the new CHECKED_ARITHMETIC code in SafeRexVi…

f76b8bf

…sitor

rubenada commented Aug 21, 2026

View reviewed changes

mihaibudiu reviewed Aug 22, 2026

View reviewed changes

thomasrebele approved these changes Aug 24, 2026

View reviewed changes

Uh oh!

Conversation

rubenada commented Aug 17, 2026

Jira Link

Changes Proposed

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

julianhyde left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rubenada commented Aug 17, 2026

Uh oh!

thomasrebele left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rubenada Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

julianhyde commented Aug 18, 2026

Uh oh!

julianhyde commented Aug 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Aug 21, 2026

Quality Gate passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasrebele left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

julianhyde left a comment •

edited

Loading

rubenada Aug 18, 2026 •

edited

Loading