asolimando commented on code in PR #4562:
URL: https://github.com/apache/calcite/pull/4562#discussion_r2398484530
##########
core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml:
##########
@@ -6975,16 +6975,18 @@ LogicalIntersect(all=[false])
</Resource>
<Resource name="planAfter">
<![CDATA[
-LogicalProject(ENAME=[CAST($0):VARCHAR])
- LogicalAggregate(group=[{0}])
- LogicalJoin(condition=[IS NOT DISTINCT FROM(CAST($0):VARCHAR,
CAST($1):VARCHAR)], joinType=[semi])
- LogicalJoin(condition=[=(CAST($0):VARCHAR, $1)], joinType=[semi])
+LogicalAggregate(group=[{0}])
+ LogicalJoin(condition=[IS NOT DISTINCT FROM($0, $1)], joinType=[semi])
+ LogicalJoin(condition=[IS NOT DISTINCT FROM($0, $1)], joinType=[semi])
+ LogicalProject(ENAME=[CAST($0):VARCHAR])
LogicalProject(ENAME=[$1])
LogicalFilter(condition=[=($7, 10)])
LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+ LogicalProject(ENAME=[CAST($0):VARCHAR])
LogicalProject(DEPTNO=[CAST($7):VARCHAR NOT NULL])
LogicalFilter(condition=[OR(=($1, 'a'), =($1, 'b'))])
LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+ LogicalProject(ENAME=[CAST($0):VARCHAR])
Review Comment:
https://github.com/apache/calcite/pull/4562#discussion_r2390182429 <-- IIUC,
this is the comment that replies to your question.
In that comment I explained exactly why we need that projection, there is
code attached to see the effect in practice, if you run it locally, you will
see that:
> JoinPushExpressionsRule is working on each join separately, it has no
global vision of repeated predicates it pushes down, and it's probably not
worth trying to enforce that either, but this leaves us with little choice on
how to tackle this, either we have a project or we accept redundant elements A,
A0 etc.
As you say, the same join condition will anyway be pushed down as a
projection in later stages, but in this way we avoid `JoinPushExpressionsRule`
creating redundant conditions like in the original version.
At the moment I don't see a way where we can do better without going back to
the original problem, the only alternative would be to make
`JoinPushExpressionsRule` more complex and context-aware, but I am not sure
it's worth the trouble.
PS: I am not in a rush for merging this as it's a (slight) improvement, not
a bug-fix, I am happy to discuss further with you or anybody interested in
this, but I am under the impression that we are running in circles at this
point, unless I missed something along the way. If you think I missed a point,
please come up with a concrete example so it's easier to check if there is
alignment or not
--
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]
