Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/10771 )
Change subject: IMPALA-110: Support for multiple DISTINCT ...................................................................... Patch Set 10: (17 comments) http://gerrit.cloudera.org:8080/#/c/10771/10/be/src/exec/aggregation-node.cc File be/src/exec/aggregation-node.cc: http://gerrit.cloudera.org:8080/#/c/10771/10/be/src/exec/aggregation-node.cc@57 PS10, Line 57: make_unique<RowBatch>(child(0)->row_desc(), state->batch_size(), mem_tracker())); > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/10771/10/be/src/exec/streaming-aggregation-node.cc File be/src/exec/streaming-aggregation-node.cc: http://gerrit.cloudera.org:8080/#/c/10771/10/be/src/exec/streaming-aggregation-node.cc@101 PS10, Line 101: make_unique<RowBatch>(child(0)->row_desc(), state->batch_size(), mem_tracker())); > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/10771/10/fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java File fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java: http://gerrit.cloudera.org:8080/#/c/10771/10/fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java@40 PS10, Line 40: unique set of DISTINCT expressions > nit: unique list of grouping expressions? s/set/list/ is fine I think it's clearer to say 'DISTINCT exprs' than 'grouping exprs', since the grouping exprs will vary for different stages within a class. http://gerrit.cloudera.org:8080/#/c/10771/10/fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java@53 PS10, Line 53: contains the phase 2 aggregate functions and : * grouping exprs > also contains merging aggregate functions for non-distinct aggregates, righ No, those will be in aggInfo.secondPhaseDistinctAggInfo.mergeAggInfo http://gerrit.cloudera.org:8080/#/c/10771/10/fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java@56 PS10, Line 56: grouping exprs are identical > identical to aggInfo's grouping exprs Done http://gerrit.cloudera.org:8080/#/c/10771/10/fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java@58 PS10, Line 58: grouping exprs are identical > grouping exprs are identical to aggInfo.secondPhaseDistinctAggInfo's groupi Done http://gerrit.cloudera.org:8080/#/c/10771/10/fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java@178 PS10, Line 178: * At the moment, we require that all distinct aggregate : * functions be applied to the same set of exprs (ie, we can't do something : * like SELECT COUNT(DISTINCT id), COUNT(DISTINCT address)). > Obsolete comment Done http://gerrit.cloudera.org:8080/#/c/10771/10/fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java@193 PS10, Line 193: * TODO: expand implementation to cover the general case; this will require : * a different execution strategy > please resolve TODO Done http://gerrit.cloudera.org:8080/#/c/10771/10/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java File fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java: http://gerrit.cloudera.org:8080/#/c/10771/10/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java@44 PS10, Line 44: DISTINCT exprs. > nit: list of grouping exprs? Done http://gerrit.cloudera.org:8080/#/c/10771/10/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java@57 PS10, Line 57: the non-distinct class is carried along the two phases > Could be elaborated a bit: Done http://gerrit.cloudera.org:8080/#/c/10771/10/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java@78 PS10, Line 78: To distinguish between the aggregation cla > nit: Maybe it would be better to start with the context/motivation before g Done http://gerrit.cloudera.org:8080/#/c/10771/10/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java@289 PS10, Line 289: aggClass > nit: we both have aggregation classes and aggregation infos here, I think i Done http://gerrit.cloudera.org:8080/#/c/10771/10/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java@307 PS10, Line 307: has > typo: hash, but as far I understood no hashing is involved in the transposi Done http://gerrit.cloudera.org:8080/#/c/10771/10/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java@472 PS10, Line 472: lastAggClass = aggInfos_ > nit: lastAggInfo? Done http://gerrit.cloudera.org:8080/#/c/10771/10/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java@476 PS10, Line 476: else > I'm just wondering if this 'else' is intentional here. Yes, this was a bug. Good catch! Added a test. http://gerrit.cloudera.org:8080/#/c/10771/10/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java@649 PS10, Line 649: aggClass : aggInfos_ > nit: agg class and agg info also used interchangeably here Done http://gerrit.cloudera.org:8080/#/c/10771/10/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java File fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java: http://gerrit.cloudera.org:8080/#/c/10771/10/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@793 PS10, Line 793: * for the phase 2 AggregationNode. > Might be worth mentioning the case when the aggregation phase is transpose. Done -- 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: 10 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: Fri, 07 Sep 2018 21:54:47 +0000 Gerrit-HasComments: Yes
