Alex Behm has posted comments on this change. Change subject: IMPALA-5612: join inversion should factor in parallelism ......................................................................
Patch Set 5: (7 comments) Nice! http://gerrit.cloudera.org:8080/#/c/7351/5/fe/src/main/java/org/apache/impala/planner/Planner.java File fe/src/main/java/org/apache/impala/planner/Planner.java: Line 451: * inversion. Returns false if any join input is missing relevant stats. Line 463: * - the join strategy is PARTITIONED and rows are distributed evenly. I suggest you add a quick note why BROADCAST is not particularly relevant. Otherwise, this may seem like a big unexplained assumption. Line 473: * The estimated cost of a hash join before and after inversion, measured in an estimated per-host cost Line 482: * We choose b = 10 and C = 5 empirically because it seems to give reasonable You had this great spreadsheet over a wide range of inputs. Do you mind dumping that table to the upstream JIRA so we can see how this model behaves? (Alternatively, just add the link to the Google Spreadsheet.) Line 491: private boolean isInvertedJoinCheaper(boolean isLocalPlan, JoinNode joinNode) { nit: flip args to make them consistent with invertJoins() Line 509: final long CONSTANT_COST_PER_ROW = 5; CONSTANT_COST_PER_BYTE? Line 518: LOG.trace("lhsCard " + lhsCard + " lhsBytes " + lhsBytes + Also log the tblRefIds_ and/or tupleIds_ or the join, otherwise it will be impossible to know which join this is. -- To view, visit http://gerrit.cloudera.org:8080/7351 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icacea4565ce25ef15aaab014684c9440dd501d4e Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Mostafa Mokhtar <mmokh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
