impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3063: Separate join inversion from join ordering.
Date Mon, 15 Aug 2016 18:20:50 GMT
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-3063: Separate join inversion from join ordering.
......................................................................


Patch Set 2:

(16 comments)

http://gerrit.cloudera.org:8080/#/c/3846/2/fe/src/main/java/com/cloudera/impala/analysis/Analyzer.java
File fe/src/main/java/com/cloudera/impala/analysis/Analyzer.java:

Line 125:   private boolean isStraightJoin_ = false;
nice clean-up


http://gerrit.cloudera.org:8080/#/c/3846/2/fe/src/main/java/com/cloudera/impala/planner/ExchangeNode.java
File fe/src/main/java/com/cloudera/impala/planner/ExchangeNode.java:

Line 81
do we still have these checks somewhere?


Line 95
what about this?


http://gerrit.cloudera.org:8080/#/c/3846/2/fe/src/main/java/com/cloudera/impala/planner/JoinNode.java
File fe/src/main/java/com/cloudera/impala/planner/JoinNode.java:

Line 478:     for (BinaryPredicate p: eqJoinConjuncts_) Collections.swap(p.getChildren(),
0, 1);
add BinaryPredicate.reverse()?


http://gerrit.cloudera.org:8080/#/c/3846/2/fe/src/main/java/com/cloudera/impala/planner/Planner.java
File fe/src/main/java/com/cloudera/impala/planner/Planner.java:

Line 345:    * 1. The right-hand side is a SingularRowSrcNode. We place such a node on the
the first sentence makes it sound like it inverts *if* the rhs is already a singularrowsrc,
in other words, it moves it to the lhs.


Line 355:   private void invertJoins(PlanNode root, boolean isSingleNodeExec) {
explain 2nd param


Line 360:       for (PlanNode child: root.getChildren()) {
single line


Line 372:       //    we cannot execute it efficiently.
the same is true for right-anything if it's a broadcast. do we know we're not going to run
into that?

why does a repartitioning null-aware right anti join not work?


Line 373:       if (joinNode.isStraightJoin() || joinOp.isNullAwareLeftAntiJoin()) {
what does it mean to deal with 'straight join' here and not for the entire block?


Line 380:         // Always place a singular row src on the build side because it has
has produces


Line 385:         // There is no backend support for distributed non-equi right outer/semi
joins.
this seems to contradict the condition, which triggers if !issinglenodeexec (maybe phrase
that flag as isDistrExec?)


PS2, Line 388: build-side hash table
"the materialized rhs", no? (this still applies to nlj?)


Line 398:             !(joinOp.isInnerJoin() && !joinNode.getEqJoinConjuncts().isEmpty()))
{
why can't this be applied to inner equi joins?


Line 404:     // Re-compute tuple ids since their order must correspond to the order of children.
why?


Line 414:   private PlanNode useNljForSmallBuilds(PlanNode root, Analyzer analyzer)
the name doesn't really reflect the (much narrower) semantics, find a better one.


Line 423:     if (joinNode.getJoinOp().isNullAwareLeftAntiJoin()) return root;
also check that this is really a hash join


-- 
To view, visit http://gerrit.cloudera.org:8080/3846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If86db7753fc585bb4c69612745ec0103278888a4
Gerrit-PatchSet: 2
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