You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "nealrichardson (via GitHub)" <gi...@apache.org> on 2024/11/05 21:39:31 UTC
[PR] GH-29642: [R] Support for .keep_all = TRUE with distinct() [arrow]
nealrichardson opened a new pull request, #44652:
URL: https://github.com/apache/arrow/pull/44652
### 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 more
### Are there any user-facing changes?
Yes indeed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-29642: [R] Support for .keep_all = TRUE with distinct() [arrow]
Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane commented on code in PR #44652:
URL: https://github.com/apache/arrow/pull/44652#discussion_r1835693557
##########
r/tests/testthat/test-dplyr-distinct.R:
##########
@@ -115,12 +103,57 @@ test_that("across() works in distinct()", {
})
test_that("distinct() can return all columns", {
- skip("ARROW-14045")
- compare_dplyr_binding(
- .input %>%
- distinct(lgl, .keep_all = TRUE) %>%
- collect() %>%
- arrange(int),
- tbl
- )
+ # hash_one prefers to keep non-null values, which is different from .keep_all in dplyr
+ # so we can't compare the result directly
+ expected <- tbl %>%
+ # Drop factor because of #44661:
+ # NotImplemented: Function 'hash_one' has no kernel matching input types
+ # (dictionary<values=string, indices=int8, ordered=0>, uint8)
Review 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
##########
r/R/dplyr-distinct.R:
##########
@@ -33,11 +27,28 @@ distinct.arrow_dplyr_query <- function(.data, ..., .keep_all = FALSE) {
.data <- dplyr::group_by(.data, !!!syms(names(.data)))
}
- out <- dplyr::summarize(.data, .groups = "drop")
+ if (isTRUE(.keep_all)) {
+ # 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.
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-29642: [R] Support for .keep_all = TRUE with distinct() [arrow]
Posted by "rkrug (via GitHub)" <gi...@apache.org>.
rkrug commented on PR #44652:
URL: https://github.com/apache/arrow/pull/44652#issuecomment-2514616172
Thanks. So it will be in the next release?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-29642: [R] Support for .keep_all = TRUE with distinct() [arrow]
Posted by "rkrug (via GitHub)" <gi...@apache.org>.
rkrug commented on PR #44652:
URL: https://github.com/apache/arrow/pull/44652#issuecomment-2514509390
Is there any chance to get it merged? I would very much need to use it in my code!
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-29642: [R] Support for .keep_all = TRUE with distinct() [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #44652:
URL: https://github.com/apache/arrow/pull/44652#issuecomment-2458201399
:warning: GitHub issue #29642 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-29642: [R] Support for .keep_all = TRUE with distinct() [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #44652:
URL: https://github.com/apache/arrow/pull/44652#issuecomment-2525292509
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 1b3caf6b232b7855956d3ec45ee95ede0492e78f.
There were 132 benchmark results with an error:
- Commit Run on `arm64-t4g-2xlarge-linux` at [2024-12-07 16:34:43Z](https://conbench.ursa.dev/compare/runs/444ea684b27b490080b1404f01d9b7df...001f4e6f30794636b14a38ad4c080ab5/)
- [`tpch` (R) with engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-07, scale_factor=1](https://conbench.ursa.dev/benchmark-results/0675487349d7704680002486d8a8180b)
- [`tpch` (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-09, scale_factor=1](https://conbench.ursa.dev/benchmark-results/06754875c141764a80001151abd5f0ae)
- and 130 more (see the report linked below)
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/34077875571) has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-29642: [R] Support for .keep_all = TRUE with distinct() [arrow]
Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane merged PR #44652:
URL: https://github.com/apache/arrow/pull/44652
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-29642: [R] Support for .keep_all = TRUE with distinct() [arrow]
Posted by "nealrichardson (via GitHub)" <gi...@apache.org>.
nealrichardson commented on code in PR #44652:
URL: https://github.com/apache/arrow/pull/44652#discussion_r1843769552
##########
r/R/dplyr-distinct.R:
##########
@@ -33,11 +27,28 @@ distinct.arrow_dplyr_query <- function(.data, ..., .keep_all = FALSE) {
.data <- dplyr::group_by(.data, !!!syms(names(.data)))
}
- out <- dplyr::summarize(.data, .groups = "drop")
+ if (isTRUE(.keep_all)) {
+ # 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.
Review Comment:
It is documented on the acero man page, that's the change to arrow-package.R. I'd rather not one-time warning; that's a slippery slope if we were going to be chatty about every subtle difference between how Acero works from dplyr on data.frames.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-29642: [R] Support for .keep_all = TRUE with distinct() [arrow]
Posted by "nealrichardson (via GitHub)" <gi...@apache.org>.
nealrichardson commented on code in PR #44652:
URL: https://github.com/apache/arrow/pull/44652#discussion_r1843767129
##########
r/tests/testthat/test-dplyr-distinct.R:
##########
@@ -115,12 +103,57 @@ test_that("across() works in distinct()", {
})
test_that("distinct() can return all columns", {
- skip("ARROW-14045")
- compare_dplyr_binding(
- .input %>%
- distinct(lgl, .keep_all = TRUE) %>%
- collect() %>%
- arrange(int),
- tbl
- )
+ # hash_one prefers to keep non-null values, which is different from .keep_all in dplyr
+ # so we can't compare the result directly
+ expected <- tbl %>%
+ # Drop factor because of #44661:
+ # NotImplemented: Function 'hash_one' has no kernel matching input types
+ # (dictionary<values=string, indices=int8, ordered=0>, uint8)
Review Comment:
Yeah that's the error message. I'd have to think about how/where best to catch that and translate that to R-speak. As it turns out, dictionary isn't the only unsupported type, it's just the only one we have in this test data frame. I think list types and other non-simple types are also not supported, IIRC from RTFS.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-29642: [R] Support for .keep_all = TRUE with distinct() [arrow]
Posted by "nealrichardson (via GitHub)" <gi...@apache.org>.
nealrichardson commented on code in PR #44652:
URL: https://github.com/apache/arrow/pull/44652#discussion_r1843817643
##########
r/tests/testthat/test-dplyr-distinct.R:
##########
@@ -115,12 +103,57 @@ test_that("across() works in distinct()", {
})
test_that("distinct() can return all columns", {
- skip("ARROW-14045")
- compare_dplyr_binding(
- .input %>%
- distinct(lgl, .keep_all = TRUE) %>%
- collect() %>%
- arrange(int),
- tbl
- )
+ # hash_one prefers to keep non-null values, which is different from .keep_all in dplyr
+ # so we can't compare the result directly
+ expected <- tbl %>%
+ # Drop factor because of #44661:
+ # NotImplemented: Function 'hash_one' has no kernel matching input types
+ # (dictionary<values=string, indices=int8, ordered=0>, uint8)
Review Comment:
https://github.com/apache/arrow/issues/44740
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-29642: [R] Support for .keep_all = TRUE with distinct() [arrow]
Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane commented on PR #44652:
URL: https://github.com/apache/arrow/pull/44652#issuecomment-2525198556
It missed the latest release, but once it merges it'll be in the following one. Alternatively, you could use nightly builds starting the day after it merges.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org