Xuebin Su has posted comments on this change. ( http://gerrit.cloudera.org:8080/24105 )
Change subject: IMPALA-2083: Add PIVOT clause ...................................................................... Patch Set 5: (15 comments) > Patch Set 3: > > (16 comments) Thanks for your review! 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 Thanks! I think nested fields can be extracted in a subquery. 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 select_list:agg_list 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. Thanks! Added "Limitations" section 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 Thanks! http://gerrit.cloudera.org:8080/#/c/24105/3/fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java@156 PS3, Line 156: } > nit: our code style prefers putting the parameters in less lines. Thanks! 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: // PivotTableRef.clone() creates an unanalyzed clone and can be used for reset. > nit: can we add a comment for using clone() for reset? Thanks! Added. 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: */ > nit: not used Thanks! http://gerrit.cloudera.org:8080/#/c/24105/3/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java@598 PS3, Line 598: // we use a bigint type for the grouping_id later, so we allow up to > I think it won't be hard to support pivoting on multiple columns. We just n Thanks! I agree. 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: public class PivotTableRef extends TableRef { > nit: please add a comment about using LinkedHashMap to perserve the order o Thanks! Changed. http://gerrit.cloudera.org:8080/#/c/24105/3/fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java@96 PS3, Line 96: > Aggregation functions could have more than one parameter, e.g., group_conca Thanks! Changed to cloning the whole list. http://gerrit.cloudera.org:8080/#/c/24105/3/fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java@147 PS3, Line 147: } > It's not guaranteed that these are literals. I think exprs that have consta Thanks! I think they are supported in the new Patch Set. http://gerrit.cloudera.org:8080/#/c/24105/3/fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java@206 PS3, Line 206: List<String> rawPath = slot.getPath().getRawPath(); > nit: add a @Override annotation Thanks! Added http://gerrit.cloudera.org:8080/#/c/24105/3/fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java@209 PS3, Line 209: > This might loss the field names for nested columns. I see an exception that Thanks! Fixed. http://gerrit.cloudera.org:8080/#/c/24105/3/fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java@258 PS3, Line 258: InlineViewRef multiAggInlineViewRef = new InlineViewRef( > Using ClassCastException for type checking is generally discouraged in Java Thanks! http://gerrit.cloudera.org:8080/#/c/24105/3/fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java@300 PS3, Line 300: > nit: Unnecessary 'toString()' call Thanks! Removed. 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`)" Thanks! Added. -- 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: 5 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: Wed, 01 Apr 2026 09:11:30 +0000 Gerrit-HasComments: Yes
