Peter Rozsa has posted comments on this change. ( http://gerrit.cloudera.org:8080/20133 )
Change subject: IMPALA-12089: Be able to skip pushing down a subset of the predicates ...................................................................... Patch Set 5: (17 comments) Thank you for the review Gabor! http://gerrit.cloudera.org:8080/#/c/20133/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20133/4//COMMIT_MSG@24 PS4, Line 24: Iceberg residual expression hand > This 'residual expression backtracking' sounds pretty mysterious. Might be Done http://gerrit.cloudera.org:8080/#/c/20133/4//COMMIT_MSG@37 PS4, Line 37: If Impala is unable to match a > 'If Impala is unable to match all residual expression to Impala conjuncts t Done http://gerrit.cloudera.org:8080/#/c/20133/4//COMMIT_MSG@53 PS4, Line 53: is true. It can be turned of > I think it's in fact 'iceberg_predicate_pushdown_subsetting' Done http://gerrit.cloudera.org:8080/#/c/20133/4//COMMIT_MSG@57 PS4, Line 57: - 1000 expression, 999 skipped: on avreage 2 ms > Did you have the chance to make some perf measurements when you start incre Done http://gerrit.cloudera.org:8080/#/c/20133/4/fe/src/main/java/org/apache/impala/analysis/PredicateVisitor.java File fe/src/main/java/org/apache/impala/analysis/PredicateVisitor.java: http://gerrit.cloudera.org:8080/#/c/20133/4/fe/src/main/java/org/apache/impala/analysis/PredicateVisitor.java@33 PS4, Line 33: > From Impala's perspective this class is only a Visitor for Iceberg predicat Renamed to IcebergExpressionCollector http://gerrit.cloudera.org:8080/#/c/20133/4/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/20133/4/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@118 PS4, Line 118: impalaIcebergPredicate > I think something like 'impalaIcebergPredicateMapping' would be more verbos Done http://gerrit.cloudera.org:8080/#/c/20133/4/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@375 PS4, Line 375: IcebergUtil.planFiles(getIceTable(), > indentation is off with these 2 lines. Done http://gerrit.cloudera.org:8080/#/c/20133/4/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@418 PS4, Line 418: conjuncts_.removeAll(impalaIcebergPredicateMapping_.values()); > Might be personal preference, but if you returned here then you wouldn't ne Done http://gerrit.cloudera.org:8080/#/c/20133/4/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@421 PS4, Line 421: if (!analyzer_.getQueryOptions().iceberg_predicate_pushdown_subsetting) { return; } > Doesn't this fit into the line above? Done http://gerrit.cloudera.org:8080/#/c/20133/4/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@430 PS4, Line 430: If we fail to locate any of the Iceberg residual expressions then we skip > Could you please add some comment here for more clarity, like "If we fail t Done http://gerrit.cloudera.org:8080/#/c/20133/4/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@431 PS4, Line 431: filtering the predicates to be pushed down to Impala scanner. > merge into the line above Done http://gerrit.cloudera.org:8080/#/c/20133/4/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@437 PS4, Line 437: skippedExpressions_.addAll( > I think something the other way around would be more readable if feasible. Done http://gerrit.cloudera.org:8080/#/c/20133/4/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-predicates-disabled-subsetting.test File testdata/workloads/functional-planner/queries/PlannerTest/iceberg-predicates-disabled-subsetting.test: http://gerrit.cloudera.org:8080/#/c/20133/4/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-predicates-disabled-subsetting.test@1 PS4, Line 1: # If one or more non-partition predicates are present, then all predicates are pushed down > Please mention in this comment that the pred subsetting feature is off. I k Done http://gerrit.cloudera.org:8080/#/c/20133/4/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/20133/4/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-predicates.test@77 PS4, Line 77: ==== > I think this iceberg-predicates.test file is the place where we should test Newly added test cases moved to this file http://gerrit.cloudera.org:8080/#/c/20133/4/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-v2-tables.test File testdata/workloads/functional-planner/queries/PlannerTest/iceberg-v2-tables.test: http://gerrit.cloudera.org:8080/#/c/20133/4/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-v2-tables.test@1005 PS4, Line 1005: further ro > user is null ? This is an example for a query that produces untranslated expression. http://gerrit.cloudera.org:8080/#/c/20133/4/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-v2-tables.test@1025 PS4, Line 1025: ned_position > What is an 'untranslated expression' and it's not 100% clear for me why 'ac The Iceberg expression conversion can't convert binary predicates with NULL literals: https://github.com/apache/impala/blob/c8422136962b8d08e5f44d8351fb4fe7cdb675b8/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java#L678 http://gerrit.cloudera.org:8080/#/c/20133/4/testdata/workloads/functional-planner/queries/PlannerTest/iceberg-v2-tables.test@1035 PS4, Line 1035: | > Would be nice to have a test case where there are residual expression but n Added -- To view, visit http://gerrit.cloudera.org:8080/20133 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I597f69ad03ecaf9e304613ef934654e3d9614ae8 Gerrit-Change-Number: 20133 Gerrit-PatchSet: 5 Gerrit-Owner: Peter Rozsa <pro...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Peter Rozsa <pro...@cloudera.com> Gerrit-Comment-Date: Fri, 04 Aug 2023 09:12:37 +0000 Gerrit-HasComments: Yes
