Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16219 )
Change subject: IMPALA-9983 : Pushdown limit to analytic sort operator ...................................................................... Patch Set 10: (7 comments) http://gerrit.cloudera.org:8080/#/c/16219/10/fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java File fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java: http://gerrit.cloudera.org:8080/#/c/16219/10/fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java@374 PS10, Line 374: * @param sortInfo nit: remove empty @param annotations? http://gerrit.cloudera.org:8080/#/c/16219/10/fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java@422 PS10, Line 422: if (!((SlotRef) pbExpr).toSql().equals(((SlotRef) sortExpr).toSql())) { I think we should be able to use expression equality, since the partition and sort exprs should be evaluated over the same sort tuple. It looks like the SortInfos are all substituted to point to the sort tuple, but partitionExprs_ are unsubstituted . Maybe if we use substitutedPartitionExprs_ that's enough. http://gerrit.cloudera.org:8080/#/c/16219/8/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/16219/8/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@391 PS8, Line 391: analyticNodeSort = (SortNode) analyticNode.getChild(0); I think we can generate an analytic without a sort in some edge cases, e.g. to avoid IMPALA-8533. I think we just need a bail-out in that case. http://gerrit.cloudera.org:8080/#/c/16219/8/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@427 PS8, Line 427: // further We should maybe also avoid going into Subplans? I guess it doesn't really matter with the way that we're using it. http://gerrit.cloudera.org:8080/#/c/16219/8/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@434 PS8, Line 434: return findDescendantAnalyticNode(child, intermediateNodes); This looks like it just goes down the left branch of the plan tree - is that intended? Seems like an OK limitation, but just checking. http://gerrit.cloudera.org:8080/#/c/16219/10/testdata/workloads/functional-planner/queries/PlannerTest/limit-pushdown-analytic.test File testdata/workloads/functional-planner/queries/PlannerTest/limit-pushdown-analytic.test: http://gerrit.cloudera.org:8080/#/c/16219/10/testdata/workloads/functional-planner/queries/PlannerTest/limit-pushdown-analytic.test@608 PS10, Line 608: partiion nit: partition http://gerrit.cloudera.org:8080/#/c/16219/10/testdata/workloads/functional-planner/queries/PlannerTest/limit-pushdown-analytic.test@642 PS10, Line 642: ==== Can you add a test for a top-level limit without an ORDER BY. Maybe also a flipped rank() comparison to show that the exprs get normalised, e.g. 100 > rnk. -- To view, visit http://gerrit.cloudera.org:8080/16219 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib39f46a7bb75a34466eef7f91ddc25b6e6c99284 Gerrit-Change-Number: 16219 Gerrit-PatchSet: 10 Gerrit-Owner: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: David Rorke <dro...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com> Gerrit-Reviewer: Shant Hovsepian <sh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 29 Jul 2020 18:37:33 +0000 Gerrit-HasComments: Yes
