[SPARK-40813][CONNECT] Add limit and offset to Connect DSL #38275
Conversation
This file contains hidden or 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
Contributor
Author
|
R: @cloud-fan |
Contributor
Author
|
cc @HyukjinKwon @zhengruifeng as I updated the python side accordingly. |
|
Can one of the admins verify this patch? |
HyukjinKwon
reviewed
Oct 17, 2022
Member
There was a problem hiding this comment.
qq why is this Fetch? There are two separate logical plans Limit and Offset.
Contributor
Author
There was a problem hiding this comment.
This is from the original PR which I believe follows SQL semantic. In SQL, offset is something that working with limit. It is limit first then offset.
However in DataFrame this is not the case where offset is an independent operation. I updated PR to decouple this two.
Contributor
grundprinzip
left a comment
grundprinzip
left a comment
There was a problem hiding this comment.
Thanks for doing this. Maybe add some python tests as well? Or do you want to do this in a follow up?
Contributor
Author
There was a problem hiding this comment.
Please see the PR description. TLDR DataFrame does not follow SQL semantic (e.g. df.offset(2) or df.offset(2).limit(10) are both valid).
Contributor
There was a problem hiding this comment.
please use logical.Offset to make sure where it comes from similar to the proto.Offset
Contributor
Author
I will follow up for python. Waiting for some python testing framework improvements to go in first. |
HyukjinKwon
pushed a commit
that referenced
this pull request
Oct 18, 2022
### What changes were proposed in this pull request? The mypy check in master seems to be broken: <img width="676" alt="Screen Shot 2022-10-17 at 1 09 13 PM" src="https://user-images.githubusercontent.com/1938382/196273759-64372862-6caa-4a9e-b700-b665c7ff7e6c.png"> (see it on #38279 and #38275, also reproducible locally). This PR propose to remove the relevant `type: ignore` comments. ### Why are the changes needed? Fix python lint check. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? UT Closes #38287 from amaliujia/test_python_lint. Authored-by: Rui Wang <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>
amaliujia
force-pushed
the
add_limit_offset_to_dsl
branch
2 times, most recently
from
f0922ab to
aceb40f
Compare
amaliujia
force-pushed
the
add_limit_offset_to_dsl
branch
from
aceb40f to
5be9745
Compare
cloud-fan
approved these changes
Oct 19, 2022
HyukjinKwon
approved these changes
Oct 19, 2022
Member
|
Merged to master. |
Contributor
|
Late LGTM, it's easier to follow existing expressions |
This was referenced Oct 19, 2022
HyukjinKwon
pushed a commit
that referenced
this pull request
Oct 21, 2022
… Python client ### What changes were proposed in this pull request? Following up after #38275, improve limit and offset in Python client. ### Why are the changes needed? Improve API coverage. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? UT Closes #38314 from amaliujia/python_test_limit_offset. Authored-by: Rui Wang <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>
SandishKumarHN
pushed a commit
to SandishKumarHN/spark
that referenced
this pull request
Dec 12, 2022
### What changes were proposed in this pull request? The mypy check in master seems to be broken: <img width="676" alt="Screen Shot 2022-10-17 at 1 09 13 PM" src="https://user-images.githubusercontent.com/1938382/196273759-64372862-6caa-4a9e-b700-b665c7ff7e6c.png"> (see it on apache#38279 and apache#38275, also reproducible locally). This PR propose to remove the relevant `type: ignore` comments. ### Why are the changes needed? Fix python lint check. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? UT Closes apache#38287 from amaliujia/test_python_lint. Authored-by: Rui Wang <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>
SandishKumarHN
pushed a commit
to SandishKumarHN/spark
that referenced
this pull request
Dec 12, 2022
### What changes were proposed in this pull request? This PR decouples `LIMIT` and `OFFSET` to two plan which matches with DataFrame semantic where `OFFSET` is an independent operation and can happen before a LIMIT (or any other DataFrame). ### Why are the changes needed? Improve the support for LIMIT and OFFSET. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? UT Closes apache#38275 from amaliujia/add_limit_offset_to_dsl. Authored-by: Rui Wang <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>
SandishKumarHN
pushed a commit
to SandishKumarHN/spark
that referenced
this pull request
Dec 12, 2022
… Python client ### What changes were proposed in this pull request? Following up after apache#38275, improve limit and offset in Python client. ### Why are the changes needed? Improve API coverage. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? UT Closes apache#38314 from amaliujia/python_test_limit_offset. Authored-by: Rui Wang <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
What changes were proposed in this pull request?
This PR decouples
LIMITandOFFSETto two plan which matches with DataFrame semantic whereOFFSETis an independent operation and can happen before a LIMIT (or any other DataFrame).Why are the changes needed?
Improve the support for LIMIT and OFFSET.
Does this PR introduce any user-facing change?
No
How was this patch tested?
UT