Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10394 )
Change subject: PREVIEW: IMPALA-110 (part 1): Refactor PartitionedAggregationNode ...................................................................... Patch Set 1: (10 comments) looks good... just some nits from my end and a clarifying comment for non-grouping-aggregator (that might have been present before) http://gerrit.cloudera.org:8080/#/c/10394/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10394/1//COMMIT_MSG@13 PS1, Line 13: objects nit: object http://gerrit.cloudera.org:8080/#/c/10394/1//COMMIT_MSG@35 PS1, Line 35: GroupingAggretors nit: GroupingAggregators http://gerrit.cloudera.org:8080/#/c/10394/1/be/src/exec/aggregator.h File be/src/exec/aggregator.h: http://gerrit.cloudera.org:8080/#/c/10394/1/be/src/exec/aggregator.h@109 PS1, Line 109: // nit: /// for consistency? http://gerrit.cloudera.org:8080/#/c/10394/1/be/src/exec/aggregator.h@141 PS1, Line 141: int64_t num_rows_returned_; : Runti why are both of these needed? http://gerrit.cloudera.org:8080/#/c/10394/1/be/src/exec/aggregator.cc File be/src/exec/aggregator.cc: http://gerrit.cloudera.org:8080/#/c/10394/1/be/src/exec/aggregator.cc@58 PS1, Line 58: NULL change to nullptr while here http://gerrit.cloudera.org:8080/#/c/10394/1/be/src/exec/non-grouping-aggregator.h File be/src/exec/non-grouping-aggregator.h: http://gerrit.cloudera.org:8080/#/c/10394/1/be/src/exec/non-grouping-aggregator.h@83 PS1, Line 83: singleton_output_tuple_returned_ I wasn't entirely clear about how this is used (see the comment in the cc file). My confusion was around what seems to be some blurring of the init/done state. http://gerrit.cloudera.org:8080/#/c/10394/1/be/src/exec/non-grouping-aggregator.cc File be/src/exec/non-grouping-aggregator.cc: http://gerrit.cloudera.org:8080/#/c/10394/1/be/src/exec/non-grouping-aggregator.cc@41 PS1, Line 41: NULL nit: switch to nullptr while here? several more below. http://gerrit.cloudera.org:8080/#/c/10394/1/be/src/exec/non-grouping-aggregator.cc@43 PS1, Line 43: true why initialized to true? would an enum, something like {kInit, kReady, kDone } make it clearer? http://gerrit.cloudera.org:8080/#/c/10394/1/be/src/exec/non-grouping-aggregator.cc@119 PS1, Line 119: Close is the intent for this method be idempotent? any issue if the tuple is fetched multiple times due to calling close multiple times? http://gerrit.cloudera.org:8080/#/c/10394/1/be/src/exec/streaming-aggregation-node.cc File be/src/exec/streaming-aggregation-node.cc: http://gerrit.cloudera.org:8080/#/c/10394/1/be/src/exec/streaming-aggregation-node.cc@109 PS1, Line 109: NULL change to nullptr while here -- To view, visit http://gerrit.cloudera.org:8080/10394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9e7bb583f54aa4add3738bde7f57cf3511ac567e Gerrit-Change-Number: 10394 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Mon, 21 May 2018 20:59:00 +0000 Gerrit-HasComments: Yes
