anujphadke has posted comments on this change. Change subject: IMPALA-4866: Hash join node does not apply limits correctly ......................................................................
Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/6778/2/be/src/exec/partitioned-hash-join-node.cc File be/src/exec/partitioned-hash-join-node.cc: Line 582: // Try to continue from the current probe side input. > Maybe we should set the counter at the bottom of GetNext(), instead of in t Done Line 642: // Truncate the row batch if we went over the limit. > There's a bug if the hash join node is in a subplan - SubplanNode may call Done Line 643: num_rows_added = limit_ - num_rows_returned_; > Shouldn't we decrement 'num_rows_returned_' if we truncated the batch? Othe Done Line 806: if (matched_null_probe_[null_probe_output_idx_]) continue; > Nit: we usually write this as: With the current change, I dont think num_rows_returned should be updated elsewhere except at the end of GetNext before setting the counter. line#651 If it gets incremented and exceeds the limit, num_rows_returned could end up being a -ve value. line#644 num_rows_added = limit_ - num_rows_returned_; DCHECK_GE(num_rows_added, 0); Line 944: TupleRow* out_row = out_batch->GetRow(out_batch->AddRow()); > Nit: we usually write this as: With the current change, I dont think num_rows_returned should be updated elsewhere except at the end of GetNext before setting the counter. line#651 If it gets incremented and exceeds the limit, num_rows_returned could end up being a -ve value. line#644 num_rows_added = limit_ - num_rows_returned_; DCHECK_GE(num_rows_added, 0); http://gerrit.cloudera.org:8080/#/c/6778/2/testdata/workloads/functional-query/queries/QueryTest/single-node-joins-with-limits.test File testdata/workloads/functional-query/queries/QueryTest/single-node-joins-with-limits.test: > How long do these take to run? Should they be under exhaustive? Moved them under exhaustive tests. -- To view, visit http://gerrit.cloudera.org:8080/6778 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I414124f8bb6f8b2af2df468e1c23418d05a0e29f Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: anujphadke <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: anujphadke <[email protected]> Gerrit-HasComments: Yes
