Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/24325 )
Change subject: IMPALA-15013: Fix NULL issues for PIVOT ...................................................................... Patch Set 3: (8 comments) 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: src.val != 0 nit: This is special. Can we add a comment to explain it, e.g. why AggIfMerge() doesn't need this? 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: nit: use 4-space indent 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: "Aggregate function in the PIVOT clause is not supported: " + aggName); We should also allow sum_init_zero, regr_count. They use AggregateFunctions::InitZero() as well. In the future when people add new agg functions that use InitZero(), they need to modify this hardcoded list. Since this is easily overlooked, we should eventually refactor this to be self-registering to prevent future omissions, e.g. add returnsZeroOnEmpty_ to AggregateFunction and set it to true in BuiltinDb when registering the function. Then we can avoid this hardcoded list by if (aggExpr.returnsNonNullOnEmpty() && !aggExpr.returnsZeroOnEmpty()) { throw new AnalysisException(... On the other hand, returnsNonNullOnEmpty() is only correct for builtin functions. UDAFs all have returnsNonNullOnEmpty_ = false: https://github.com/apache/impala/blob/53241a96fba2bada079604676bbacb0db2184493/fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java#L99 It's an existing issue. Let's add a TODO here to mention this. 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 "?" http://gerrit.cloudera.org:8080/#/c/24325/3/fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java@326 PS3, Line 326: entry.getValue().equals(new NullLiteral()) nit: use Expr.IS_NULL_LITERAL.apply() instead of creating a NullLiteral object everytime. 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 + "19AggIfInitZeroUpdateEPN10impala_udf15FunctionContextERKNS1_10BooleanValERKNS1_9BigIntValEPS7_", Can we reuse the existing AggIfUpdate function? prefix + AGGIF_UPDATE_SYMBOL.get(Type.BIGINT) 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: "Aggregate function in the PIVOT clause is not supported: ndv_no_finalize" nit: it'd be more helpful to add the reason, e.g. Unsupported aggregate function that returns non-nulll result on empty input in PIVOT clause: ndv_no_finalize 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. with v(a, b, p) as (values (1, null, 1), (2, 2, 1), (null, 2, 2), (2, 2, 2), (null, null, 2) ) select p1, p2 from v pivot( regr_count(a,b) for p in (1 as p1, 2 as p2)) t; -- 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: 3 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: Thu, 21 May 2026 07:21:06 +0000 Gerrit-HasComments: Yes
