Recognize non-fallible division and modulo operator#28898
Conversation
losipiuk
force-pushed
the
lukaszos/relax-myfail-semantics-for-divis-3f5635
branch
2 times, most recently
from
06607fa to
bf4533f
Compare
losipiuk
changed the title
findepi
changed the title
|
I changed PR title to what I suggest commit title could be. |
losipiuk
force-pushed
the
lukaszos/relax-myfail-semantics-for-divis-3f5635
branch
from
bf4533f to
f1abc10
Compare
pettyjamesm
left a comment
pettyjamesm
left a comment
There was a problem hiding this comment.
LGTM after that last comment is addressed (or intentionally skipped)
| if (value == null) { | ||
| yield false; // dividing by null is fine |
There was a problem hiding this comment.
Move this before switch. It's by necessity same for everyone.
There was a problem hiding this comment.
Seems like that might be more brittle. Could there be some other type that supports "division or modulus" that would have different NULL semantics? (i.e.: the default branch)
There was a problem hiding this comment.
Would be unexpected. Generally args for arithmetic operators are not nullable (so we get null as result if we get null as any arg)
There was a problem hiding this comment.
i think the idea here is that we understand what function is in the plan, and therefore can reason about its execution properties such as "may this call fail?" or "will this be null"
@pettyjamesm are you concerned that someone adds/ operator on some other type that returns non-null (or fails) on null input (perhaps abusing / to implement path-like concatenation of varchars)
this is not impossible indeed.
to guard against that, we would need to change the canCauseDivisionByZeroError(Constant) function to exist with "can fail" on any urecognized type (and change the name).
also, we would need to verify in isModulsOrDivide that operand types are the same (otherwise i could add / with heterogenous types and "fool" the rule)...
however, i would say it's an overkill. in thousand other places we don't do such rigorous checks so doing them here would not feel really right to me.
There was a problem hiding this comment.
Yeah, probably overkill- just pointing it out since we were here.
|
(we discovered the old truth: small PRs yield higher comment density 😉 ) |
No - this is more about shitty coders. |
losipiuk
force-pushed
the
lukaszos/relax-myfail-semantics-for-divis-3f5635
branch
from
f1abc10 to
6d0b51f
Compare
|
Hopefully good now |
losipiuk
force-pushed
the
lukaszos/relax-myfail-semantics-for-divis-3f5635
branch
from
6d0b51f to
03b5568
Compare
losipiuk
force-pushed
the
lukaszos/relax-myfail-semantics-for-divis-3f5635
branch
from
03b5568 to
426f27f
Compare
losipiuk
force-pushed
the
lukaszos/relax-myfail-semantics-for-divis-3f5635
branch
from
426f27f to
0b2a42f
Compare
losipiuk
force-pushed
the
lukaszos/relax-myfail-semantics-for-divis-3f5635
branch
from
0b2a42f to
d10f879
Compare
losipiuk
force-pushed
the
lukaszos/relax-myfail-semantics-for-divis-3f5635
branch
from
d10f879 to
708a19d
Compare
Current logic for division and modulo on integer types assumed it can always fail, which prohibited pushdown of filter expressions which involved those operators. With new logic we relax the logic so if we see that divisor is constant and non-zero we asume that operator will never fail and pushdown is possible.
losipiuk
force-pushed
the
lukaszos/relax-myfail-semantics-for-divis-3f5635
branch
from
708a19d to
1e0bb5d
Compare
4 participants
Current logic for division and modulo on integer types assumed it can always fail, which prohibited pushdown of filter expressions which involved those operators.
With new logic we relax the logic so if we see that divisor is constant and non-zero we asume that operator will never fail and pushdown is possible.
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: