[CALCITE-7746] Review operation safety on arithmetic on dates and intervals - #5228
[CALCITE-7746] Review operation safety on arithmetic on dates and intervals#5228rubenada wants to merge 5 commits into
Conversation
|
| */ | ||
| enum SafeRexVisitor implements RexVisitor<Boolean> { | ||
| INSTANCE; | ||
| private static class SafeRexVisitor implements RexVisitor<Boolean> { |
There was a problem hiding this comment.
is this javadoc still accurate?
There was a problem hiding this comment.
Looked outdated indeed. I've updated it.
| 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The problem is a third party cannot add new operators without modifying this visitor
2 participants
Jira Link
CALCITE-7746
Changes Proposed
Review operation safety on arithmetic on dates and intervals: treat them as it happens already with CHECKED_ARITHMETIC (since they can also throw at runtime).
The current check "so it is only safe when the arithmetic is never performed, i.e. when an operand is NULL", needed to be improved, because in same cases we may have an operand that is not a NULL literal, but will become effectively a NULL literal upon simplification (e.g. the operand is another RexCall containing a NULL literal inside). Without this adjustment, we'd get regressions (e.g. SqlOperatorTest.testCeilFuncInterval would fail on the last check).