Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10394 )
Change subject: PREVIEW: IMPALA-110 (part 1): Refactor PartitionedAggregationNode ...................................................................... Patch Set 1: (7 comments) I did a quick pass, didn't look closely for correctness issues. This seems like a vast improvement. I had some minor comments. It seems like we could defer some of the cleanup to a follow-on patch if that makes this initial refactoring more mechanical (it gets harder to review if there's a mix of large-scale code motion and non-mechanical cleanup in the same file). 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); Add override specifiers? 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: if (UNLIKELY(VLOG_ROW_IS_ON)) { I'd be ok with deleting this code :) http://gerrit.cloudera.org:8080/#/c/10394/1/be/src/exec/aggregation-node.cc@111 PS1, Line 111: RETURN_IF_ERROR(aggregator_->MoveHashPartitions(child(0)->rows_returned())); It feels like this is the wrong abstraction - should it just be InputDone() like the Sorter? 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@141 PS1, Line 141: int64_t num_rows_returned_; : Runti > why are both of these needed? Yeah it seems like rows_returned_counter_ is probably only needed in ExecNode? Or is this looking forward to having a separate sub-profile for the multiple aggregators in a multi-agg node? 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: if (tnode.agg_node.use_streaming_preaggregation) { Maybe these should be different TPlanNodeTypes now? 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: virtual Status Init(const TPlanNode& tnode, RuntimeState* state); Add overrides? 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 Status Open(RuntimeState* state); override specifiers? -- 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: Tim Armstrong <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Wed, 23 May 2018 22:30:18 +0000 Gerrit-HasComments: Yes
