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-3126: Conservative assignment of inner-join On-clause predicates.
Date Thu, 08 Dec 2016 01:57:25 GMT
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 <alex.behm@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message