Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19534 )
Change subject: IMPALA-11701 Part1: Don't push down predicates if already applied by Iceberg ...................................................................... Patch Set 3: (7 comments) Thanks Gabor. Could you also address the comments on the commit message? http://gerrit.cloudera.org:8080/#/c/19534/3/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/19534/3/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1390 PS3, Line 1390: is 'is' is not needed here? http://gerrit.cloudera.org:8080/#/c/19534/3/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@2027 PS3, Line 2027: explainLevel 'explainLevel' may not be the best name as it could be confused with the verbosity level of the explain query ('detailLevel' here) - we can use "set EXPLAIN_LEVEL=..." in the shell. 'indentPrefix' would be better. http://gerrit.cloudera.org:8080/#/c/19534/1/fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java File fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java: http://gerrit.cloudera.org:8080/#/c/19534/1/fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java@64 PS1, Line 64: > 'skippedConjuncts_' is already removed from 'conjuncts_' in the Iceberg pla The comment on 'nonIdentityConjuncts_' says that it is a subset of 'conjuncts_' that does not include the conjuncts pushed down to Iceberg - isn't the latter set the same as 'skippedConjuncts_'? That's why I thought that the union of 'nonIdentityConjuncts_' and 'skippedConjuncts_' would be the original value of 'conjuncts_'. But then after removing 'skippedConjuncts_' from 'conjuncts_', 'nonIdentityConjuncts_' is not a (proper) subset of 'conjuncts_' but the same as 'conjuncts_'. Is this not correct? http://gerrit.cloudera.org:8080/#/c/19534/3/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java File fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java: http://gerrit.cloudera.org:8080/#/c/19534/3/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@115 PS3, Line 115: have Nit: has http://gerrit.cloudera.org:8080/#/c/19534/1/fe/src/test/java/org/apache/impala/planner/PlannerTest.java File fe/src/test/java/org/apache/impala/planner/PlannerTest.java: http://gerrit.cloudera.org:8080/#/c/19534/1/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@1279 PS1, Line 1279: exercising > I googled it, and for me it seems that it's 'exercising' :) You're right, sorry :) http://gerrit.cloudera.org:8080/#/c/19534/3/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-predicates.test File testdata/workloads/functional-planner/queries/PlannerTest/iceberg-predicates.test: http://gerrit.cloudera.org:8080/#/c/19534/3/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-predicates.test@49 PS3, Line 49: could be Could not be? http://gerrit.cloudera.org:8080/#/c/19534/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-in-predicate-push-down.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-in-predicate-push-down.test: http://gerrit.cloudera.org:8080/#/c/19534/3/testdata/workloads/functional-query/queries/QueryTest/iceberg-in-predicate-push-down.test@51 PS3, Line 51: predicae Nit: predicate -- To view, visit http://gerrit.cloudera.org:8080/19534 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icfa80ce469cecfcfbcd0dcb595a6b04b7027285b Gerrit-Change-Number: 19534 Gerrit-PatchSet: 3 Gerrit-Owner: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Fri, 03 Mar 2023 13:28:15 +0000 Gerrit-HasComments: Yes
