Aman Sinha 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 2: (7 comments) > Patch Set 1: > > (1 comment) Thanks Quanlong for the quick review. Pls see responses. http://gerrit.cloudera.org:8080/#/c/17615/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17615/1//COMMIT_MSG@20 PS1, Line 20: Testing: > TODO: run all FE and relevant end-to-end tests. Done 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: Expr viewPred = pred.substitute(inlineViewRef.getSmap(), analyzer, false); : tupleIds.clear(); > nit: this can be replaced as Done http://gerrit.cloudera.org:8080/#/c/17615/1/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1402 PS1, Line 1402: ere MAX > nit: this can be null since we don't need the slotIds. Done http://gerrit.cloudera.org:8080/#/c/17615/1/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1408 PS1, Line 1408: } > nit: Can we simply use isBound()? i.e. I did not change this because after looking at the implementation of SlotRef.isBoundByTupleIds() and SlotRef.referencesTuple(). I see there's an extra Preconditions check in referencesTuple: Preconditions.checkState(type_.isValid()); I think it is useful to do that check, so I left this as-is. Let me know if you think otherwise. 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: ---- RESULTS > nit: we don't need the ORDER BY since the RESULTS section is not specified Makes sense. Done. http://gerrit.cloudera.org:8080/#/c/17615/1/testdata/workloads/tpch/queries/analytic-fns.test@52 PS1, Line 52: ULTS > nit: could you cast this to BIGINT? Just concerning if this will be flaky s Good point. Added the cast. http://gerrit.cloudera.org:8080/#/c/17615/1/testdata/workloads/tpch/queries/analytic-fns.test@53 PS1, Line 53: 1.00,1,11,7,12 > nit: we don't need the ORDER BY since the RESULTS section is not specified Done -- 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: 2 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 23:18:19 +0000 Gerrit-HasComments: Yes
