GH-34437: [R] Use FetchNode and OrderByNode #34685
Merged
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
nealrichardson
force-pushed
the
order-by-node
branch
2 times, most recently
from
b90834d to
92e841d
Compare
thisisnic
reviewed
Apr 11, 2023
r/R/arrow-info.R
Outdated
Member
There was a problem hiding this comment.
I think you might need a rebase? This was done in #34943 right?
github-actions
bot
added
awaiting changes
thisisnic
approved these changes
Apr 11, 2023
Member
thisisnic
left a comment
thisisnic
left a comment
There was a problem hiding this comment.
Looks great, just needs a rebase!
r/tests/testthat/test-dplyr-query.R
Outdated
Comment on lines
+522
to
+528
github-actions
bot
added
awaiting merge
nealrichardson
force-pushed
the
order-by-node
branch
from
285824a to
2996779
Compare
Member
Author
|
rebased; will merge when green |
|
Benchmark runs are scheduled for baseline = 6b4e0f6 and contender = 47a602d. 47a602d is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
liujiacheng777
pushed a commit
to LoongArch-Python/arrow
that referenced
this pull request
May 11, 2023
### Rationale for this change See also apache#32991. By using the new nodes, we're closer to having all dplyr query business happening inside the ExecPlan. Unfortunately, there are still two cases where we have to apply operations in R after running a query: * apache#34941: Taking head/tail on unordered data, which has non-deterministic results but that should be possible, in the case where the user wants to see a slice of the result, any slice * apache#34942: Implementing tail in the FetchNode or similar would enable removing more hacks and workarounds. Once those are resolved, we can simply further and then move to the new Declaration class. ### What changes are included in this PR? This removes the use of different SinkNodes and many R-specific workarounds to support sorting and head/tail, so *almost* everything we do in a query should be represented in an ExecPlan. ### Are these changes tested? Yes. This is mostly an internal refactor, but behavior changes are accompanied by test updates. ### Are there any user-facing changes? The `show_query()` method will print slightly different ExecPlans. In many cases, they will be more informative. `tail()` now actually returns the tail of the data in cases where the data has an implicit order (currently only in-memory tables). Previously it was non-deterministic (and would return the head or some other slice of the data). When printing query objects that include `summarize()` when the `arrow.summarize.sort = TRUE` option is set, the sorting is correctly printed. It's unclear if there should be changes in performance; running benchmarks would be good but it's also not clear that our benchmarks cover all affected scenarios. * Closes: apache#34437 * Closes: apache#31980 * Closes: apache#31982 Authored-by: Neal Richardson <neal.p.richardson@gmail.com> Signed-off-by: Nic Crane <thisisnic@gmail.com>
liujiacheng777
pushed a commit
to LoongArch-Python/arrow
that referenced
this pull request
May 11, 2023
…d) (apache#35074) This PR updates the decorator in a function introduced in apache#34685 which inadvertently referenced `arrow` and not `acero`. * Closes: apache#35073 Authored-by: Nic Crane <thisisnic@gmail.com> Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>
ArgusLi
pushed a commit
to Bit-Quill/arrow
that referenced
this pull request
May 15, 2023
### Rationale for this change See also apache#32991. By using the new nodes, we're closer to having all dplyr query business happening inside the ExecPlan. Unfortunately, there are still two cases where we have to apply operations in R after running a query: * apache#34941: Taking head/tail on unordered data, which has non-deterministic results but that should be possible, in the case where the user wants to see a slice of the result, any slice * apache#34942: Implementing tail in the FetchNode or similar would enable removing more hacks and workarounds. Once those are resolved, we can simply further and then move to the new Declaration class. ### What changes are included in this PR? This removes the use of different SinkNodes and many R-specific workarounds to support sorting and head/tail, so *almost* everything we do in a query should be represented in an ExecPlan. ### Are these changes tested? Yes. This is mostly an internal refactor, but behavior changes are accompanied by test updates. ### Are there any user-facing changes? The `show_query()` method will print slightly different ExecPlans. In many cases, they will be more informative. `tail()` now actually returns the tail of the data in cases where the data has an implicit order (currently only in-memory tables). Previously it was non-deterministic (and would return the head or some other slice of the data). When printing query objects that include `summarize()` when the `arrow.summarize.sort = TRUE` option is set, the sorting is correctly printed. It's unclear if there should be changes in performance; running benchmarks would be good but it's also not clear that our benchmarks cover all affected scenarios. * Closes: apache#34437 * Closes: apache#31980 * Closes: apache#31982 Authored-by: Neal Richardson <neal.p.richardson@gmail.com> Signed-off-by: Nic Crane <thisisnic@gmail.com>
ArgusLi
pushed a commit
to Bit-Quill/arrow
that referenced
this pull request
May 15, 2023
…d) (apache#35074) This PR updates the decorator in a function introduced in apache#34685 which inadvertently referenced `arrow` and not `acero`. * Closes: apache#35073 Authored-by: Nic Crane <thisisnic@gmail.com> Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Rationale for this change
See also #32991. By using the new nodes, we're closer to having all dplyr query business happening inside the ExecPlan. Unfortunately, there are still two cases where we have to apply operations in R after running a query:
Once those are resolved, we can simply further and then move to the new Declaration class.
What changes are included in this PR?
This removes the use of different SinkNodes and many R-specific workarounds to support sorting and head/tail, so almost
everything we do in a query should be represented in an ExecPlan.
Are these changes tested?
Yes. This is mostly an internal refactor, but behavior changes are accompanied by test updates.
Are there any user-facing changes?
The
show_query()method will print slightly different ExecPlans. In many cases, they will be more informative.tail()now actually returns the tail of the data in cases where the data has an implicit order (currently only in-memory tables). Previously it was non-deterministic (and would return the head or some other slice of the data).When printing query objects that include
summarize()when thearrow.summarize.sort = TRUEoption is set, the sorting is correctly printed.It's unclear if there should be changes in performance; running benchmarks would be good but it's also not clear that our benchmarks cover all affected scenarios.