Support limit push down for offset_plan #2566
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Ted-Jiang
commented
May 18, 2022
|
|
||
| self.limit(plan, query.limit) | ||
| //make limit as offset's input will enable limit push down simply | ||
| self.offset(plan, query.offset) |
There was a problem hiding this comment.
If there is an offset in sql, we will put it as root node in plan tree. So it will easily push new_limit to limit operator.
There was a problem hiding this comment.
Don't we need to apply the offset before the limit here?
It looks like we don't currently have a test with both a non-zero offset and a limit. Could you add one here so we can see what the plan looks like?
Per https://www.postgresql.org/docs/current/queries-limit.html If both OFFSET and LIMIT appear, then OFFSET rows are skipped before starting to count the LIMIT rows that are returned. and I am not sure this change will ensure that
There was a problem hiding this comment.
form the explain from pg
postgres=# explain analyze select * from users limit 5 offset 7;
QUERY PLAN
-------------------------------------------------------------------------------------------------------------
Limit (cost=0.17..0.29 rows=5 width=80) (actual time=0.117..0.226 rows=5 loops=1)
-> Seq Scan on users (cost=0.00..239.02 rows=10002 width=80) (actual time=0.013..0.082 rows=12 loops=1)
Planning Time: 0.060 ms
Execution Time: 0.350 ms
(4 rows)
I think in PG, the limit operator has a param with offset， in DF we separate it. So we need apply offset after limit a+b
add test #2566 (comment)
andygrove
reviewed
May 18, 2022
andygrove
reviewed
May 18, 2022
andygrove
reviewed
May 18, 2022
Co-authored-by: Andy Grove <andygrove73@gmail.com>
Ted-Jiang
commented
May 20, 2022
Comment on lines
+4864
to
+4886
| #[test] | ||
| fn test_offset_after_limit_with_limit_push() { | ||
| let sql = "select id from person where person.id > 100 LIMIT 5 OFFSET 3;"; | ||
| let expected = "Offset: 3\ | ||
| \n Limit: 8\ | ||
| \n Projection: #person.id\ | ||
| \n Filter: #person.id > Int64(100)\ | ||
| \n TableScan: person projection=None"; | ||
|
|
||
| quick_test_with_limit_pushdown(sql, expected); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_offset_before_limit_with_limit_push() { | ||
| let sql = "select id from person where person.id > 100 OFFSET 3 LIMIT 5;"; | ||
| let expected = "Offset: 3\ | ||
| \n Limit: 8\ | ||
| \n Projection: #person.id\ | ||
| \n Filter: #person.id > Int64(100)\ | ||
| \n TableScan: person projection=None"; | ||
| quick_test_with_limit_pushdown(sql, expected); | ||
| } | ||
|
|
There was a problem hiding this comment.
Thanks! That makes it much clearer
andygrove
approved these changes
May 20, 2022
gandronchik
pushed a commit
to cube-js/arrow-datafusion
that referenced
this pull request
Aug 30, 2022
* support offset push down * change the limit and offset order * add test for join and subquery * fmt * Apply suggestions from code review Co-authored-by: Andy Grove <andygrove73@gmail.com> * add sql test in planner.rs * add sql test in planner.rs * fix clippy Co-authored-by: Andy Grove <andygrove73@gmail.com>
gandronchik
pushed a commit
to cube-js/arrow-datafusion
that referenced
this pull request
Aug 31, 2022
* support offset push down * change the limit and offset order * add test for join and subquery * fmt * Apply suggestions from code review Co-authored-by: Andy Grove <andygrove73@gmail.com> * add sql test in planner.rs * add sql test in planner.rs * fix clippy Co-authored-by: Andy Grove <andygrove73@gmail.com>
gandronchik
pushed a commit
to cube-js/arrow-datafusion
that referenced
this pull request
Sep 2, 2022
* support offset push down * change the limit and offset order * add test for join and subquery * fmt * Apply suggestions from code review Co-authored-by: Andy Grove <andygrove73@gmail.com> * add sql test in planner.rs * add sql test in planner.rs * fix clippy Co-authored-by: Andy Grove <andygrove73@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #2550.
like in pg
if we have
limit a + offset bwill push limita+bto tableScanRationale for this change
What changes are included in this PR?
Are there any user-facing changes?