[Impala-ASF-CR] IMPALA-4080: [Part 1] Move static state from ExecNode into a PlanNode

["Daniel Becker (Code Review)" <ge...@cloudera.org>]

Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/14764 )

Change subject: IMPALA-4080: [Part 1] Move static state from ExecNode into a PlanNode
......................................................................


Patch Set 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/14764/3/be/src/exec/aggregation-node-base.h
File be/src/exec/aggregation-node-base.h:

http://gerrit.cloudera.org:8080/#/c/14764/3/be/src/exec/aggregation-node-base.h@28
PS3, Line 28: /// Base class containing common code for the ExecNodes that do aggregation,
            : /// AggregationNode and StreamingAggregationNode.
Shouldn't this comment be moved to AggregationNodeBase? Or reworded to fit for both AggregationPlanNode and AggregationNodeBase (now it mentions ExecNode).
This also applies at other places.


http://gerrit.cloudera.org:8080/#/c/14764/3/be/src/exec/aggregation-node.cc
File be/src/exec/aggregation-node.cc:

http://gerrit.cloudera.org:8080/#/c/14764/3/be/src/exec/aggregation-node.cc@39
PS3, Line 39:   for (int i = 0; i < pnode.tnode_->agg_node.aggregators.size(); ++i) {
Maybe a for-each loop would be more readable.


http://gerrit.cloudera.org:8080/#/c/14764/3/be/src/exec/aggregator.h
File be/src/exec/aggregator.h:

http://gerrit.cloudera.org:8080/#/c/14764/3/be/src/exec/aggregator.h@81
PS3, Line 81:   /// Certain aggregates require a finalize step, which is the final step of the
Maybe we could give an example of what kind of aggregator needs a finalize step? Is AVG() a good example?



-- 
To view, visit http://gerrit.cloudera.org:8080/14764
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I69f1676bf67bac31fa5902511b3fcc269fd67472
Gerrit-Change-Number: 14764
Gerrit-PatchSet: 5
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Dec 2019 16:09:25 +0000
Gerrit-HasComments: Yes