[CALCITE-7722] RexSimplify IS [NOT] NULL on a safe operator with Strong policy ANY and unsafe operands can be further simplified

[rubenada]

Jira Link

CALCITE-7722

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]

@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".

[julianhyde]

+1

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

[julianhyde]

@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.

[mihaibudiu]

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

[rubenada]

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?

[mihaibudiu]

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?

[rubenada]

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

[thomasrebele]

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

[rubenada]

I have created https://issues.apache.org/jira/browse/CALCITE-7746 as a follow-up , but probably won't be able to work on that soon. I'd rather add a comment here mentioning that these tests might need an adjustment due to that ticket, and finalize this PR (to avoid blocking it more time). wdyt?

[mihaibudiu]

you could @Ignore("CALCITE-7746") this test too - or something equivalent.

[rubenada]

yes, that is cleaner, I'll push a commit doing that.

[mihaibudiu]

You have an approval - that's good enough. I have no objections, but I think you have raised more questions than you have answered. One is about what other operations need "safety" and don't have it correctly computed today, and the second, more important, is whether Calcite's "safety" is too strict.

[rubenada]

I understand @mihaibudiu , I'll try to work on the follow-up ticket asap.
I agree, IMO the current PR unveiled certain potential issues around the "isSafe" notion for RexSimplify, but did not introduce them, they were already there, latent; but that should not block applying the current simplification proposal, which seems a valid one.

I'll proceed with commit squash and will merge soon.

[thomasrebele]

LGTM

[thomasrebele]

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

[sonarqubecloud]

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