Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/24105 )
Change subject: IMPALA-2083: Add PIVOT clause ...................................................................... Patch Set 9: (7 comments) I've done the second round of review. The rewrite solution LGTM. I think we just need to fix some special cases like nested PIVOT and add more tests like DMLs. If some special cases are not supported, we need better error messages. Thanks for this hard work! http://gerrit.cloudera.org:8080/#/c/24105/9/fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java File fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java: http://gerrit.cloudera.org:8080/#/c/24105/9/fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java@216 PS9, Line 216: sourceTableSlotPath > Just curious, is it possible that sourceTableRef_ is also a PivotTableRef? Just tried nested PIVOT, it doesn't work: with t1 as ( select year, v1, v2 from functional_parquet.alltypestiny pivot ( min(id) for month in (1 as v1, 2 as v2)) as t ) select * from t1 pivot ( min(v1) for year in (2009 as y2009)) as t2; RuntimeException: java.lang.IllegalStateException CAUSED BY: IllegalStateException: null http://gerrit.cloudera.org:8080/#/c/24105/9/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/24105/9/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@450 PS9, Line 450: ); Let's also test multiple aggs in this scenario. AnalysisError( "with t1 (v2_min, b) as (select 1, 2) " + "select * from t1 pivot (" + " min(b) as min, max(b) as max for b in (2 as v2, 4 as v4)) as t;", "Duplicate column name: v2_min" ); http://gerrit.cloudera.org:8080/#/c/24105/9/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@467 PS9, Line 467: ); Let's add a test for "Duplicate alias for aggregation in the PIVOT clause". http://gerrit.cloudera.org:8080/#/c/24105/9/testdata/workloads/functional-query/queries/QueryTest/pivot-clause.test File testdata/workloads/functional-query/queries/QueryTest/pivot-clause.test: http://gerrit.cloudera.org:8080/#/c/24105/9/testdata/workloads/functional-query/queries/QueryTest/pivot-clause.test@6 PS9, Line 6: min(month) for month Lots of tests use the header column in agg, e.g. "min(month) for month", which is not that easy to understand the purpose for people that are new to this feature. It'd be better to use non-header column in agg, e.g. min(id), so the agg function actaully aggregates multiple values. http://gerrit.cloudera.org:8080/#/c/24105/9/testdata/workloads/functional-query/queries/QueryTest/pivot-clause.test@21 PS9, Line 21: ==== QUERY Should be ==== ---- Query http://gerrit.cloudera.org:8080/#/c/24105/9/testdata/workloads/functional-query/queries/QueryTest/pivot-clause.test@155 PS9, Line 155: ==== The above tests use inline views. Let's add some tests for local views like functional.alltypes_view. E.g. select year, m5, m6 from functional.alltypes_view pivot( count(id) for month in (5 as m5, 6 as m6)) t; http://gerrit.cloudera.org:8080/#/c/24105/9/testdata/workloads/functional-query/queries/QueryTest/pivot-clause.test@329 PS9, Line 329: ==== Let's test the HAVING clause, e.g. select year, m5, m6 from functional.alltypes pivot( max(id) for month in (5 as m5, 6 as m6)) t having m5 > 2000; -- To view, visit http://gerrit.cloudera.org:8080/24105 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib799b772be18d2a3db8983b24e1068e7dfdf1ca9 Gerrit-Change-Number: 24105 Gerrit-PatchSet: 9 Gerrit-Owner: Xuebin Su <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Xuebin Su <[email protected]> Gerrit-Comment-Date: Thu, 14 May 2026 07:11:55 +0000 Gerrit-HasComments: Yes
