Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/20379 )
Change subject: IMPALA-12383: Fix SingleNodePlanner aggregation limits ...................................................................... Patch Set 4: (4 comments) The change looks great. Just have one question on performance. http://gerrit.cloudera.org:8080/#/c/20379/1/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/20379/1/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@85 PS1, Line 85: endsMultiPhase_ = f > Looked into this a bit more. Most of those paths don't use pushdown because Done http://gerrit.cloudera.org:8080/#/c/20379/4/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/20379/4/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@83 PS4, Line 83: finalize nit node? http://gerrit.cloudera.org:8080/#/c/20379/4/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@172 PS4, Line 172: Preconditions.checkState(needsFinalize_); && !endsMultiPhase_ http://gerrit.cloudera.org:8080/#/c/20379/4/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@786 PS4, Line 786: && isMultiPhase() I wonder if this effectively requires all non-root aggregation nodes (i.e., their endsMultiPhase_ is false) to complete their work first, prior to the merge aggregate node. If so, this may cause performance degradation. Is it possible to fix this bug in backend by counting number of values x coming up at the merge aggregate and bailing out of the loop when x >= n? -- To view, visit http://gerrit.cloudera.org:8080/20379 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic5eec1190e8e182152aa954897b79cc3f219c816 Gerrit-Change-Number: 20379 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qfc...@hotmail.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Comment-Date: Tue, 22 Aug 2023 18:11:14 +0000 Gerrit-HasComments: Yes
