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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-HasComments: Yes
