Tim Armstrong 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) 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()? I was copying the other method's name, but deepCopy is clearer. 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 I have the basic CUBE test and used CUBE for the other test you requested. I had mostly used ROLLUP for the other tests since the plans are a bit more concise. I also added basic tests for the WITH CUBE/WITH ROLLUP syntax just to prove that it generates the same plan. 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 Oh good idea. http://gerrit.cloudera.org:8080/#/c/16128/6/testdata/workloads/functional-planner/queries/PlannerTest/grouping-sets.test@815 PS6, Line 815: ==== I added some degenerate tests grouping by 1 or 0 columns. 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. Done 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 cha You're right about this - we apply CARDINALITY_FILTER in the planner tests to strip out cardinality values for the purposes of comparison. SO this part of the plan should be ignored for most planner tests (we enable it for a handful with the option VALIDATE_CARDINALITY). -- 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 23:50:35 +0000 Gerrit-HasComments: Yes
