Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/16128 )
Change subject: WIP - IMPALA-9898: generate grouping set plans ...................................................................... Patch Set 6: (6 comments) Looks good to me! Have some comments. http://gerrit.cloudera.org:8080/#/c/16128/6/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: http://gerrit.cloudera.org:8080/#/c/16128/6/fe/src/main/java/org/apache/impala/analysis/Expr.java@1104 PS6, Line 1104: cloneLists Maybe rename it to deepCopy()? http://gerrit.cloudera.org:8080/#/c/16128/6/fe/src/main/java/org/apache/impala/analysis/GroupByClause.java File fe/src/main/java/org/apache/impala/analysis/GroupByClause.java: http://gerrit.cloudera.org:8080/#/c/16128/6/fe/src/main/java/org/apache/impala/analysis/GroupByClause.java@58 PS6, Line 58: a bit It may be helpful to provide an example on the mapping of bits set to the corresponding group expressions http://gerrit.cloudera.org:8080/#/c/16128/6/testdata/workloads/functional-planner/queries/PlannerTest/grouping-sets.test File testdata/workloads/functional-planner/queries/PlannerTest/grouping-sets.test: http://gerrit.cloudera.org:8080/#/c/16128/6/testdata/workloads/functional-planner/queries/PlannerTest/grouping-sets.test@1 PS6, Line 1: ROLLUP I wonder if some tests on CUBE should be included, as the name of the test file indicates the test subject is grouping sets. http://gerrit.cloudera.org:8080/#/c/16128/6/testdata/workloads/functional-planner/queries/PlannerTest/grouping-sets.test@330 PS6, Line 330: select int_col, bool_col, string_col, count(*) from functional.alltypes : group by rollup(int_col, 3, 2, 3, string_col) Maybe add another similar query where some expression in the select list is duplicated, such as elect int_col, bool_col, string_col, bool_col, count(*) from functional.alltypes group by rollup(int_col, 3, 2, 4, string_col) Since 2 and 4 refer to the same column bool_Col, # of aggregates (classes) should be 4. http://gerrit.cloudera.org:8080/#/c/16128/6/testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test File testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test: http://gerrit.cloudera.org:8080/#/c/16128/6/testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test@3300 PS6, Line 3300: correlated columns are pushed into the GROUP BY It does not sound right. http://gerrit.cloudera.org:8080/#/c/16128/6/testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test File testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test: http://gerrit.cloudera.org:8080/#/c/16128/6/testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test@8835 PS6, Line 8835: 75.61K Just wonder if the row count here will be compared. If so, seems like a change in the cardinality logic will cause lots of updates here. -- To view, visit http://gerrit.cloudera.org:8080/16128 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie454c5bf7aee266321dee615548d7f2b71380197 Gerrit-Change-Number: 16128 Gerrit-PatchSet: 6 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: David Rorke <dro...@cloudera.com> Gerrit-Reviewer: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 01 Jul 2020 16:18:28 +0000 Gerrit-HasComments: Yes
