Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17615 )
Change subject: IMPALA-10755: Fix migration of analytic predicate to inline view. ...................................................................... Patch Set 1: Code-Review+1 (6 comments) Thanks for fixing this quickly! I can bump my +1 to +2 once it passes CORE tests. Just left some minor comments. http://gerrit.cloudera.org:8080/#/c/17615/1/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/17615/1/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1397 PS1, Line 1397: for (int i = 0; i < conjuncts.size(); ++i) { : Expr pred = conjuncts.get(i); nit: this can be replaced as for (Expr pred : conjuncts) { http://gerrit.cloudera.org:8080/#/c/17615/1/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1402 PS1, Line 1402: slotIds nit: this can be null since we don't need the slotIds. http://gerrit.cloudera.org:8080/#/c/17615/1/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1408 PS1, Line 1408: if (viewPred.referencesTuple(analyticTuple.getId()) && tupleIds.size() <= 1) { nit: Can we simply use isBound()? i.e. viewPred.isBound(analyticTuple.getId()) http://gerrit.cloudera.org:8080/#/c/17615/1/testdata/workloads/tpch/queries/analytic-fns.test File testdata/workloads/tpch/queries/analytic-fns.test: http://gerrit.cloudera.org:8080/#/c/17615/1/testdata/workloads/tpch/queries/analytic-fns.test@23 PS1, Line 23: ORDER BY 1, 2, 3, 4 nit: we don't need the ORDER BY since the RESULTS section is not specified with VERIFY_IS_EQUAL_SORTED http://gerrit.cloudera.org:8080/#/c/17615/1/testdata/workloads/tpch/queries/analytic-fns.test@52 PS1, Line 52: avg_nr_pvp nit: could you cast this to BIGINT? Just concerning if this will be flaky since it's comparing DOUBLE with BIGINT. cast(avg_nr_pvp as bigint) = max_nr_pvp - 5 http://gerrit.cloudera.org:8080/#/c/17615/1/testdata/workloads/tpch/queries/analytic-fns.test@53 PS1, Line 53: ORDER BY 1, 2, 3, 4, 5 nit: we don't need the ORDER BY since the RESULTS section is not specified with VERIFY_IS_EQUAL_SORTED -- To view, visit http://gerrit.cloudera.org:8080/17615 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib5cad3d408ee3695cafb35f66a4f19b4e8d0529e Gerrit-Change-Number: 17615 Gerrit-PatchSet: 1 Gerrit-Owner: Aman Sinha <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Wed, 23 Jun 2021 06:52:57 +0000 Gerrit-HasComments: Yes
