Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24311 )
Change subject: IMPALA-14999: Calcite planner: support Iceberg tables (part 2) ...................................................................... Patch Set 10: (7 comments) http://gerrit.cloudera.org:8080/#/c/24311/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java: http://gerrit.cloudera.org:8080/#/c/24311/15/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexCallConverter.java@237 PS15, Line 237: Preconditions.checkNotNull > nit: the above Enum.valueOf() doesn't return NULL, it throws either Illegal Removed http://gerrit.cloudera.org:8080/#/c/24311/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaHdfsScanRel.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaHdfsScanRel.java: http://gerrit.cloudera.org:8080/#/c/24311/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaHdfsScanRel.java@133 PS10, Line 133: context.parentAggregate_.hasCountStarOnly() > Nope, you are correct, great find! The non-Iceberg tables require count(*) On second glance... It appears my testing wasn't good. The only reason I can think of is that I tested on v1 and not v2. V1 does allow non-count(*) aggregates in the select list. But v2 does not, and it's because of this loop here and specifically the FunctionCallExpr.isCountStarFunctionCallExpr() check: https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java#L1562-L1572 So I reverted the fix here (and also ensured it won't do the optimization if there is a group). I also had to put a check in IcebergCountStarOptimizer for v2. http://gerrit.cloudera.org:8080/#/c/24311/15/testdata/workloads/functional-query/queries/QueryTest/iceberg-partitioned-insert-v1.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-partitioned-insert-v1.test: http://gerrit.cloudera.org:8080/#/c/24311/15/testdata/workloads/functional-query/queries/QueryTest/iceberg-partitioned-insert-v1.test@303 PS15, Line 303: IMPALA-XXXXX > Placeholder ticket Done http://gerrit.cloudera.org:8080/#/c/24311/15/testdata/workloads/functional-query/queries/QueryTest/iceberg-partitioned-insert-v1.test@316 PS15, Line 316: IMPALA-XXXXX > Placeholder ticket Done http://gerrit.cloudera.org:8080/#/c/24311/15/testdata/workloads/functional-query/queries/QueryTest/iceberg-partitioned-insert-v2.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-partitioned-insert-v2.test: http://gerrit.cloudera.org:8080/#/c/24311/15/testdata/workloads/functional-query/queries/QueryTest/iceberg-partitioned-insert-v2.test@303 PS15, Line 303: IMPALA-XXXXX > Placeholder ticket Done http://gerrit.cloudera.org:8080/#/c/24311/15/testdata/workloads/functional-query/queries/QueryTest/iceberg-partitioned-insert-v2.test@316 PS15, Line 316: IMPALA-XXXXX > Placeholder ticket Done http://gerrit.cloudera.org:8080/#/c/24311/15/testdata/workloads/functional-query/queries/QueryTest/iceberg-v2-plain-count-star-optimization.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-v2-plain-count-star-optimization.test: http://gerrit.cloudera.org:8080/#/c/24311/15/testdata/workloads/functional-query/queries/QueryTest/iceberg-v2-plain-count-star-optimization.test@42 PS15, Line 42: union > Should be BIGINT,BIGINT Sigh... Ok, so this shows that this hasn't been tested :( The only reason I can think of that this got missed is that I accidentally tested the v1 version and that succeeded. But there are big problems here. The good news at least is that this would have failed the jenkins validation when run because this would fail for the original planner too. This test is now fixed, but see my comment in ImpalaHdfsScanRel:133 -- To view, visit http://gerrit.cloudera.org:8080/24311 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1854012b1caac63ced292b338d40074db950b42d Gerrit-Change-Number: 24311 Gerrit-PatchSet: 10 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Sun, 28 Jun 2026 17:51:35 +0000 Gerrit-HasComments: Yes
