Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10394 )
Change subject: IMPALA-110 (part 2): Refactor PartitionedAggregationNode ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/10394/3/be/src/exec/aggregator.h File be/src/exec/aggregator.h: http://gerrit.cloudera.org:8080/#/c/10394/3/be/src/exec/aggregator.h@54 PS3, Line 54: AggregationNode Used for other exec nodes, no? (e.g. streaming) http://gerrit.cloudera.org:8080/#/c/10394/3/be/src/exec/aggregator.h@63 PS3, Line 63: virtual Status Init(const TPlanNode& tnode, RuntimeState* state) WARN_UNUSED_RESULT; : virtual Status Prepare(RuntimeState* state) WARN_UNUSED_RESULT; : virtual void Codegen(RuntimeState* state) = 0; : virtual Status Open(RuntimeState* state) WARN_UNUSED_RESULT; : virtual Status GetNext( : RuntimeState* state, RowBatch* row_batch, bool* eos) WARN_UNUSED_RESULT = 0; : virtual Status Reset(RuntimeState* state) WARN_UNUSED_RESULT = 0; : virtual void Close(RuntimeState* state); I guess these are used one-to-one to implement the ExecNode interface? Or are there any subtle differences in lifecycle? let's briefly comment either way. http://gerrit.cloudera.org:8080/#/c/10394/3/be/src/exec/aggregator.h@80 PS3, Line 80: SetDebugOptions why does the aggregator need to copy the debug_options? where does it get used at the aggregator level? http://gerrit.cloudera.org:8080/#/c/10394/3/be/src/exec/streaming-aggregation-node.h File be/src/exec/streaming-aggregation-node.h: http://gerrit.cloudera.org:8080/#/c/10394/3/be/src/exec/streaming-aggregation-node.h@31 PS3, Line 31: Node for doing partitioned hash aggregation. : /// This node consumes the input from child(0) during GetNext() and then passes it to the : /// Aggregator, which does the actual work of aggregating. : /// This node only supports grouping aggregations. somewhere in here we should explain how this differs from AggregationNode, i.e. what does streaming mean. -- 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: 3 Gerrit-Owner: Thomas Marshall <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Thu, 21 Jun 2018 00:21:21 +0000 Gerrit-HasComments: Yes
