asolimando commented on code in PR #4078:
URL: https://github.com/apache/calcite/pull/4078#discussion_r1874928885
##########
core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java:
##########
@@ -322,7 +325,9 @@ void register(MutableRel result, MutableRel query) {
ImmutableList.of(condition2, target2));
RexNode r =
canonizeNode(rexBuilder, simplify.simplifyUnknownAsFalse(x2));
- if (!r.isAlwaysFalse() && isEquivalent(condition2, r)) {
+ RexNode simplifiedCond2 =
+ canonizeNode(rexBuilder,
simplify.simplifyUnknownAsFalse(condition2));
+ if (!r.isAlwaysFalse() && isEquivalent(simplifiedCond2, r)) {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
