Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21860 )
Change subject: IMPALA-13405: Do tuple analysis to lower AggregationNode cardinality ...................................................................... Patch Set 10: (8 comments) http://gerrit.cloudera.org:8080/#/c/21860/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21860/7//COMMIT_MSG@14 PS7, Line 14: consider > nit. considers Done http://gerrit.cloudera.org:8080/#/c/21860/7/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/21860/7/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@252 PS7, Line 252: > Maybe including actual sizes in the error message? Done http://gerrit.cloudera.org:8080/#/c/21860/7/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@294 PS7, Line 294: cardinalit > Can we log the cardinality_ is trace is enabled? Done http://gerrit.cloudera.org:8080/#/c/21860/7/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@303 PS7, Line 303: getDisplayLabel(), > Can we add a Preconditions.checkState for aggInputCardinality, similar to l Done http://gerrit.cloudera.org:8080/#/c/21860/7/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@307 PS7, Line 307: > How about we use a static number for it? something like NON_GROUPING_AGG_NU Done http://gerrit.cloudera.org:8080/#/c/21860/7/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@337 PS7, Line 337: > nit. a single Done http://gerrit.cloudera.org:8080/#/c/21860/7/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@351 PS7, Line 351: exprsWithUniqueTupleId.addAll(entry.getValue()); : it.remove(); > Maybe including actual sizes in the error message? Done http://gerrit.cloudera.org:8080/#/c/21860/7/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@360 PS7, Line 360: exprsWithUniqu > Seems we can use planNode.getNodesPostOrder() directly, and remove last lin It does not work, I presume because the signature of getNodesPostOrder() has generic template. public <C extends TreeNode<NodeType>> List<C> getNodesPostOrder() I can only make it work if I store it into variable first, like in L359. -- To view, visit http://gerrit.cloudera.org:8080/21860 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icd589ab5f7ba9566a0d35784f61f5ffaef5696e7 Gerrit-Change-Number: 21860 Gerrit-PatchSet: 10 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Mon, 14 Oct 2024 23:55:30 +0000 Gerrit-HasComments: Yes
