Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/16219 )
Change subject: IMPALA-9983 : Pushdown limit to analytic sort operator ...................................................................... Patch Set 11: (11 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 The sort info from the outer sort node > nit: remove empty @param annotations? Added the descriptions. http://gerrit.cloudera.org:8080/#/c/16219/10/fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java@422 PS10, Line 422: > I think we should be able to use expression equality, since the partition a Good point about using the substitutedPartitionExprs_. When I used it, it was still not able to do the equivalence comparison. After working through various expr mappings, I realized that it is a little late to use the sortInfo's sortExprs since they have already been substituted. I added a new field in SortInfo to keep the original sort exprs and after substituting those, was able to do the comparison. http://gerrit.cloudera.org:8080/#/c/16219/10/fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java@431 PS10, Line 431: !(pbExpr instanceof SlotRef && so > I wonder if this check is necessary. If the sort order is descending, then This check is needed because the partition-by exprs are always ASC order. http://gerrit.cloudera.org:8080/#/c/16219/10/fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java@481 PS10, Line 481: lhs).getDe > Not sure the rational behind it. I added a comment explaining this. http://gerrit.cloudera.org:8080/#/c/16219/10/fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java@487 PS10, Line 487: get(0)).getFnC > Not follow. Same as above. It has to do with containment within the limit values. http://gerrit.cloudera.org:8080/#/c/16219/6/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java File fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java: http://gerrit.cloudera.org:8080/#/c/16219/6/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java@266 PS6, Line 266: return createSortInfo(input, sortExprs, isAsc, nullsFirst, TSortingOrder.LEXICAL); > I will revert this particular change in the next patch since this is not ne Done 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: // so limit pushdown is not applicable > I think we can generate an analytic without a sort in some edge cases, e.g. Yes, this was a bug. Fixed it. http://gerrit.cloudera.org:8080/#/c/16219/8/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@427 PS8, Line 427: } > We should maybe also avoid going into Subplans? I guess it doesn't really m Added a check for Subplan and actually restricted the tree-walk to only single input operators. http://gerrit.cloudera.org:8080/#/c/16219/8/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@434 PS8, Line 434: root.getChildren().size() > 1) { > This looks like it just goes down the left branch of the plan tree - is tha Yeah, I was thinking of the narrow use case where it goes left deep on single input operators..but yeah this is confusing. I rewrote it and simplified to only allow single child. http://gerrit.cloudera.org:8080/#/c/16219/10/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/10/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@343 PS10, Line 343: offset == 0 > Thought that offset is non-negative in SQL. Done http://gerrit.cloudera.org:8080/#/c/16219/10/fe/src/main/java/org/apache/impala/planner/SortNode.java File fe/src/main/java/org/apache/impala/planner/SortNode.java: http://gerrit.cloudera.org:8080/#/c/16219/10/fe/src/main/java/org/apache/impala/planner/SortNode.java@143 PS10, Line 143: partitioningExprs_ = partitioningExprs; > If this is a TopN sort, then the method should succeed if both the limit an Yeah, in theory one can call convertToTopN on a node that is already TopN ,but for this patch I would prefer to restrict it to a narrower use case. -- 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: 11 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: Fri, 31 Jul 2020 02:13:56 +0000 Gerrit-HasComments: Yes
