Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10771 )
Change subject: IMPALA-110: Support for multiple DISTINCT ...................................................................... Patch Set 9: (7 comments) Did an initial pass over the backend. Found couple of nits, feel free to ignore the ones you don't agree with. Will continue to look at the FE as well. http://gerrit.cloudera.org:8080/#/c/10771/9/be/src/exec/aggregation-node-base.cc File be/src/exec/aggregation-node-base.cc: http://gerrit.cloudera.org:8080/#/c/10771/9/be/src/exec/aggregation-node-base.cc@77 PS9, Line 77: curr_output_agg_idx_ = 0; I think Reset() should be called on the aggregators in aggs_. http://gerrit.cloudera.org:8080/#/c/10771/9/be/src/exec/aggregation-node.cc File be/src/exec/aggregation-node.cc: http://gerrit.cloudera.org:8080/#/c/10771/9/be/src/exec/aggregation-node.cc@53 PS9, Line 53: for (int i = 0; i < aggs_.size(); ++i) { : mini_batches.push_back( : make_unique<RowBatch>(child(0)->row_desc(), state->batch_size(), mem_tracker())); : } nit: this could be nested in an if (!replicate_input_) http://gerrit.cloudera.org:8080/#/c/10771/9/be/src/exec/aggregation-node.cc@77 PS9, Line 77: DCHECK_EQ(aggs_.size(), child(0)->row_desc()->tuple_descriptors().size()); nit: DCHECK_EQ could be placed after the definition of num_tupes, so it could be just DCHECK_EQ(aggs_.size(), num_tuples); http://gerrit.cloudera.org:8080/#/c/10771/9/be/src/exec/grouping-aggregator.h File be/src/exec/grouping-aggregator.h: http://gerrit.cloudera.org:8080/#/c/10771/9/be/src/exec/grouping-aggregator.h@296 PS9, Line 296: AddBatchStreaing nit: typo http://gerrit.cloudera.org:8080/#/c/10771/9/be/src/exec/streaming-aggregation-node.cc File be/src/exec/streaming-aggregation-node.cc: http://gerrit.cloudera.org:8080/#/c/10771/9/be/src/exec/streaming-aggregation-node.cc@97 PS9, Line 97: for (int i = 0; i < aggs_.size(); ++i) { : mini_batches.push_back( : make_unique<RowBatch>(child(0)->row_desc(), state->batch_size(), mem_tracker())); : } nit: can be nested in if (!replicate_input_) http://gerrit.cloudera.org:8080/#/c/10771/9/be/src/exec/streaming-aggregation-node.cc@134 PS9, Line 134: && eos nit: it doesn't seem necessary to check eos since it will be always true if replicate_agg_idx_ == aggs_.size() http://gerrit.cloudera.org:8080/#/c/10771/9/be/src/exec/streaming-aggregation-node.cc@143 PS9, Line 143: DCHECK_EQ nit: it could be DCHECK(aggs_.size(), num_tuples) if placed few lines later -- To view, visit http://gerrit.cloudera.org:8080/10771 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I055402eaef6d81e5f70e850d9f8a621e766830a4 Gerrit-Change-Number: 10771 Gerrit-PatchSet: 9 Gerrit-Owner: Thomas Marshall <thomasmarsh...@cmu.edu> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Thomas Marshall <thomasmarsh...@cmu.edu> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Thu, 30 Aug 2018 16:54:06 +0000 Gerrit-HasComments: Yes
