Alex Behm has posted comments on this change. Change subject: IMPALA-3084: Cache the sequence of table ref and materialized tuple ids during analysis. ......................................................................
Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/2367/3/fe/src/main/java/com/cloudera/impala/analysis/BinaryPredicate.java File fe/src/main/java/com/cloudera/impala/analysis/BinaryPredicate.java: Line 319: public void swapChildren() { > what's that for? Sorry. From testing. Removed. http://gerrit.cloudera.org:8080/#/c/2367/3/fe/src/main/java/com/cloudera/impala/planner/SingleNodePlanner.java File fe/src/main/java/com/cloudera/impala/planner/SingleNodePlanner.java: Line 1392: if (analyzer.hasValueTransfer(lhsSid, rhsSid) && > we (mostly) use Done Line 1393: analyzer.hasValueTransfer(rhsSid, lhsSid)) { > isn't the lhs->rhs value transfer sufficient? Here is one example where that is not sufficient/correct: select a.id aid, b.id bid, a.int_col aint, b.int_col bint from functional.alltypes a inner join functional.alltypes b on a.int_col < b.int_col left outer join functional.alltypes c on a.id = b.id and a.id = c.id We cannot make the a/b join a HJ because that would reject non-matches from 'c' which would have otherwise been nulled. I added a clarifying comment as discussed. Line 1396: // equalities are redundant > this comments seems to contradict the fact that you're doing this for all l Good catch. We should break as soon as we added one predicate (all others are indeed redundant). We do it for all slotIds because we need to find one with a mutual value transfer. Elements in the same equivalence class only require a value transfer in one direction (i.e. the eq class is a superset of those slots with mutual transfers) http://gerrit.cloudera.org:8080/#/c/2367/3/testdata/workloads/functional-planner/queries/PlannerTest/join-order.test File testdata/workloads/functional-planner/queries/PlannerTest/join-order.test: Line 799: 09:HASH JOIN [INNER JOIN] > where does the improvement come from? it doesn't look like the predicates c Notice that we have an inverted join right below these inner joins. The inversion, unfortunately, used to limit the plans we explored because we relied on the TableRef chain for checking join applicability. Now that we cache the table ref and materialized tuple ids we can explore more plans after an inverted join. -- To view, visit http://gerrit.cloudera.org:8080/2367 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I298b8695c9f26644a395ca9f0e86040e3f5f3846 Gerrit-PatchSet: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.5.0_5.7.0 Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-HasComments: Yes
