Dan Hecht has posted comments on this change. Change subject: IMPALA-1583: Simplify PartitionedHashJoinNode::ProcessProbeBatch() ......................................................................
Patch Set 5: (12 comments) http://gerrit.cloudera.org:8080/#/c/2893/5/be/src/exec/partitioned-hash-join-node-ir.cc File be/src/exec/partitioned-hash-join-node-ir.cc: Line 132: // At this point the probe is considered matched. confusing because in ProcessProbeRowRightSemiJoins() we had the same comment but it was the hash table node that has the match flag. clarify these two comments. Line 134: // We can safely ignore this probe row for left anti joins. this comment sounds like we should only do it for left anti join, but we always do SetAtEnd(). clarify/fix. Line 156: current_probe_row_, status))) { DCHECK(!status.ok()); ? Line 197: outter outer Line 244: if (UNLIKELY(!AppendRow(null_probe_rows_, current_probe_row_, status))) { DCHECK(!status.ok()); Line 270: if (UNLIKELY(!AppendRow(partition->probe_rows(), current_probe_row_, status))) { DCHECK(!status.ok()); Line 333: DCHECK(hash_tbl_iterator_.AtEnd()); DCHECK_GE(num_rows_added, 0); ? i.e. it can't be -1 on this path, right? Line 341: DCHECK_LE(num_rows_added, max_rows); DCHECK_EQ(num_rows_added == -1, !status.ok()); http://gerrit.cloudera.org:8080/#/c/2893/5/be/src/exec/partitioned-hash-join-node.h File be/src/exec/partitioned-hash-join-node.h: Line 177: out_batch here and elsewhere - update comment for out_batch_iterator Line 181: remaining this is confusing because it's only the remaining capacity on the first iteration. rather than passing both num_rows_added and max_rows, could we just pass a *remaining_capacity which is decremented for each row (and then you have to stop when it hits 0)? If that doesn't work (maybe because of the -1 case for num_rows_added, let's document that 'max_rows' is the stopping point for num_rows_added. Line 189: '. what about the RIGHT_ANTI_JOIN case? could word it similar to ProcessProbeRowLeftSemiJoins() comment instead. Line 230: /// next probe row and its corresponding partition. here and others: document num_rows_added == -1 case and relationship to status. -- To view, visit http://gerrit.cloudera.org:8080/2893 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie2091bdf97ab34c5cdc84e84394c579a5b36afc0 Gerrit-PatchSet: 5 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
