Michael Ho 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 commen Done 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 al Done Line 156: current_probe_row_, status))) { > DCHECK(!status.ok()); ? Done Line 197: outter > outer Done Line 244: if (UNLIKELY(!AppendRow(null_probe_rows_, current_probe_row_, status))) { > DCHECK(!status.ok()); Done Line 270: if (UNLIKELY(!AppendRow(partition->probe_rows(), current_probe_row_, status))) { > DCHECK(!status.ok()); Done Line 333: DCHECK(hash_tbl_iterator_.AtEnd()); > DCHECK_GE(num_rows_added, 0); ? i.e. it can't be -1 on this path, right? Added DCHECK(status->ok()); Line 341: DCHECK_LE(num_rows_added, max_rows); > DCHECK_EQ(num_rows_added == -1, !status.ok()); The new patch checks if the status is ok and if not, set num_rows_added to -1 instead. 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 Done Line 181: remaining > this is confusing because it's only the remaining capacity on the first ite Done. Alternately, we can do 'out_batch_iterator->parent()->AtCapacity()' as capacity check but this is likely to be slower as (1) the generated IR is likely to load the fields from memory and (2) the original logic in this path doesn't check for memory usage (probably for a good reason). Added some comments about it. Line 189: '. > what about the RIGHT_ANTI_JOIN case? could word it similar to ProcessProbe Done Line 230: /// next probe row and its corresponding partition. > here and others: document num_rows_added == -1 case and relationship to sta Not relevant for this function anymore as the new patch stops relying on setting this variable to -1 to propagate error. -- 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
