Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16663 )
Change subject: IMPALA-10295: fix analytic limit pushdown with no predicates ...................................................................... Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/16663/2/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/16663/2/fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java@478 PS2, Line 478: if (!analyticSortExprs.get(i).equals(sortExprs.get(i))) return false; > Will this comparison return True if for example both the analytic sort expr It returns true in some circumstances. That particular example didn't consider them equal because we materialized the order by expression into a slot of the sort tuple, and one of the exprs referenced that slot and the other referenced the original slot. So this is now slightly more general than the previous variant, which only handled bare SlotRefs, but it doesn't handle all cases where the optimisation might be valid. http://gerrit.cloudera.org:8080/#/c/16663/2/fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java@480 PS2, Line 480: if (!sortInfo.getIsAscOrder().get(i).equals(analyticSortInfo.getIsAscOrder().get(i))) { > line too long (93 > 90) Done http://gerrit.cloudera.org:8080/#/c/16663/2/fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java@483 PS2, Line 483: if (!sortInfo.getNullsFirst().get(i).equals(analyticSortInfo.getNullsFirst().get(i))) { > line too long (93 > 90) Done http://gerrit.cloudera.org:8080/#/c/16663/2/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/16663/2/testdata/workloads/functional-planner/queries/PlannerTest/limit-pushdown-analytic.test@213 PS2, Line 213: # Limit pushdown should be applied > Change this comment according to the new behavior. Done -- To view, visit http://gerrit.cloudera.org:8080/16663 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I254e85edd5ea6b6e76d20cbdf27fd88059a98a21 Gerrit-Change-Number: 16663 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 29 Oct 2020 17:53:58 +0000 Gerrit-HasComments: Yes
