Alex Behm has posted comments on this change. Change subject: IMPALA-3126: Conservative assignment of inner-join On-clause predicates. ......................................................................
Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/4982/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: Line 1340: * join of the corresponding On-clause and the top-most full-outer join below > "must be assigned between": i don't find that particularly useful, because I rephrased this. Not sure if it's clearer though. The Analyzer is actually aware of some parts of the tree, e.g., look at outerJoinedTupleIds or fullOuterJoinedConjuncts. Those indirectly describe parts of the tree. Line 1348: * lowest node that materializes the required tuple ids, unless they reference > i think you can remove the 'lowest node' reference here, since this is only Agreed. Done. Line 1366: // evaluate it the join that the On-clause belongs to. > "evaluate it at ..."? Done http://gerrit.cloudera.org:8080/#/c/4982/2/testdata/workloads/functional-planner/queries/PlannerTest/outer-joins.test File testdata/workloads/functional-planner/queries/PlannerTest/outer-joins.test: Line 900: on c.id = d.id > i think we should stick to a minimal repro, ie, 2 joins. Makes sense. The minimal repro requires 3 joins though. Let me explain: A LOJ B ON(...) JOIN C ON(A.id = B.id) Here we "accidentally" assign the On-clause predicate correctly to the A/B join. We need to stick another outer join in between to make the assignment incorrect (without this patch). The min repro is: A LOJ B ON(...) ROJ C ON(...) JOIN D ON(A.id = B.id) Before this patch we assigned the On-clause predicate to the A/B join (which is wrong). Changes tests to use the min repro. -- To view, visit http://gerrit.cloudera.org:8080/4982 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idf45323ed9102ffb45c9d94a130ea3692286f215 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-HasComments: Yes
