Yida Wu 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 7: (8 comments) Some minor 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 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: Preconditions.checkState(prevMulticlassAgg.aggInfos_.size() == aggInfos_.size()); Maybe including actual sizes in the error message? http://gerrit.cloudera.org:8080/#/c/21860/7/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@294 PS7, Line 294: cardinality_ Can we log the cardinality_ is trace is enabled? http://gerrit.cloudera.org:8080/#/c/21860/7/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@303 PS7, Line 303: aggInputCardinality Can we add a Preconditions.checkState for aggInputCardinality, similar to line 247, to protect against potential careless callers? http://gerrit.cloudera.org:8080/#/c/21860/7/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@307 PS7, Line 307: return 1; How about we use a static number for it? something like NON_GROUPING_AGG_NUM_GROUPS or DEFAULT_NUM_GROUPS? http://gerrit.cloudera.org:8080/#/c/21860/7/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@337 PS7, Line 337: single nit. a single http://gerrit.cloudera.org:8080/#/c/21860/7/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@351 PS7, Line 351: Preconditions.checkState(exprsWithUniqueTupleId.size() == groupingExprs.size(), : "Missing expression after TupleId analysis!"); Maybe including actual sizes in the error message? http://gerrit.cloudera.org:8080/#/c/21860/7/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@360 PS7, Line 360: postOrderNodes Seems we can use planNode.getNodesPostOrder() directly, and remove last line -- 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: 7 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: Tue, 08 Oct 2024 00:20:19 +0000 Gerrit-HasComments: Yes
