GH-29642: [R] Support for .keep_all = TRUE with distinct() #44652
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
|
|
jonkeane
reviewed
Nov 10, 2024
Comment on lines
+109
to
+111
| # Drop factor because of #44661: | ||
| # NotImplemented: Function 'hash_one' has no kernel matching input types | ||
| # (dictionary<values=string, indices=int8, ordered=0>, uint8) |
There was a problem hiding this comment.
Is 110-111 the error that someone would get if they tried distinct(..., .keep_all = TRUE) with a factor in the table/data.frame?
We might want to make that a bit nicer / more grokable for folks who might not have the dictionary -> factor knowledge top of mind
Comment on lines
+31
to
+33
| # Note: in regular dplyr, `.keep_all = TRUE` returns the first row's value. | ||
| # However, Acero's `hash_one` function prefers returning non-null values. | ||
| # So, you'll get the same shape of data, but the values may differ. |
There was a problem hiding this comment.
This behavior change is probably either not-impactful, or if folks are relying on it, that is actually a bug in their code. Though it does seem like something we should mention (in docs at least?).
Or maybe with a one-time warning?
github-actions
bot
added
awaiting changes
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.
Rationale for this change
Support a missing feature, just wiring up some stuff from R to Acero, then adding docs and tests.
This is mostly picking up where #13934 started and finishing it out. Thanks @mopcup for the initial lift.
What changes are included in this PR?
An aggregation binding, some symbol manipulation, and tests. I also cleaned up some dplyr test shims from 2022.
Are these changes tested?
Yes, though if anyone knows of odd corners in
distinct()that aren't covered by this, we can add moreAre there any user-facing changes?
Yes indeed.