[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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
The API should take a RexCall which can access types.
There was a problem hiding this comment.
Yes.... I guess in SqlOperator we could deprecate
public Boolean isSafeOperator() {
return false;
}
And start using instead:
public Boolean isSafeOperator(RexCall call) {
return false;
}
|
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).