Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22046 )
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner ...................................................................... Patch Set 4: Code-Review+1 (3 comments) Just some nits. http://gerrit.cloudera.org:8080/#/c/22046/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/22046/4/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@223 PS4, Line 223: // to the proper place in the distributed plan via transferConjuncts(). Is this comment still accurate? http://gerrit.cloudera.org:8080/#/c/22046/4/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/22046/4/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@a1073 PS4, Line 1073: I see you deprecated transferConjuncts. It looks like what was happening is you did orderConjunctsByCost, then transferred additional conjuncts. And then didn't order again. So this both saves recomputing stats, and ensures all conjuncts are included in orderConjunctsByCost. http://gerrit.cloudera.org:8080/#/c/22046/4/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@1055 PS4, Line 1055: List<Expr> havingPredicates = new ArrayList<>(node.conjuncts_); nit: could you do List<Expr> havingPredicates = node.conjuncts_; node.conjuncts_ = new ArrayList<>(); -- To view, visit http://gerrit.cloudera.org:8080/22046 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d Gerrit-Change-Number: 22046 Gerrit-PatchSet: 4 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Fri, 06 Dec 2024 20:18:48 +0000 Gerrit-HasComments: Yes
