You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by "Alex Behm (Code Review)" <ge...@cloudera.org> on 2016/08/11 04:28:23 UTC
[Impala-ASF-CR] IMPALA-3063: Separate join inversion from join ordering.
Alex Behm has uploaded a new change for review.
http://gerrit.cloudera.org:8080/3846
Change subject: IMPALA-3063: Separate join inversion from join ordering.
......................................................................
IMPALA-3063: Separate join inversion from join ordering.
Before this change joins were inverted while doing join ordering.
That approach was unnecessarily complex because it required
modifying the global analysis state for correct conjunct
placement, etc. However, join inversion is independent of join
ordering, and the existing approach could lead to generating
invalid plans with distributed non-equi right outer/semi joins,
which we cannot execute in the backend.
After this change joins are inverted in a separate pass over
the single-node plan. This simplifies the inversion
logic and allows us to avoid generating those invalid plans.
This patch also moves the logic of converting hash joins to
nested-loop joins into a separate pass over the single-node plan.
Change-Id: If86db7753fc585bb4c69612745ec0103278888a4
---
M fe/src/main/java/com/cloudera/impala/analysis/Analyzer.java
M fe/src/main/java/com/cloudera/impala/analysis/SelectList.java
M fe/src/main/java/com/cloudera/impala/analysis/TableRef.java
M fe/src/main/java/com/cloudera/impala/planner/AggregationNode.java
M fe/src/main/java/com/cloudera/impala/planner/AnalyticEvalNode.java
M fe/src/main/java/com/cloudera/impala/planner/DistributedPlanner.java
M fe/src/main/java/com/cloudera/impala/planner/ExchangeNode.java
M fe/src/main/java/com/cloudera/impala/planner/HashJoinNode.java
M fe/src/main/java/com/cloudera/impala/planner/HdfsScanNode.java
M fe/src/main/java/com/cloudera/impala/planner/JoinNode.java
M fe/src/main/java/com/cloudera/impala/planner/NestedLoopJoinNode.java
M fe/src/main/java/com/cloudera/impala/planner/PlanNode.java
M fe/src/main/java/com/cloudera/impala/planner/Planner.java
M fe/src/main/java/com/cloudera/impala/planner/SelectNode.java
M fe/src/main/java/com/cloudera/impala/planner/SingleNodePlanner.java
M fe/src/main/java/com/cloudera/impala/planner/SingularRowSrcNode.java
M fe/src/main/java/com/cloudera/impala/planner/SortNode.java
M fe/src/main/java/com/cloudera/impala/planner/SubplanNode.java
M fe/src/main/java/com/cloudera/impala/planner/UnionNode.java
M fe/src/main/java/com/cloudera/impala/planner/UnnestNode.java
M testdata/workloads/functional-planner/queries/PlannerTest/implicit-joins.test
M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M testdata/workloads/functional-planner/queries/PlannerTest/nested-collections.test
M testdata/workloads/functional-planner/queries/PlannerTest/nested-loop-join.test
M testdata/workloads/functional-planner/queries/PlannerTest/outer-joins.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
27 files changed, 494 insertions(+), 410 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/3846/2
--
To view, visit http://gerrit.cloudera.org:8080/3846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: If86db7753fc585bb4c69612745ec0103278888a4
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
[Impala-ASF-CR] IMPALA-3063: Separate join inversion from join ordering.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-3063: Separate join inversion from join ordering.
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3846/4/fe/src/main/java/com/cloudera/impala/planner/Planner.java
File fe/src/main/java/com/cloudera/impala/planner/Planner.java:
Line 405: // Re-compute tuple ids because the backend assumes that their order corresponds to
> agreed, let's not fix this as part of this change.
Added a TODO in the BE class comment of NLJ.
I think we might need computeTipleIds() when we do projection.
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3063: Separate join inversion from join ordering.
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-3063: Separate join inversion from join ordering.
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/3846/2//COMMIT_MSG
Commit Message:
Line 7: IMPALA-3063: Separate join inversion from join ordering.
> Correct. Let me explain why it is not only a separation of functionality.
a) please add this explanation to the commit msg
b) follow-on work: a join b and b join a should really be symmetric/the fk/pk detection heuristic should work in both directions (having it be symmetric means that the inversion logic really can be separated from ordering, aside from doing that just for code structuring purposes).
http://gerrit.cloudera.org:8080/#/c/3846/2/testdata/workloads/functional-planner/queries/PlannerTest/outer-joins.test
File testdata/workloads/functional-planner/queries/PlannerTest/outer-joins.test:
Line 814: | |--07:HASH JOIN [RIGHT OUTER JOIN]
> Good catch. The cardinality estimates are clear: Join 08 should indeed be i
i think we want to improve the existing behavior, no? :)
http://gerrit.cloudera.org:8080/#/c/3846/2/testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
File testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test:
Line 1088: | hash predicates: l_suppkey = s_suppkey, l_partkey = ps_partkey
> We do order them based on cost, but we don't have a tie breaker to get an a
yes, i didn't mean as part of this patch. and this sounds like a ramp-up task for someone else.
Line 1777: | hash predicates: p_partkey = ps_partkey
> Another good catch. The change was only coincidentally caused by this patch
so is that fixed now?
--
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 <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3063: Separate join inversion from join ordering.
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
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 <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3063: Separate join inversion from join ordering.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-3063: Separate join inversion from join ordering.
......................................................................
Patch Set 6: Code-Review+2
--
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3063: Separate join inversion from join ordering.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-3063: Separate join inversion from join ordering.
......................................................................
Patch Set 4:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/3846/4/fe/src/main/java/com/cloudera/impala/planner/Planner.java
File fe/src/main/java/com/cloudera/impala/planner/Planner.java:
Line 385: // This inversion is only valid for local plans because there is no backend
> but you are inverting *only* if !isLocalPlan
Sorry, this comment did not make sense. Rephrased.
Line 405: // Re-compute tuple ids because the backend assumes that their order corresponds to
> where is that assumed?
it's DCHECKed in blocking-join-node.cc L102 and following
I tried removing those DCHECKs queries with inverted joins returned garbage results, so the assumption seems more ingrained than just those checks.
http://gerrit.cloudera.org:8080/#/c/3846/4/testdata/workloads/functional-planner/queries/PlannerTest/joins.test
File testdata/workloads/functional-planner/queries/PlannerTest/joins.test:
Line 728: ---- PLAN
> is the changed plan warranted based on the cardinalities?
yes and this change is also checked in based on:
https://gerrit.cloudera.org/#/c/3865/
the diffs should go away after I rebase
http://gerrit.cloudera.org:8080/#/c/3846/4/testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test
File testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test:
Line 2506: ---- PLAN
> are these changes warranted, based on the cardinalities?
These are the same plan changes we saw in this CR:
https://gerrit.cloudera.org/#/c/3865/
We see them as a diff because I've not rebased against that change yet.
We see the same changes because after this patch we do join cardinality estimation on the fully substituted equality predicates which has the same effect as the changes in
https://gerrit.cloudera.org/#/c/3865/ for some plans.
Line 5131: ---- PARALLELPLANS
> very strange, this is missing a distributed-plans section
Strange indeed. Added.
Line 5139: 34:HASH JOIN [INNER JOIN, PARTITIONED]
> is this justified?
same response as above - these changes are already checked in but I haven't rebased yet
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3063: Separate join inversion from join ordering.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-3063: Separate join inversion from join ordering.
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3846/4/fe/src/main/java/com/cloudera/impala/planner/Planner.java
File fe/src/main/java/com/cloudera/impala/planner/Planner.java:
Line 405: // Re-compute tuple ids because the backend assumes that their order corresponds to
> Added a TODO in the BE class comment of NLJ.
Typed too quickly. The TODO of course is added to the BE blocking-join-node.
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3063: Separate join inversion from join ordering.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has uploaded a new patch set (#3).
Change subject: IMPALA-3063: Separate join inversion from join ordering.
......................................................................
IMPALA-3063: Separate join inversion from join ordering.
Before this change joins were inverted while doing join ordering.
That approach was unnecessarily complex because it required
modifying the global analysis state for correct conjunct
placement, etc. However, join inversion is independent of join
ordering, and the existing approach could lead to generating
invalid plans with distributed non-equi right outer/semi joins,
which we cannot execute in the backend.
After this change joins are inverted in a separate pass over
the single-node plan. This simplifies the inversion
logic and allows us to avoid generating those invalid plans.
This patch also moves the logic of converting hash joins to
nested-loop joins into a separate pass over the single-node plan.
Change-Id: If86db7753fc585bb4c69612745ec0103278888a4
---
M fe/src/main/java/com/cloudera/impala/analysis/Analyzer.java
M fe/src/main/java/com/cloudera/impala/analysis/SelectList.java
M fe/src/main/java/com/cloudera/impala/analysis/TableRef.java
M fe/src/main/java/com/cloudera/impala/planner/AggregationNode.java
M fe/src/main/java/com/cloudera/impala/planner/AnalyticEvalNode.java
M fe/src/main/java/com/cloudera/impala/planner/DistributedPlanner.java
M fe/src/main/java/com/cloudera/impala/planner/ExchangeNode.java
M fe/src/main/java/com/cloudera/impala/planner/HashJoinNode.java
M fe/src/main/java/com/cloudera/impala/planner/HdfsScanNode.java
M fe/src/main/java/com/cloudera/impala/planner/JoinNode.java
M fe/src/main/java/com/cloudera/impala/planner/NestedLoopJoinNode.java
M fe/src/main/java/com/cloudera/impala/planner/PlanNode.java
M fe/src/main/java/com/cloudera/impala/planner/Planner.java
M fe/src/main/java/com/cloudera/impala/planner/SelectNode.java
M fe/src/main/java/com/cloudera/impala/planner/SingleNodePlanner.java
M fe/src/main/java/com/cloudera/impala/planner/SingularRowSrcNode.java
M fe/src/main/java/com/cloudera/impala/planner/SortNode.java
M fe/src/main/java/com/cloudera/impala/planner/SubplanNode.java
M fe/src/main/java/com/cloudera/impala/planner/UnionNode.java
M fe/src/main/java/com/cloudera/impala/planner/UnnestNode.java
M testdata/workloads/functional-planner/queries/PlannerTest/implicit-joins.test
M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M testdata/workloads/functional-planner/queries/PlannerTest/nested-collections.test
M testdata/workloads/functional-planner/queries/PlannerTest/nested-loop-join.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
27 files changed, 577 insertions(+), 478 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/3846/3
--
To view, visit http://gerrit.cloudera.org:8080/3846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If86db7753fc585bb4c69612745ec0103278888a4
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
[Impala-ASF-CR] IMPALA-3063: Separate join inversion from join ordering.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Hello Marcel Kornacker,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3846
to look at the new patch set (#6).
Change subject: IMPALA-3063: Separate join inversion from join ordering.
......................................................................
IMPALA-3063: Separate join inversion from join ordering.
Before this change joins were inverted while doing join ordering.
That approach was unnecessarily complex because it required
modifying the global analysis state for correct conjunct
placement, etc. However, join inversion is independent of join
ordering, and the existing approach could lead to generating
invalid plans with distributed non-equi right outer/semi joins,
which we cannot execute in the backend.
After this change joins are inverted in a separate pass over
the single-node plan. This simplifies the inversion
logic and allows us to avoid generating those invalid plans.
Note that this change is not only a separation of functionality
for the following reasons:
1. Our join cardinality estimation is not symmetric, i.e., A JOIN B
may not give the same estimate as B JOIN A due to our FK/PK detection
heuristic. In the context of this patch this means that an inverted
join may have a different cardinality estimate, so plans may change
depending on whether the inversion is done during join ordering of after.
2. We currently only invert outer/semi/anti joins based on the rhs table
ref join op. In this patch I want to preserve the existing behavior as
much as possible, but when doing the join ordering in a separate pass we
may see a join opn in a JoinNode that is different from the rhs table ref.
So in some situations the inversion behavior based on the join op could be
different and there are some examples in this patch.
This patch also moves the logic of converting hash joins to
nested-loop joins into a separate pass over the single-node plan.
Change-Id: If86db7753fc585bb4c69612745ec0103278888a4
---
M be/src/exec/blocking-join-node.h
M fe/src/main/java/com/cloudera/impala/analysis/Analyzer.java
M fe/src/main/java/com/cloudera/impala/analysis/BinaryPredicate.java
M fe/src/main/java/com/cloudera/impala/analysis/SelectList.java
M fe/src/main/java/com/cloudera/impala/analysis/TableRef.java
M fe/src/main/java/com/cloudera/impala/planner/AggregationNode.java
M fe/src/main/java/com/cloudera/impala/planner/AnalyticEvalNode.java
M fe/src/main/java/com/cloudera/impala/planner/DistributedPlanner.java
M fe/src/main/java/com/cloudera/impala/planner/ExchangeNode.java
M fe/src/main/java/com/cloudera/impala/planner/HashJoinNode.java
M fe/src/main/java/com/cloudera/impala/planner/HdfsScanNode.java
M fe/src/main/java/com/cloudera/impala/planner/JoinNode.java
M fe/src/main/java/com/cloudera/impala/planner/NestedLoopJoinNode.java
M fe/src/main/java/com/cloudera/impala/planner/PlanNode.java
M fe/src/main/java/com/cloudera/impala/planner/Planner.java
M fe/src/main/java/com/cloudera/impala/planner/SelectNode.java
M fe/src/main/java/com/cloudera/impala/planner/SingleNodePlanner.java
M fe/src/main/java/com/cloudera/impala/planner/SingularRowSrcNode.java
M fe/src/main/java/com/cloudera/impala/planner/SortNode.java
M fe/src/main/java/com/cloudera/impala/planner/SubplanNode.java
M fe/src/main/java/com/cloudera/impala/planner/UnionNode.java
M fe/src/main/java/com/cloudera/impala/planner/UnnestNode.java
M testdata/workloads/functional-planner/queries/PlannerTest/implicit-joins.test
M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M testdata/workloads/functional-planner/queries/PlannerTest/nested-collections.test
M testdata/workloads/functional-planner/queries/PlannerTest/nested-loop-join.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
29 files changed, 661 insertions(+), 363 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/3846/6
--
To view, visit http://gerrit.cloudera.org:8080/3846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If86db7753fc585bb4c69612745ec0103278888a4
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
[Impala-ASF-CR] IMPALA-3063: Separate join inversion from join ordering.
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.
Change subject: IMPALA-3063: Separate join inversion from join ordering.
......................................................................
Patch Set 6: Verified+1
--
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3063: Separate join inversion from join ordering.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has uploaded a new patch set (#4).
Change subject: IMPALA-3063: Separate join inversion from join ordering.
......................................................................
IMPALA-3063: Separate join inversion from join ordering.
Before this change joins were inverted while doing join ordering.
That approach was unnecessarily complex because it required
modifying the global analysis state for correct conjunct
placement, etc. However, join inversion is independent of join
ordering, and the existing approach could lead to generating
invalid plans with distributed non-equi right outer/semi joins,
which we cannot execute in the backend.
After this change joins are inverted in a separate pass over
the single-node plan. This simplifies the inversion
logic and allows us to avoid generating those invalid plans.
Note that this change is not only a separation of functionality
for the following reasons:
1. Our join cardinality estimation is not symmetric, i.e., A JOIN B
may not give the same estimate as B JOIN A due to our FK/PK detection
heuristic. In the context of this patch this means that an inverted
join may have a different cardinality estimate, so plans may change
depending on whether the inversion is done during join ordering of after.
2. We currently only invert outer/semi/anti joins based on the rhs table
ref join op. In this patch I want to preserve the existing behavior as
much as possible, but when doing the join ordering in a separate pass we
may see a join opn in a JoinNode that is different from the rhs table ref.
So in some situations the inversion behavior based on the join op could be
different and there are some examples in this patch.
This patch also moves the logic of converting hash joins to
nested-loop joins into a separate pass over the single-node plan.
Change-Id: If86db7753fc585bb4c69612745ec0103278888a4
---
M fe/src/main/java/com/cloudera/impala/analysis/Analyzer.java
M fe/src/main/java/com/cloudera/impala/analysis/BinaryPredicate.java
M fe/src/main/java/com/cloudera/impala/analysis/SelectList.java
M fe/src/main/java/com/cloudera/impala/analysis/TableRef.java
M fe/src/main/java/com/cloudera/impala/planner/AggregationNode.java
M fe/src/main/java/com/cloudera/impala/planner/AnalyticEvalNode.java
M fe/src/main/java/com/cloudera/impala/planner/DistributedPlanner.java
M fe/src/main/java/com/cloudera/impala/planner/ExchangeNode.java
M fe/src/main/java/com/cloudera/impala/planner/HashJoinNode.java
M fe/src/main/java/com/cloudera/impala/planner/HdfsScanNode.java
M fe/src/main/java/com/cloudera/impala/planner/JoinNode.java
M fe/src/main/java/com/cloudera/impala/planner/NestedLoopJoinNode.java
M fe/src/main/java/com/cloudera/impala/planner/PlanNode.java
M fe/src/main/java/com/cloudera/impala/planner/Planner.java
M fe/src/main/java/com/cloudera/impala/planner/SelectNode.java
M fe/src/main/java/com/cloudera/impala/planner/SingleNodePlanner.java
M fe/src/main/java/com/cloudera/impala/planner/SingularRowSrcNode.java
M fe/src/main/java/com/cloudera/impala/planner/SortNode.java
M fe/src/main/java/com/cloudera/impala/planner/SubplanNode.java
M fe/src/main/java/com/cloudera/impala/planner/UnionNode.java
M fe/src/main/java/com/cloudera/impala/planner/UnnestNode.java
M testdata/workloads/functional-planner/queries/PlannerTest/implicit-joins.test
M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M testdata/workloads/functional-planner/queries/PlannerTest/nested-collections.test
M testdata/workloads/functional-planner/queries/PlannerTest/nested-loop-join.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
28 files changed, 592 insertions(+), 476 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/3846/4
--
To view, visit http://gerrit.cloudera.org:8080/3846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If86db7753fc585bb4c69612745ec0103278888a4
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
[Impala-ASF-CR] IMPALA-3063: Separate join inversion from join ordering.
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-3063: Separate join inversion from join ordering.
......................................................................
Patch Set 4:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/3846/4/fe/src/main/java/com/cloudera/impala/planner/Planner.java
File fe/src/main/java/com/cloudera/impala/planner/Planner.java:
Line 385: // This inversion is only valid for local plans because there is no backend
but you are inverting *only* if !isLocalPlan
Line 405: // Re-compute tuple ids because the backend assumes that their order corresponds to
where is that assumed?
http://gerrit.cloudera.org:8080/#/c/3846/4/testdata/workloads/functional-planner/queries/PlannerTest/joins.test
File testdata/workloads/functional-planner/queries/PlannerTest/joins.test:
Line 728: ---- PLAN
is the changed plan warranted based on the cardinalities?
http://gerrit.cloudera.org:8080/#/c/3846/4/testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test
File testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test:
Line 2506: ---- PLAN
are these changes warranted, based on the cardinalities?
Line 5131: ---- PARALLELPLANS
very strange, this is missing a distributed-plans section
Line 5139: 34:HASH JOIN [INNER JOIN, PARTITIONED]
is this justified?
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3063: Separate join inversion from join ordering.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has uploaded a new patch set (#5).
Change subject: IMPALA-3063: Separate join inversion from join ordering.
......................................................................
IMPALA-3063: Separate join inversion from join ordering.
Before this change joins were inverted while doing join ordering.
That approach was unnecessarily complex because it required
modifying the global analysis state for correct conjunct
placement, etc. However, join inversion is independent of join
ordering, and the existing approach could lead to generating
invalid plans with distributed non-equi right outer/semi joins,
which we cannot execute in the backend.
After this change joins are inverted in a separate pass over
the single-node plan. This simplifies the inversion
logic and allows us to avoid generating those invalid plans.
Note that this change is not only a separation of functionality
for the following reasons:
1. Our join cardinality estimation is not symmetric, i.e., A JOIN B
may not give the same estimate as B JOIN A due to our FK/PK detection
heuristic. In the context of this patch this means that an inverted
join may have a different cardinality estimate, so plans may change
depending on whether the inversion is done during join ordering of after.
2. We currently only invert outer/semi/anti joins based on the rhs table
ref join op. In this patch I want to preserve the existing behavior as
much as possible, but when doing the join ordering in a separate pass we
may see a join opn in a JoinNode that is different from the rhs table ref.
So in some situations the inversion behavior based on the join op could be
different and there are some examples in this patch.
This patch also moves the logic of converting hash joins to
nested-loop joins into a separate pass over the single-node plan.
Change-Id: If86db7753fc585bb4c69612745ec0103278888a4
---
M fe/src/main/java/com/cloudera/impala/analysis/Analyzer.java
M fe/src/main/java/com/cloudera/impala/analysis/BinaryPredicate.java
M fe/src/main/java/com/cloudera/impala/analysis/SelectList.java
M fe/src/main/java/com/cloudera/impala/analysis/TableRef.java
M fe/src/main/java/com/cloudera/impala/planner/AggregationNode.java
M fe/src/main/java/com/cloudera/impala/planner/AnalyticEvalNode.java
M fe/src/main/java/com/cloudera/impala/planner/DistributedPlanner.java
M fe/src/main/java/com/cloudera/impala/planner/ExchangeNode.java
M fe/src/main/java/com/cloudera/impala/planner/HashJoinNode.java
M fe/src/main/java/com/cloudera/impala/planner/HdfsScanNode.java
M fe/src/main/java/com/cloudera/impala/planner/JoinNode.java
M fe/src/main/java/com/cloudera/impala/planner/NestedLoopJoinNode.java
M fe/src/main/java/com/cloudera/impala/planner/PlanNode.java
M fe/src/main/java/com/cloudera/impala/planner/Planner.java
M fe/src/main/java/com/cloudera/impala/planner/SelectNode.java
M fe/src/main/java/com/cloudera/impala/planner/SingleNodePlanner.java
M fe/src/main/java/com/cloudera/impala/planner/SingularRowSrcNode.java
M fe/src/main/java/com/cloudera/impala/planner/SortNode.java
M fe/src/main/java/com/cloudera/impala/planner/SubplanNode.java
M fe/src/main/java/com/cloudera/impala/planner/UnionNode.java
M fe/src/main/java/com/cloudera/impala/planner/UnnestNode.java
M testdata/workloads/functional-planner/queries/PlannerTest/implicit-joins.test
M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M testdata/workloads/functional-planner/queries/PlannerTest/nested-collections.test
M testdata/workloads/functional-planner/queries/PlannerTest/nested-loop-join.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
28 files changed, 775 insertions(+), 477 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/3846/5
--
To view, visit http://gerrit.cloudera.org:8080/3846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If86db7753fc585bb4c69612745ec0103278888a4
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
[Impala-ASF-CR] IMPALA-3063: Separate join inversion from join ordering.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-3063: Separate join inversion from join ordering.
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/3846/2//COMMIT_MSG
Commit Message:
Line 7: IMPALA-3063: Separate join inversion from join ordering.
> the changes in test output make it look like some plans also change (so thi
Correct. Let me explain why it is not only a separation of functionality.
1. Our join cardinality estimation is not symmetric, i.e., A JOIN B may not give the same estimate as B JOIN A due to our FK/PK detection heuristic. In the context of this patch this means that an inverted join may have a different cardinality estimate, so plans may change depending on whether the inversion is done during join ordering of after.
2. We currently only invert outer/semi/anti joins based on the rhs table ref join op. In this patch I want to preserve the existing behavior as much as possible, but when doing the join ordering in a separate pass we may see a join opn in a JoinNode that is different from the rhs table ref. So in some situations the inversion behavior based on the join op could be different and there are some examples in this patch.
It's hard to say which method produces better plans, but the post-ordering join inversion seems simpler and easier to reason about. Maybe you have other thoughts?
http://gerrit.cloudera.org:8080/#/c/3846/2/testdata/workloads/functional-planner/queries/PlannerTest/outer-joins.test
File testdata/workloads/functional-planner/queries/PlannerTest/outer-joins.test:
Line 814: | |--07:HASH JOIN [RIGHT OUTER JOIN]
> why is this cross product flipped?
Good catch. The cardinality estimates are clear: Join 08 should indeed be inverted.
However, I'm trying to minimize the behavioral changes in this patch and I missed the case of non-equi inner joins which we should not invert if we want to preserve the existing behavior.
http://gerrit.cloudera.org:8080/#/c/3846/2/testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
File testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test:
Line 1088: | hash predicates: l_suppkey = s_suppkey, l_partkey = ps_partkey
> we've seen meaningless changes like this before. i'm wondering whether we s
We do order them based on cost, but we don't have a tie breaker to get an absolute order.
I tried using toSql() as a tie breaker which is good enough for the explain use case (still not an absolute order).
Unfortunately, that changes a ton of plans, so I'd prefer to do that in a follow-on patch. I believe we even have a JIRA for that. Sound good?
Line 1777: | hash predicates: p_partkey = ps_partkey
> is this the right thing to do, given selectivities?
Another good catch. The change was only coincidentally caused by this patch. There was an independent bug in estimating cross join cardinalities which lead us to compute different cardinalities for:
NESTED_LOOP_JOIN
UNNEST
SINGULAR_ROW_SRC
versus.
NESTED_LOOP_JOIN
SINGULAR_ROW_SRC
UNNEST
That difference caused a plan change here because during join ordering we had a different version than before.
--
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 <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3063: Separate join inversion from join ordering.
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-3063: Separate join inversion from join ordering.
......................................................................
Patch Set 2:
(4 comments)
will look at logic changes in a bit...
http://gerrit.cloudera.org:8080/#/c/3846/2//COMMIT_MSG
Commit Message:
Line 7: IMPALA-3063: Separate join inversion from join ordering.
the changes in test output make it look like some plans also change (so this isn't purely a separation of functionality?).
http://gerrit.cloudera.org:8080/#/c/3846/2/testdata/workloads/functional-planner/queries/PlannerTest/outer-joins.test
File testdata/workloads/functional-planner/queries/PlannerTest/outer-joins.test:
Line 814: | |--07:HASH JOIN [RIGHT OUTER JOIN]
why is this cross product flipped?
http://gerrit.cloudera.org:8080/#/c/3846/2/testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
File testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test:
Line 1088: | hash predicates: l_suppkey = s_suppkey, l_partkey = ps_partkey
we've seen meaningless changes like this before. i'm wondering whether we should simply sort predicate lists
Line 1777: | hash predicates: p_partkey = ps_partkey
is this the right thing to do, given selectivities?
--
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 <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3063: Separate join inversion from join ordering.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-3063: Separate join inversion from join ordering.
......................................................................
Patch Set 2:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/3846/2//COMMIT_MSG
Commit Message:
Line 7: IMPALA-3063: Separate join inversion from join ordering.
> a) please add this explanation to the commit msg
a) Done.
b) Filed IMPALA-3985
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
thanks!
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?
These conditions seem trivially satisfied now that an ExchangeNode must be constructed with a single child, i.e. after calling the c'tor the should be satisfied.
Note that these checks only used to be run when adding the second child - a case which is not supported anymore.
Line 95
> what about this?
Restored it, though it's not possible to construct an exch node without a child anymore.
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()?
Done
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
Agreed. Reformulated.
Line 355: private void invertJoins(PlanNode root, boolean isSingleNodeExec) {
> explain 2nd param
Done and also renamed for clarity (single node exec was not quite accurate)
Line 360: for (PlanNode child: root.getChildren()) {
> single line
Done
Line 372: // we cannot execute it efficiently.
> the same is true for right-anything if it's a broadcast. do we know we're n
1. Your comment about right-anything is correct and we will make the appropriate distribution decision when generating the distributed plan (i.e. taking the join op constraints into account)
2. In order to execute a left null aware anti join we need to know whether the build side contained any row with a NULL in one of the slots that participates in the hash lookup. So, a right version of that joins is not practical, and a partitioning strategy does not really work either.
Line 373: if (joinNode.isStraightJoin() || joinOp.isNullAwareLeftAntiJoin()) {
> what does it mean to deal with 'straight join' here and not for the entire
We're dealing with a plan tree here, so we don't know which query block this JoinNode originated from. Therefore, we preserve the straight_join info when creating a JoinNode and check for it here. Not sure if that answered your question?
Line 380: // Always place a singular row src on the build side because it has
> has produces
Done
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
Expanded comment. Renamed param to isLocalPlan.
PS2, Line 388: build-side hash table
> "the materialized rhs", no? (this still applies to nlj?)
Correct. Done.
Line 398: !(joinOp.isInnerJoin() && !joinNode.getEqJoinConjuncts().isEmpty())) {
> why can't this be applied to inner equi joins?
It can be applied and that's the intended follow-on change. My thinking is that it would be easier to review the changes in isolation so we don't have to reason about all the plan changes at once.
Line 404: // Re-compute tuple ids since their order must correspond to the order of children.
> why?
Expanded comment. I think removing this BE assumption may be involved.
Line 414: private PlanNode useNljForSmallBuilds(PlanNode root, Analyzer analyzer)
> the name doesn't really reflect the (much narrower) semantics, find a bette
Done
Line 423: if (joinNode.getJoinOp().isNullAwareLeftAntiJoin()) return root;
> also check that this is really a hash join
Done
--
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 <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3063: Separate join inversion from join ordering.
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has submitted this change and it was merged.
Change subject: IMPALA-3063: Separate join inversion from join ordering.
......................................................................
IMPALA-3063: Separate join inversion from join ordering.
Before this change joins were inverted while doing join ordering.
That approach was unnecessarily complex because it required
modifying the global analysis state for correct conjunct
placement, etc. However, join inversion is independent of join
ordering, and the existing approach could lead to generating
invalid plans with distributed non-equi right outer/semi joins,
which we cannot execute in the backend.
After this change joins are inverted in a separate pass over
the single-node plan. This simplifies the inversion
logic and allows us to avoid generating those invalid plans.
Note that this change is not only a separation of functionality
for the following reasons:
1. Our join cardinality estimation is not symmetric, i.e., A JOIN B
may not give the same estimate as B JOIN A due to our FK/PK detection
heuristic. In the context of this patch this means that an inverted
join may have a different cardinality estimate, so plans may change
depending on whether the inversion is done during join ordering of after.
2. We currently only invert outer/semi/anti joins based on the rhs table
ref join op. In this patch I want to preserve the existing behavior as
much as possible, but when doing the join ordering in a separate pass we
may see a join opn in a JoinNode that is different from the rhs table ref.
So in some situations the inversion behavior based on the join op could be
different and there are some examples in this patch.
This patch also moves the logic of converting hash joins to
nested-loop joins into a separate pass over the single-node plan.
Change-Id: If86db7753fc585bb4c69612745ec0103278888a4
Reviewed-on: http://gerrit.cloudera.org:8080/3846
Reviewed-by: Alex Behm <al...@cloudera.com>
Tested-by: Internal Jenkins
---
M be/src/exec/blocking-join-node.h
M fe/src/main/java/com/cloudera/impala/analysis/Analyzer.java
M fe/src/main/java/com/cloudera/impala/analysis/BinaryPredicate.java
M fe/src/main/java/com/cloudera/impala/analysis/SelectList.java
M fe/src/main/java/com/cloudera/impala/analysis/TableRef.java
M fe/src/main/java/com/cloudera/impala/planner/AggregationNode.java
M fe/src/main/java/com/cloudera/impala/planner/AnalyticEvalNode.java
M fe/src/main/java/com/cloudera/impala/planner/DistributedPlanner.java
M fe/src/main/java/com/cloudera/impala/planner/ExchangeNode.java
M fe/src/main/java/com/cloudera/impala/planner/HashJoinNode.java
M fe/src/main/java/com/cloudera/impala/planner/HdfsScanNode.java
M fe/src/main/java/com/cloudera/impala/planner/JoinNode.java
M fe/src/main/java/com/cloudera/impala/planner/NestedLoopJoinNode.java
M fe/src/main/java/com/cloudera/impala/planner/PlanNode.java
M fe/src/main/java/com/cloudera/impala/planner/Planner.java
M fe/src/main/java/com/cloudera/impala/planner/SelectNode.java
M fe/src/main/java/com/cloudera/impala/planner/SingleNodePlanner.java
M fe/src/main/java/com/cloudera/impala/planner/SingularRowSrcNode.java
M fe/src/main/java/com/cloudera/impala/planner/SortNode.java
M fe/src/main/java/com/cloudera/impala/planner/SubplanNode.java
M fe/src/main/java/com/cloudera/impala/planner/UnionNode.java
M fe/src/main/java/com/cloudera/impala/planner/UnnestNode.java
M testdata/workloads/functional-planner/queries/PlannerTest/implicit-joins.test
M testdata/workloads/functional-planner/queries/PlannerTest/join-order.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M testdata/workloads/functional-planner/queries/PlannerTest/nested-collections.test
M testdata/workloads/functional-planner/queries/PlannerTest/nested-loop-join.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
29 files changed, 661 insertions(+), 363 deletions(-)
Approvals:
Internal Jenkins: Verified
Alex Behm: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/3846
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: If86db7753fc585bb4c69612745ec0103278888a4
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
[Impala-ASF-CR] IMPALA-3063: Separate join inversion from join ordering.
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-3063: Separate join inversion from join ordering.
......................................................................
Patch Set 5: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3846/4/fe/src/main/java/com/cloudera/impala/planner/Planner.java
File fe/src/main/java/com/cloudera/impala/planner/Planner.java:
Line 405: // Re-compute tuple ids because the backend assumes that their order corresponds to
> it's DCHECKed in blocking-join-node.cc L102 and following
agreed, let's not fix this as part of this change.
however, this assumption doesn't really make sense, and should be removed. leave at least one todo in a conspicuous place.
computeTupleIds() seems like a nice abstraction on its own, but it was introduced in order to recompute tuple ids because of this hard-to-comprehend assumption in blocking join node. if you feel that's the only reason why you want computeTupleIds(), leave a todo to remove that as well.
--
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes