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 1: (11 comments) http://gerrit.cloudera.org:8080/#/c/8196/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8196/1//COMMIT_MSG@8 PS1, Line 8: > Can you add a testing section? It would be good to mention what test covera is there anyway we can make sure through a test that codegen was successful for this node? http://gerrit.cloudera.org:8080/#/c/8196/1//COMMIT_MSG@9 PS1, Line 9: Preformance > Performance Done http://gerrit.cloudera.org:8080/#/c/8196/1//COMMIT_MSG@10 PS1, Line 10: perdicates > predicates Done http://gerrit.cloudera.org:8080/#/c/8196/1//COMMIT_MSG@11 PS1, Line 11: [column_name > value AND] > Can you include a concrete example of the query to make it easier for other Done http://gerrit.cloudera.org:8080/#/c/8196/1/be/src/exec/select-node-ir.cc File be/src/exec/select-node-ir.cc: http://gerrit.cloudera.org:8080/#/c/8196/1/be/src/exec/select-node-ir.cc@29 PS1, Line 29: child_row_batch_->num_rows() > This probably doesn't matter, but you could save a few instructions by hois Done http://gerrit.cloudera.org:8080/#/c/8196/1/be/src/exec/select-node-ir.cc@41 PS1, Line 41: COUNTER_SET(rows_returned_counter_, num_rows_returned_); > COUNTER_SET can be pretty expensive because of the atomic operation. We can Done http://gerrit.cloudera.org:8080/#/c/8196/1/be/src/exec/select-node.cc File be/src/exec/select-node.cc: http://gerrit.cloudera.org:8080/#/c/8196/1/be/src/exec/select-node.cc@55 PS1, Line 55: NULL > Use nullptr in new code (here and below). Done http://gerrit.cloudera.org:8080/#/c/8196/1/be/src/exec/select-node.cc@65 PS1, Line 65: if (codegen_status.ok()) { > Instead of this branch the control flow may be easier to follow if you fact Done http://gerrit.cloudera.org:8080/#/c/8196/1/be/src/exec/select-node.cc@71 PS1, Line 71: DCHECK(copy_rows_fn != NULL); > We should probably check if this is NULL and fail codegen if so. I'm not su Done http://gerrit.cloudera.org:8080/#/c/8196/1/be/src/exec/select-node.cc@99 PS1, Line 99: bool copy_rows_success = false; > Just declare it inside the loop where it's needed? When I see it declared u Done http://gerrit.cloudera.org:8080/#/c/8196/1/be/src/exec/select-node.cc@114 PS1, Line 114: copy_rows_success = false; > It's assigned on both branches so this isn't necessary. 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: 1 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, 03 Oct 2017 20:52:29 +0000 Gerrit-HasComments: Yes
