Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/24105 )
Change subject: IMPALA-2083: Add PIVOT clause ...................................................................... Patch Set 3: (16 comments) http://gerrit.cloudera.org:8080/#/c/24105/3/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/24105/3/fe/src/main/cup/sql-parser.cup@3525 PS3, Line 3525: ident_or_unreserved I think we need dotted_path for header_column to support nested fields, e.g., select * from functional_parquet.complextypes_structs pivot (count(*) for alltypes.ti in (90, 100, 123, 127)); http://gerrit.cloudera.org:8080/#/c/24105/3/fe/src/main/cup/sql-parser.cup@3524 PS3, Line 3524: | dotted_path:path KW_PIVOT LPAREN function_call_expr:aggregation KW_FOR : ident_or_unreserved:header_column KW_IN : LPAREN select_list:header_value_list RPAREN RPAREN alias_clause:alias I think this doesn't support multiple aggregates or multiple pivot columns. https://docs.oracle.com/cd/E11882_01/server.112/e25554/analysis.htm#sthref1224 https://docs.oracle.com/cd/E11882_01/server.112/e25554/analysis.htm#sthref1223 Let's mention this in the commit message. http://gerrit.cloudera.org:8080/#/c/24105/3/fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java File fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java: http://gerrit.cloudera.org:8080/#/c/24105/3/fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java@110 PS3, Line 110: nit: use 4 spaces indent http://gerrit.cloudera.org:8080/#/c/24105/3/fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java@156 PS3, Line 156: boolean isForPivoting) throws AnalysisException { nit: our code style prefers putting the parameters in less lines. http://gerrit.cloudera.org:8080/#/c/24105/3/fe/src/main/java/org/apache/impala/analysis/FromClause.java File fe/src/main/java/org/apache/impala/analysis/FromClause.java: http://gerrit.cloudera.org:8080/#/c/24105/3/fe/src/main/java/org/apache/impala/analysis/FromClause.java@197 PS3, Line 197: set(i, origTblRef.clone()); nit: can we add a comment for using clone() for reset? http://gerrit.cloudera.org:8080/#/c/24105/3/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java File fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java: http://gerrit.cloudera.org:8080/#/c/24105/3/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java@592 PS3, Line 592: int numAggs = aggTuple.getSlots().size() - numGroupingExprs; nit: not used http://gerrit.cloudera.org:8080/#/c/24105/3/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java@598 PS3, Line 598: Operator.EQ, headerSlotRef, e.getValue().clone()); I think it won't be hard to support pivoting on multiple columns. We just need CompoundPredicates to connect all the EQ predicates. http://gerrit.cloudera.org:8080/#/c/24105/3/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/3/fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java@62 PS3, Line 62: private final LinkedHashMap<String, LiteralExpr> headerValueMap_ = nit: please add a comment about using LinkedHashMap to perserve the order of header values. nit: We prefer declaring the variable using interface type Map, e.g., in Analyzer: public final Map<ExprId, Expr> conjuncts = new LinkedHashMap<>(); http://gerrit.cloudera.org:8080/#/c/24105/3/fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java@96 PS3, Line 96: Expr aggArg = aggExpr.getParams().exprs().get(0); Aggregation functions could have more than one parameter, e.g., group_concat(col, separator). We need to clone all the params. http://gerrit.cloudera.org:8080/#/c/24105/3/fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java@147 PS3, Line 147: LiteralExpr e = (LiteralExpr)item.getExpr(); It's not guaranteed that these are literals. I think exprs that have constant results also worth to support, e.g., month(now()), 1 + 1, etc. For non-constant exprs, we should throw AnalysisExceptions. http://gerrit.cloudera.org:8080/#/c/24105/3/fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java@206 PS3, Line 206: void notifySlotRefRegistered(Analyzer analyzer, SlotDescriptor slot) nit: add a @Override annotation http://gerrit.cloudera.org:8080/#/c/24105/3/fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java@209 PS3, Line 209: String columnName = rawPath.get(0); This might loss the field names for nested columns. I see an exception that might due to this: set EXPAND_COMPLEX_TYPES=true; select * from functional_parquet.complextypes_structs pivot (count(*) for str in ('first item', 'second item')) t; I20260324 10:25:42.164049 271305 jni-util.cc:335] 9046ddbdbe70501a:97c1513a00000000] java.lang.RuntimeException: java.lang.IllegalStateException at org.apache.impala.service.Frontend.getTExecRequestWithFallback(Frontend.java:2459) at org.apache.impala.service.Frontend.createExecRequest(Frontend.java:2126) at org.apache.impala.service.JniFrontend.createExecRequest(JniFrontend.java:175) Caused by: java.lang.IllegalStateException at com.google.common.base.Preconditions.checkState(Preconditions.java:496) at org.apache.impala.analysis.SlotRef.addStructChildrenAsSlotRefs(SlotRef.java:264) at org.apache.impala.analysis.SlotRef.<init>(SlotRef.java:93) at org.apache.impala.analysis.AggregateInfoBase.createTupleDesc(AggregateInfoBase.java:135) at org.apache.impala.analysis.AggregateInfoBase.createTupleDescs(AggregateInfoBase.java:101) at org.apache.impala.analysis.AggregateInfo.create(AggregateInfo.java:173) at org.apache.impala.analysis.AggregateInfo.create(AggregateInfo.java:202) at org.apache.impala.analysis.MultiAggregateInfo.groupAggExprs(MultiAggregateInfo.java:318) at org.apache.impala.analysis.MultiAggregateInfo.analyzeForPivoting(MultiAggregateInfo.java:559) at org.apache.impala.analysis.PivotTableRef.createMultiAggregateInfo(PivotTableRef.java:251) at org.apache.impala.analysis.SelectStmt$SelectAnalyzer.combineBaseTableSmaps(SelectStmt.java:841) at org.apache.impala.analysis.SelectStmt$SelectAnalyzer.analyze(SelectStmt.java:369) at org.apache.impala.analysis.SelectStmt.analyze(SelectStmt.java:277) at org.apache.impala.analysis.AnalysisContext$AnalysisDriverImpl.analyze(AnalysisContext.java:565) at org.apache.impala.analysis.AnalysisContext.analyzeAndAuthorize(AnalysisContext.java:496) at org.apache.impala.service.Frontend.doCreateExecRequest(Frontend.java:3033) at org.apache.impala.service.Frontend.getTExecRequest(Frontend.java:2590) at org.apache.impala.service.Frontend.getTExecRequestWithFallback(Frontend.java:2436) ... 2 more http://gerrit.cloudera.org:8080/#/c/24105/3/fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java@258 PS3, Line 258: } catch (ClassCastException e) { Using ClassCastException for type checking is generally discouraged in Java. Please use instanceof. http://gerrit.cloudera.org:8080/#/c/24105/3/fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java@300 PS3, Line 300: .toString() nit: Unnecessary 'toString()' call http://gerrit.cloudera.org:8080/#/c/24105/3/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/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@455 PS3, Line 455: ); Let's add another test for "month in (1, 2 as `1`)" http://gerrit.cloudera.org:8080/#/c/24105/3/testdata/workloads/functional-query/queries/QueryTest/ranger_column_masking.test File testdata/workloads/functional-query/queries/QueryTest/ranger_column_masking.test: http://gerrit.cloudera.org:8080/#/c/24105/3/testdata/workloads/functional-query/queries/QueryTest/ranger_column_masking.test@994 PS3, Line 994: ==== Let's add some tests in ranger_column_masking.test and ranger_row_filtering.test to make sure this feature works with Ranger column-masking/row-filtering. -- 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: 3 Gerrit-Owner: Xuebin Su <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Tue, 24 Mar 2026 05:51:57 +0000 Gerrit-HasComments: Yes
