Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/24105 )
Change subject: IMPALA-2083: Add PIVOT clause ...................................................................... Patch Set 9: (9 comments) http://gerrit.cloudera.org:8080/#/c/24105/9/be/src/exprs/aggregate-functions-ir.cc File be/src/exprs/aggregate-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/24105/9/be/src/exprs/aggregate-functions-ir.cc@3287 PS9, Line 3287: if (!cond.is_null && cond.val) *dst = src; nit: Please mention this change in the commit message, e.g. when will cond.is_null be true and why we need it for this patch. 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: /// The predicate must not evaluate to NULL. nit: We should remove this if it's allowed now. http://gerrit.cloudera.org:8080/#/c/24105/9/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java: http://gerrit.cloudera.org:8080/#/c/24105/9/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@625 PS9, Line 625: if (stmt_ instanceof SelectStmt) { > It'd be more tidy to use a method like requiresSubqueryRewrite(). Then we c This also skips DMLs like CreateTableAsSelect, INSERT. I think we should be able to support them. If not, let's add an error message. Please also add tests for DMLs that use PIVOT. 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.AuthorizationChecker; nit: this and TableMask, MaterializedViewHdfsTable become unused now. Need to remove the imports as well. 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? Do we have a test case for that. Another question is should we require 'analyzer' here is the same that we used in analyze() ? 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.AuthorizationContext; nit: unused import 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.planner.PlanNode; nit: unused import http://gerrit.cloudera.org:8080/#/c/24105/9/fe/src/main/java/org/apache/impala/analysis/TableRef.java@885 PS9, Line 885: void notifySlotRefRegistered(Analyzer analyzer, SlotDescriptor slot) nit: might worth a comment here explaining its usage. http://gerrit.cloudera.org:8080/#/c/24105/9/fe/src/main/java/org/apache/impala/analysis/TableRef.java@915 PS9, Line 915: public TableRef applyTableMask(Analyzer analyzer) throws AnalysisException { Do you want to implement this for InlineViewRef and CollectionTableRef? Otherwise, it seems we don't need to move it from Analyzer to here. If we do need this, please mention this refactor in the commit message. -- 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 04:07:47 +0000 Gerrit-HasComments: Yes
