Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24325 )
Change subject: IMPALA-15013: Fix NULL issues for PIVOT ...................................................................... Patch Set 2: (5 comments) Thanks for fixing this! http://gerrit.cloudera.org:8080/#/c/24325/2/be/src/exprs/aggregate-functions.h File be/src/exprs/aggregate-functions.h: http://gerrit.cloudera.org:8080/#/c/24325/2/be/src/exprs/aggregate-functions.h@407 PS2, Line 407: static void AggIfInitZeroUpdate( Nit, optional: Do we care if the order is different in the C++ file versus how it's laid out here? I personally don't care, just noticed the difference. http://gerrit.cloudera.org:8080/#/c/24325/2/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/2/fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java@201 PS2, Line 201: if (aggExpr.returnsNonNullOnEmpty()) { Question: Is it the "returnsNonNullOnEmpty()" function what determines whether we use aggif_init_zero? Maybe we can get rid of this check (and see my other comment below) http://gerrit.cloudera.org:8080/#/c/24325/2/fe/src/main/java/org/apache/impala/analysis/PivotTableRef.java@331 PS2, Line 331: String transposeAggName = (aggName.equals("count") || aggName.equals("ndv")) ? Second comment related to the one above: Can we just do a check for returnsNonNullOnEmpty and set it to aggif_init_zero? http://gerrit.cloudera.org:8080/#/c/24325/2/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/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@521 PS2, Line 521: " ndv_no_finalize(month) for month in (1 as v1)) as t;", Related to my other comments... I suppose this would be the one function we don't want using...but do we ever really use this function? Isn't this one sort of hidden? How do we treat other "*_no_finalize" functions...I would guess those would be kinda weird to use? http://gerrit.cloudera.org:8080/#/c/24325/2/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/2/testdata/workloads/functional-query/queries/QueryTest/pivot-clause.test@436 PS2, Line 436: # The PIVOT clause contains the NULL literal. Do we want an NDV test too? -- 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: 2 Gerrit-Owner: Xuebin Su <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Tue, 19 May 2026 15:36:08 +0000 Gerrit-HasComments: Yes
