-
Notifications
You must be signed in to change notification settings - Fork 177
Support push down sort after limit #4657
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
3f612f2
65c1bfd
427e476
04ba056
958fc5a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| calcite: | ||
| logical: | | ||
| LogicalSystemLimit(sort0=[$0], dir0=[ASC-nulls-first], fetch=[10000], type=[QUERY_SIZE_LIMIT]) | ||
| LogicalProject(age=[$8]) | ||
| LogicalSort(sort0=[$8], dir0=[ASC-nulls-first]) | ||
| LogicalSort(sort0=[$3], dir0=[ASC-nulls-first], fetch=[5]) | ||
| CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]]) | ||
| physical: | | ||
| EnumerableLimit(fetch=[10000]) | ||
| EnumerableCalc(expr#0..1=[{inputs}], age=[$t1]) | ||
| EnumerableSort(sort0=[$1], dir0=[ASC-nulls-first]) | ||
| CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]], PushDownContext=[[PROJECT->[balance, age], SORT->[{ | ||
| "balance" : { | ||
| "order" : "asc", | ||
| "missing" : "_first" | ||
| } | ||
| }], LIMIT->5], OpenSearchRequestBuilder(sourceBuilder={"from":0,"size":5,"timeout":"1m","_source":{"includes":["balance","age"],"excludes":[]},"sort":[{"balance":{"order":"asc","missing":"_first"}}]}, requestedTotalSize=5, pageSize=null, startFrom=0)]) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,11 +48,12 @@ public interface Config extends RelRule.Config { | |
| .oneInput( | ||
| b1 -> | ||
| b1.operand(AbstractCalciteIndexScan.class) | ||
| // Skip the rule if a limit has already been pushed down | ||
| // because pushing down a sort after a limit will be treated | ||
| // as sort-then-limit by OpenSearch DSL. | ||
| // Skip the rule if Top-K(i.e. sort + limit) has already been | ||
| // pushed down. Otherwise, | ||
| // Continue to push down sort although limit has already been | ||
| // pushed down since we don't promise collation with only limit. | ||
| .predicate( | ||
| Predicate.not(AbstractCalciteIndexScan::isLimitPushed) | ||
| Predicate.not(AbstractCalciteIndexScan::isTopKPushed) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we add a case for below ppl? The Cannot for following ppl either. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both cases cannot push the sort and it's not related to whether there is limit in the PPL query. Their no push down reason are:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could you add these cases in explain IT:
|
||
| .and( | ||
| Predicate.not( | ||
| AbstractCalciteIndexScan | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Better to check NoPushDownIT case. We may need different branch for these two test cases