Zoltan Borok-Nagy 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 16: Code-Review+2 (5 comments) Only left small comments that are easy to fix. Feel free to carry my +2 afterwards. http://gerrit.cloudera.org:8080/#/c/24311/16//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24311/16//COMMIT_MSG@9 PS16, Line 9: Note to reviewers: If this commit is too big, I can break this into smaller chunks : based on the description in this commit. The only issue with doing it this : way is that my testing was with existing Iceberg tests while using the Calcite : planner. It will be awkward to isolate the tests just for the purpose of : getting small parts of the Calcite functionality to work. : : Second note to reviewers: As mentioned in the previous paragraph, the testing : was done with the Calcite planner. While a couple of tests were added to : calcite.test to ensure that some Iceberg tests are working, there is no way : to tell the difference between tests that work on Calcite versus ones that : fail on Calcite and fallback to the original planner. This is still in the : works and will be there within the coming months. : : The above two paragraphs are just for the review process and will be removed : once this has been reviewed. Don't forget to remove these lines. http://gerrit.cloudera.org:8080/#/c/24311/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/IcebergCountStarOptimizer.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/IcebergCountStarOptimizer.java: http://gerrit.cloudera.org:8080/#/c/24311/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/IcebergCountStarOptimizer.java@75 PS16, Line 75: because of the extra : * sum that is needed, but a Project on top adds the precalculated count to the : * count needed to handle the delete files. All other non-count(*) expressions remain : * as/is (if they exist). So if the output of the Agg was 2 columns, sum(col) and : * count(*), the 2 columns of the project will be "InputRef($0)" (for the sum) and : * "InputRef($1) + <precalculated count(*) constant>" for the count(*). Outdated due to L115. http://gerrit.cloudera.org:8080/#/c/24311/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/IcebergCountStarOptimizer.java@112 PS16, Line 112: : nit: extra empty line http://gerrit.cloudera.org:8080/#/c/24311/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/IcebergCountStarOptimizer.java@147 PS16, Line 147: !(aggCall.getAggregation().getKind().equals(SqlKind.COUNT) && : aggCall.getArgList().size() == 0)) { : // For any non-count(*), the input is just the inputref, which is the value that : // comes out of the agg. : projects.add(aggInput); Dead code? http://gerrit.cloudera.org:8080/#/c/24311/16/testdata/workloads/functional-query/queries/QueryTest/iceberg-plain-count-star-optimization.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-plain-count-star-optimization.test: http://gerrit.cloudera.org:8080/#/c/24311/16/testdata/workloads/functional-query/queries/QueryTest/iceberg-plain-count-star-optimization.test@11 PS16, Line 11: count(*) Can we add test for 'count(*) + 1000' as well? Currently it is not optimized, and probably it's OK as it's a strange expression, but we should verify its correctness. -- 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: 16 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: Wed, 01 Jul 2026 13:18:19 +0000 Gerrit-HasComments: Yes
