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