Xuebin Su has posted comments on this change. ( http://gerrit.cloudera.org:8080/24325 )
Change subject: IMPALA-15013: Fix NULL issues for PIVOT ...................................................................... Patch Set 5: (8 comments) > Patch Set 3: > > (8 comments) Thanks for your reviews! http://gerrit.cloudera.org:8080/#/c/24325/3/be/src/exprs/aggregate-functions-ir.cc File be/src/exprs/aggregate-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/24325/3/be/src/exprs/aggregate-functions-ir.cc@3308 PS3, Line 3308: s::AggIfMerg > nit: This is special. Can we add a comment to explain it, e.g. why AggIfMer Thanks! Added. http://gerrit.cloudera.org:8080/#/c/24325/3/be/src/exprs/aggregate-functions.h File be/src/exprs/aggregate-functions.h: http://gerrit.cloudera.org:8080/#/c/24325/3/be/src/exprs/aggregate-functions.h@408 PS3, Line 408: static > nit: use 4-space indent Thanks! http://gerrit.cloudera.org:8080/#/c/24325/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/24325/3/fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java@205 PS3, Line 205: throw new AnalysisException( > We should also allow sum_init_zero, regr_count. They use AggregateFunctions Thanks! Changed. http://gerrit.cloudera.org:8080/#/c/24325/3/fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java@324 PS3, Line 324: > nti: missing space after "?" Thanks! Added. http://gerrit.cloudera.org:8080/#/c/24325/3/fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java@326 PS3, Line 326: s needed to keep the headerValueMap read-o > nit: use Expr.IS_NULL_LITERAL.apply() instead of creating a NullLiteral obj Thanks! Changed. http://gerrit.cloudera.org:8080/#/c/24325/3/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java File fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java: http://gerrit.cloudera.org:8080/#/c/24325/3/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java@1349 PS3, Line 1349: prefix + AGGIF_UPDATE_SYMBOL.get(Type.BIGINT), > Can we reuse the existing AggIfUpdate function? Thanks! Changed. http://gerrit.cloudera.org:8080/#/c/24325/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/24325/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@522 PS3, Line 522: "Duplicate alias for aggregations in the PIVOT clause: min" > nit: it'd be more helpful to add the reason, e.g. Thanks! Added. http://gerrit.cloudera.org:8080/#/c/24325/3/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/24325/3/testdata/workloads/functional-query/queries/QueryTest/pivot-clause.test@466 PS3, Line 466: ==== > I think at least we should support builtin functions like regr_count, e.g. Thanks! Added the support. -- To view, visit http://gerrit.cloudera.org:8080/24325 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idae4a510dd3d6c4cd16a3e03aa0161ad84863e5a Gerrit-Change-Number: 24325 Gerrit-PatchSet: 5 Gerrit-Owner: Xuebin Su <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Reviewer: Xuebin Su <[email protected]> Gerrit-Comment-Date: Fri, 29 May 2026 09:24:44 +0000 Gerrit-HasComments: Yes
