Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/10394 )
Change subject: IMPALA-110 (part 2): Refactor PartitionedAggregationNode ...................................................................... Patch Set 2: (14 comments) While developing this patch, I maintained a branch with a series of commits separating the work out into several mostly mechanical steps to keep everything straight. It may be easier to deal with this review by looking at that branch, which has all of the actually significant code changes in the top commit (to be clear, even these changes are designed not to change any of the logic of the aggregation algorithm). The branch is available here: https://github.com/twmarshall/impala/commits/agg-refactor I can also submit it as a review, if people prefer to look at it in the gerrit interface. 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: object > nit: object Done http://gerrit.cloudera.org:8080/#/c/10394/1//COMMIT_MSG@35 PS1, Line 35: GroupingAggregato > nit: GroupingAggregators Done http://gerrit.cloudera.org:8080/#/c/10394/1/be/src/exec/aggregation-node.h File be/src/exec/aggregation-node.h: http://gerrit.cloudera.org:8080/#/c/10394/1/be/src/exec/aggregation-node.h@38 PS1, Line 38: virtual Status Init(const TPlanNode& tnode, RuntimeState* state) override; > Add override specifiers? Done http://gerrit.cloudera.org:8080/#/c/10394/1/be/src/exec/aggregation-node.cc File be/src/exec/aggregation-node.cc: http://gerrit.cloudera.org:8080/#/c/10394/1/be/src/exec/aggregation-node.cc@89 PS1, Line 89: // exception is if we are inside a subplan expecting to call Open()/GetNext() on the > I'd be ok with deleting this code :) Done http://gerrit.cloudera.org:8080/#/c/10394/1/be/src/exec/aggregation-node.cc@111 PS1, Line 111: return Status::OK(); > It feels like this is the wrong abstraction - should it just be InputDone() Done 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? Done http://gerrit.cloudera.org:8080/#/c/10394/1/be/src/exec/aggregator.h@141 PS1, Line 141: /// Conjuncts and their evaluators in this aggregator. 'conjuncts_' live in the : /// q > Yeah it seems like rows_returned_counter_ is probably only needed in ExecNo Right, once we go to multiple aggregators, each one will have its own entry in the runtime profile. 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 Done http://gerrit.cloudera.org:8080/#/c/10394/1/be/src/exec/exec-node.cc File be/src/exec/exec-node.cc: http://gerrit.cloudera.org:8080/#/c/10394/1/be/src/exec/exec-node.cc@362 PS1, Line 362: break; > Maybe these should be different TPlanNodeTypes now? I think it probably makes the most sense to leave it this way - a TStreamingAggNode and TAggNode would have exactly the same fields, so it seems cleaner to just differentiate with a flag. http://gerrit.cloudera.org:8080/#/c/10394/1/be/src/exec/grouping-aggregator.h File be/src/exec/grouping-aggregator.h: http://gerrit.cloudera.org:8080/#/c/10394/1/be/src/exec/grouping-aggregator.h@117 PS1, Line 117: const DescriptorTbl& descs); > Add overrides? Done 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@48 PS1, Line 48: virtual void Codegen(RuntimeState* state) override; > override specifiers? Done 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. Done http://gerrit.cloudera.org:8080/#/c/10394/1/be/src/exec/non-grouping-aggregator.cc@43 PS1, Line 43: true > I feel like we should defer changing some of the more subtle details of the Yeah, the intention was to make this patch as mechanical as possible, since its a very large change and will be easier to review that way 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: ( > change to nullptr while here Done -- 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: 2 Gerrit-Owner: Thomas Marshall <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Fri, 15 Jun 2018 23:04:19 +0000 Gerrit-HasComments: Yes
