Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19768 )
Change subject: IMPALA-12051: Propagate analytic tuple predicates of outer-joined InlineView ...................................................................... Patch Set 2: (6 comments) Thanks for fixing this! The fix LGTM. http://gerrit.cloudera.org:8080/#/c/19768/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19768/2//COMMIT_MSG@9 PS2, Line 9: In some cases, direct pushing down predicates that reference analytic tuple into inline view leads to incorrect query results. such as sql: nit: wrap this line into 72 characters Could you also explain how the bug happens? IIUC, while pushing down analytic predicates (e.g. row_number() < 10), we should also divide them into two groups. Some of them can be migrated into the view so are removed in the current scope. Some of them can be copied into the view but still need to be evaluated in the current scope. The bug is due to we migrate all of them into the view. http://gerrit.cloudera.org:8080/#/c/19768/2/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java: http://gerrit.cloudera.org:8080/#/c/19768/2/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1394 PS2, Line 1394: List<Expr> preds = new ArrayList<>(); Line 1394 to 1404 duplicates the code from line 1415 to 1427 (except the logging code). Can we extract them into a method? The method name can be something like "migrateOrCopyConjunctsToInlineView" http://gerrit.cloudera.org:8080/#/c/19768/2/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1399 PS2, Line 1399: preds It'd be helpful to also log 'evalAfterJoinPreds'. We can improve this to LOG.trace("Migrate analytic predicates into view: {}. " + "Copy analytic predicates into view: {}", Expr.debugString(preds), Expr.debugString(evalAfterJoinPreds)); http://gerrit.cloudera.org:8080/#/c/19768/2/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1422 PS2, Line 1422: viewPredicates below nit: This comment is stale. Please remove it by the way. http://gerrit.cloudera.org:8080/#/c/19768/2/testdata/workloads/functional-planner/queries/PlannerTest/analytic-rank-pushdown.test File testdata/workloads/functional-planner/queries/PlannerTest/analytic-rank-pushdown.test: http://gerrit.cloudera.org:8080/#/c/19768/2/testdata/workloads/functional-planner/queries/PlannerTest/analytic-rank-pushdown.test@2041 PS2, Line 2041: b.rnk > 100 Shouldn't this be "b.rnk < 100" ? In the plan, the predicate is "row_number() < 100". http://gerrit.cloudera.org:8080/#/c/19768/2/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test File testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test: http://gerrit.cloudera.org:8080/#/c/19768/2/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test@2242 PS2, Line 2242: 0,1 The SELECT list has 3 columns but the results have only 2. You can run the test locally by impala-py.test tests/query_test/test_queries.py::TestQueries::test_analytic_fns -- To view, visit http://gerrit.cloudera.org:8080/19768 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If6c209b2a64bad37d893ba8b520342bf1f9a7513 Gerrit-Change-Number: 19768 Gerrit-PatchSet: 2 Gerrit-Owner: Minghui Zhu <huihui.cod...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qfc...@hotmail.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Reviewer: Xianqing He <hexianqing...@126.com> Gerrit-Comment-Date: Tue, 30 May 2023 12:23:47 +0000 Gerrit-HasComments: Yes
