Xuebin Su has posted comments on this change. ( http://gerrit.cloudera.org:8080/24105 )
Change subject: IMPALA-2083: Add PIVOT clause ...................................................................... Patch Set 12: (19 comments) > 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! Thanks for your reviews! http://gerrit.cloudera.org:8080/#/c/24105/9/be/src/exprs/aggregate-functions.h File be/src/exprs/aggregate-functions.h: http://gerrit.cloudera.org:8080/#/c/24105/9/be/src/exprs/aggregate-functions.h@399 PS9, Line 399: /// It skips the current row if 'predicate' is null. > nit: We should remove this if it's allowed now. Thanks! Removed. http://gerrit.cloudera.org:8080/#/c/24105/9/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/24105/9/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@47 PS9, Line 47: import org.apache.impala.authorization.AuthorizationConfig; > nit: this and TableMask, MaterializedViewHdfsTable become unused now. Need Thanks! Removed. 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@96 PS3, Line 96: sourceTableRef_ = sourceTableRef; > Can we add a test for this? Thanks! Added. http://gerrit.cloudera.org:8080/#/c/24105/3/fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java@147 PS3, Line 147: Preconditions.checkState(analyzedAggList_.size() >= 1); > The new patch set still cast the Expr to LiteralExpr which doesn't work for Thanks! Added an item in the commit message and a test case. 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@21 PS9, Line 21: import java.util.HashSet; > nit: unused import Thanks! Removed. http://gerrit.cloudera.org:8080/#/c/24105/9/fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java@73 PS9, Line 73: // Maps each alias to the Agg Expr in the PIVOT clause. : private final Map<String, FunctionCallExpr> analyzedAggList_ = new LinkedHashMap<>(); : > nit: Let's add a comment mentioning what are the keys of these maps, e.g. a Thanks! Added. http://gerrit.cloudera.org:8080/#/c/24105/9/fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java@277 PS9, Line 277: > nit: at first glance, it seems all items use the same alias. But later I re Thanks! Added. http://gerrit.cloudera.org:8080/#/c/24105/9/fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java@291 PS9, Line 291: new SelectListItem(aggItem.getExpr().clone(), singleAggAlias)); > nit: not used Thanks! Removed. http://gerrit.cloudera.org:8080/#/c/24105/9/fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java@298 PS9, Line 298: new FromClause(Lists.newArrayList(sourceTableRef)), null, : new GroupByClause(groupingExprs, GroupingSetsType.NONE), null, null, null); : > Oracle iterates the header values before the agg alias: https://docs.oracle Thanks! Changed. http://gerrit.cloudera.org:8080/#/c/24105/9/fe/src/main/java/org/apache/impala/analysis/TableRef.java File fe/src/main/java/org/apache/impala/analysis/TableRef.java: http://gerrit.cloudera.org:8080/#/c/24105/9/fe/src/main/java/org/apache/impala/analysis/TableRef.java@32 PS9, Line 32: import org.apache.impala.authorization.Privilege; > nit: unused import Thanks! Removed. http://gerrit.cloudera.org:8080/#/c/24105/9/fe/src/main/java/org/apache/impala/analysis/TableRef.java@45 PS9, Line 45: import org.apache.impala.rewrite.ExprRewriter; > nit: unused import Thanks! Removed. http://gerrit.cloudera.org:8080/#/c/24105/9/fe/src/main/java/org/apache/impala/analysis/TableRef.java@885 PS9, Line 885: // Analyzer.registerSlotRef(). It is designed to be overridden by the subclasses. > nit: might worth a comment here explaining its usage. Thanks! Added. http://gerrit.cloudera.org:8080/#/c/24105/9/fe/src/main/java/org/apache/impala/analysis/TableRef.java@915 PS9, Line 915: other.aliases_ = aliases_; > Do you want to implement this for InlineViewRef and CollectionTableRef? Oth Thanks! This refactor is mentioned in https://gerrit.cloudera.org/c/23922/ . 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. Thanks! Added. http://gerrit.cloudera.org:8080/#/c/24105/9/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@467 PS9, Line 467: "Duplicate alias for header values in the PIVOT clause: v1" > Let's add a test for "Duplicate alias for aggregation in the PIVOT clause". Thanks! Added. 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(id) for month in > Lots of tests use the header column in agg, e.g. "min(month) for month", wh Thanks! Added a test case. http://gerrit.cloudera.org:8080/#/c/24105/9/testdata/workloads/functional-query/queries/QueryTest/pivot-clause.test@21 PS9, Line 21: ---- RESULTS > Should be Thanks! Changed. http://gerrit.cloudera.org:8080/#/c/24105/9/testdata/workloads/functional-query/queries/QueryTest/pivot-clause.test@155 PS9, Line 155: with t1 (a, b) as ( > The above tests use inline views. Let's add some tests for local views like Thanks! Added. 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. 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: 12 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 11:39:41 +0000 Gerrit-HasComments: Yes
