Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/8196 )
Change subject: IMPALA-4236: Codegen CopyRows() for select nodes ...................................................................... Patch Set 6: (6 comments) http://gerrit.cloudera.org:8080/#/c/8196/6/be/src/exec/select-node-ir.cc File be/src/exec/select-node-ir.cc: http://gerrit.cloudera.org:8080/#/c/8196/6/be/src/exec/select-node-ir.cc@29 PS6, Line 29: while (child_row_idx_ < child_batch_num_rows) > Have you looked into using FOREACH_ROW_LIMIT() ? May have some complication Done http://gerrit.cloudera.org:8080/#/c/8196/6/be/src/exec/select-node.h File be/src/exec/select-node.h: http://gerrit.cloudera.org:8080/#/c/8196/6/be/src/exec/select-node.h@40 PS6, Line 40: virtual void Codegen(RuntimeState* state); > While you are at it, would you mind adding override to these functions ? Done http://gerrit.cloudera.org:8080/#/c/8196/6/be/src/exec/select-node.h@63 PS6, Line 63: codegend_copy_rows_fn_; > Feel free to ignore but I think we have recently switched to initializing f I should do that change for all the member variables. Do you recommend I make that change in this commit? http://gerrit.cloudera.org:8080/#/c/8196/6/be/src/exec/select-node.h@69 PS6, Line 69: /// Codegen CopyRows(). > May help to explain that this is mostly codegen'ing the conjuncts evaluatio Done http://gerrit.cloudera.org:8080/#/c/8196/6/be/src/exec/select-node.cc File be/src/exec/select-node.cc: http://gerrit.cloudera.org:8080/#/c/8196/6/be/src/exec/select-node.cc@91 PS6, Line 91: if (ReachedLimit() || (child_row_idx_ == child_row_batch_->num_rows() && child_eos_)) { > This is still a bit wonky (not your change but we should try to leave this Done http://gerrit.cloudera.org:8080/#/c/8196/6/be/src/exec/select-node.cc@107 PS6, Line 107: child_row_batch_->TransferResourceOwnership(row_batch); > Sorry for the additional churn but I think the resource transfer is overly Done -- To view, visit http://gerrit.cloudera.org:8080/8196 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie0d496d004418468e16b6f564f90f45ebbf87c1e Gerrit-Change-Number: 8196 Gerrit-PatchSet: 6 Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 10 Oct 2017 21:46:37 +0000 Gerrit-HasComments: Yes
