Alex Behm has posted comments on this change. Change subject: IMPALA-5856: Fix outer join predicate assignment. ......................................................................
Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/8039/1/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: PS1, Line 1381: // Right-most full-outer join table ref that is equal to or to the left of the : // table ref materializing 'tids'. > Trying to visualize this hurts my brain :) Not sure if it helps to understa Agree, difficult to comprehend. Removed. PS1, Line 1385: this > What is "this" refer to? After reading this more carefully, I think I under Rephrased. The comment is supposed to describe what 'ojRef' is - that's why it's above 'ojRef'. PS1, Line 1398: globalState_.ojClauseByConjunct.get(e.getId()); > getOjRef(e)? Done http://gerrit.cloudera.org:8080/#/c/8039/1/testdata/workloads/functional-planner/queries/PlannerTest/outer-joins.test File testdata/workloads/functional-planner/queries/PlannerTest/outer-joins.test: PS1, Line 1020: from the On-clause of a left outer join > Out of curiosity, did you check if we have the same issue if there is a lef We have a test for that in L634. Please check if you think it's sufficient. I don't think those cases are similar because if the FOJ comes after the LOJ, then only the FOJ On-clause predicates are considered to be full-outer-joined (and will be handled by the non-full-outer join assignment code path). Line 1028: and t1.bigint_col > 10 and t2.bigint_col > 30 > To make it more interesting, add a where clause with one predicate that ref Good idea. Done. -- To view, visit http://gerrit.cloudera.org:8080/8039 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I93db34d988cb66e00aa05d7dc161e0ca47042acb Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-HasComments: Yes
