[CALCITE-6718] Optimize SubstitutionVisitor's splitFilter with early return and uniform simplification for equivalence checking #4078
Conversation
hannerwang
force-pushed
the
enhance_split_filter
branch
from
18bc5f7 to
dd17183
Compare
|
| if (!r.isAlwaysFalse() && isEquivalent(condition2, r)) { | ||
| RexNode simplifiedCond2 = | ||
| canonizeNode(rexBuilder, simplify.simplifyUnknownAsFalse(condition2)); | ||
| if (!r.isAlwaysFalse() && isEquivalent(simplifiedCond2, r)) { |
There was a problem hiding this comment.
I agree with Early Return, it cuts out some unnecessary steps. But doing another simplify for condition2 seems unnecessary, condition2 only canonizeNode after simplify.
How about using simplify instead of simplifyUnknownAsFalse for x2 .
There was a problem hiding this comment.
Actually I'm not sure why this function uses both simplify and simplifyUnknownAsFalse, so I made a conservative change to minimize impact. If possible, we could replace simplify with simplifyUnknownAsFalse, but that seems too radical.
There was a problem hiding this comment.
I agree with @suibianwanwank, we shouldn't need another simplification on condition2 while early return seems good.
Here we are dealing with filtering predicates, so I guess that simplifyUnknownAsFalse has the right semantics, rather than just simplify, but we need tests covering these changes and showing that we are doing the right thing on corner cases involving unknown and the early return condition.
@hannerwang, can you add them?
There was a problem hiding this comment.
Hello @asolimando ,
I attempted to change simplifyUnknownAsFalse to simplify, but I encountered some issues.
For instance, with the expression x >= 5 and x > 5, the simplify method does not reduce it, whereas simplifyUnknownAsFalse simplifies it to x > 5. Additionally, expressions like x > 5 and x < 3 are simplified to x <> x when UNKNOWN is treated as UNKNOWN, and to false when UNKNOWN is treated as FALSE.
Therefore, we cannot simply replace simplifyUnknownAsFalse with simplify, as it would cause many tests to fail.
There was a problem hiding this comment.
I was actually suggesting the opposite, simplifyUnknownAsFalse seems the right simplification to be used in the context of filtering predicares, we could use it for simplifying condition and target at the very beginning to keep it consistent with the simplification of x2 later on. To validate that we aren't missing anything here I was suggesting that we need tests covering cases where Unknown is involved.
If those tests already exist it's fine, we just need to validate that we are doing the right thing. I hope it's clearer now, I am sorry if I confused you with my previous comment.
hannerwang
force-pushed
the
enhance_split_filter
branch
from
dd17183 to
9f87b8f
Compare
…return and uniform simplification for equivalence checking
hannerwang
force-pushed
the
enhance_split_filter
branch
from
9f87b8f to
cf9b38d
Compare
| if (target.isAlwaysTrue()) { | ||
| return condition2; | ||
| } | ||
| target = simplify.simplify(target); |
There was a problem hiding this comment.
Can this target be 'true' after simplifying? Do we need to handle when the target is always false?
What changes were proposed in this pull request?
Why are the changes needed?
Early Return: Many materialized views do not have filters, so implementing early return can avoid unnecessary checks and improve performance.
Uniform Simplification: In user-customized applications, we cannot guarantee that both sides of the equivalence check have been simplified using simplifyUnknownAsFalse. Adding this ensures consistent behavior.
Does this PR introduce any user-facing change?
No.