Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/10771 )
Change subject: IMPALA-110: Support for multiple DISTINCT ...................................................................... Patch Set 6: (12 comments) http://gerrit.cloudera.org:8080/#/c/10771/5/be/src/exec/streaming-aggregation-node.cc File be/src/exec/streaming-aggregation-node.cc: http://gerrit.cloudera.org:8080/#/c/10771/5/be/src/exec/streaming-aggregation-node.cc@158 PS5, Line 158: this > nit: caps Done http://gerrit.cloudera.org:8080/#/c/10771/5/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/5/fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java@36 PS5, Line 36: * Encapsulates all the information needed to compute a list of aggregate functions with > It might be worth explaining how this relates to MultiAggregateInfo - why i Done http://gerrit.cloudera.org:8080/#/c/10771/5/fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java@38 PS5, Line 38: * A mix of distinct and non-distinct aggregation functions is allowed as long as all > nit: mix of Done http://gerrit.cloudera.org:8080/#/c/10771/5/fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java@205 PS5, Line 205: for (int i = 1; i < distinctAggExprs.size(); ++i) { > Is this reachable now? Can it be a precondition? Done http://gerrit.cloudera.org:8080/#/c/10771/5/fe/src/main/java/org/apache/impala/analysis/AggregateInfoBase.java File fe/src/main/java/org/apache/impala/analysis/AggregateInfoBase.java: http://gerrit.cloudera.org:8080/#/c/10771/5/fe/src/main/java/org/apache/impala/analysis/AggregateInfoBase.java@33 PS5, Line 33: * Base class for AggregateInfo and AnalyticInfo containing the intermediate and output > Update? Still seems correct to me http://gerrit.cloudera.org:8080/#/c/10771/5/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/5/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java@40 PS5, Line 40: * SELECT block including their distributed execution by wrapping a list of > Might be worth explaining more directly the relationship to AggregateInfo u Done http://gerrit.cloudera.org:8080/#/c/10771/5/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java@366 PS5, Line 366: groupingExprs into a CAS > typo: transpose Done http://gerrit.cloudera.org:8080/#/c/10771/5/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java@521 PS5, Line 521: } > nit: unnecessary intermediate variable 'result' Done http://gerrit.cloudera.org:8080/#/c/10771/5/fe/src/main/java/org/apache/impala/planner/AggregationNode.java File fe/src/main/java/org/apache/impala/planner/AggregationNode.java: http://gerrit.cloudera.org:8080/#/c/10771/5/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@176 PS5, Line 176: // TODO: If this is the transposition phase, then we can push conjuncts that > Is this an important optimisation? Not sure, I'll see if I can get something mocked up and do some experiments http://gerrit.cloudera.org:8080/#/c/10771/5/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@277 PS5, Line 277: MURMUR_HASH > Tianyi decided to use FastHash for exchanges over murmur because it has sli Sure, I'd be interested to see what he turned up about the differences, to see if it would have an impact on this particular use case http://gerrit.cloudera.org:8080/#/c/10771/5/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@453 PS5, Line 453: nodeResourceProfile_ = ResourceProfile.noReservation(0); > Probably not if we're making the decision at the aggregator level. Done http://gerrit.cloudera.org:8080/#/c/10771/5/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@519 PS5, Line 519: .setMemEstimateBytes(perInstanceMemEstimate) > nit: Unnecessary intermediate var. 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: 6 Gerrit-Owner: Thomas Marshall <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Tue, 21 Aug 2018 00:18:07 +0000 Gerrit-HasComments: Yes
