Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8196 )
Change subject: IMPALA-4236: Codegen CopyRows() for select nodes ...................................................................... Patch Set 6: (5 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 with bumping child_row_idx_ at the end but worth looking into it. 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 ? 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 fields directly in the class declaration here: - codegend_copy_rows_fn_ = nullptr; 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 evaluation logic so as to provide more context. http://gerrit.cloudera.org:8080/#/c/8196/6/be/src/exec/select-node.h@70 PS6, Line 70: Status CodegenCopyRows(RuntimeState* state); WARN_UNUSED_RESULT -- 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 00:36:21 +0000 Gerrit-HasComments: Yes
