impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5856: Fix outer join predicate assignment.
Date Thu, 14 Sep 2017 00:00:26 GMT
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 <alex.behm@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message