impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Qifan Chen (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-9983 : Pushdown limit to analytic sort operator
Date Wed, 29 Jul 2020 16:52:32 GMT
Qifan Chen 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)

Looks good to me!

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@406
PS10, Line 406: partitionExprs_.size() > sortExprs.size()
Will this be a possibility?

sort on a
partition on (a,1)


http://gerrit.cloudera.org:8080/#/c/16219/10/fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java@416
PS10, Line 416: SlotRef
Not clear on this. I thought SlotRef is useful when dealing with run-time property of an expression.


http://gerrit.cloudera.org:8080/#/c/16219/10/fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java@431
PS10, Line 431: !sortInfo.getIsAscOrder().get(i) 
I wonder if this check is necessary. If the sort order is descending, then if the same order
is applied to the split version of the sort instances, push-down LIMIT still works.

Assume the pushdown works by asking the child sort operators to produce at least <n>
rows. where <n> is the limit at the query level.


http://gerrit.cloudera.org:8080/#/c/16219/10/fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java@481
PS10, Line 481: r =, <, <= 
Not sure the rational behind it.


http://gerrit.cloudera.org:8080/#/c/16219/10/fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java@487
PS10, Line 487: rhs of the predic
Not follow.


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.


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: Preconditions.checkArgument(type_ == TSortType.TOTAL);
If this is a TopN sort, then the method should succeed if both the limit and partitioningExprs
in args and contained within this are the same.



-- 
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 <amsinha@cloudera.com>
Gerrit-Reviewer: Aman Sinha <amsinha@cloudera.com>
Gerrit-Reviewer: David Rorke <drorke@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenkins@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qchen@cloudera.com>
Gerrit-Reviewer: Shant Hovsepian <shant@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Comment-Date: Wed, 29 Jul 2020 16:52:32 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message