You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/09/17 15:29:32 UTC
[GitHub] [arrow] eitsupi opened a new pull request, #14160: ARROW-17738: [R] dplyr::compute does not work for grouped arrow dplyr query
eitsupi opened a new pull request, #14160:
URL: https://github.com/apache/arrow/pull/14160
The name of the test file was changed (test-dplyr-collapse.R -> test-dplyr-collect.R) because it did not match the file in the package (dplyr-collect.R).
--
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
[GitHub] [arrow] nealrichardson commented on a diff in pull request #14160: ARROW-17738: [R] dplyr::compute should convert from grouped arrow_dplyr_query to arrow Table
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on code in PR #14160:
URL: https://github.com/apache/arrow/pull/14160#discussion_r975171976
##########
r/tests/testthat/test-dplyr-collect.R:
##########
@@ -276,3 +276,16 @@ test_that("collapse doesn't unnecessarily add ProjectNodes", {
)
expect_length(grep("ProjectNode", plan), 1)
})
+
+test_that("compute", {
Review Comment:
I would put this test in `test-dplyr-query.R` and rename this file back to `test-dplyr-collapse.R` since it's all about `collapse()`. I get the point of trying to have the R and test file names matching, but it's not so straightforward here--nearly every dplyr test we have calls `collect()` so we're testing it everywhere.
Also let's give the test a more expressive name
```suggestion
test_that("compute() on a grouped query returns a Table with groups in metadata", {
```
##########
r/tests/testthat/test-dplyr-query.R:
##########
@@ -119,7 +119,7 @@ test_that("collect(as_data_frame=FALSE)", {
filter(int > 5) %>%
group_by(int) %>%
collect(as_data_frame = FALSE)
- expect_s3_class(b4, "arrow_dplyr_query")
Review Comment:
I think we need a test (and matching code change) that a Table with groups in the `$metadata` prints that it is grouped. Currently, the print method does not reveal that you have groupings, even though they get restored when you `as.data.frame()`:
```
> tab <- arrow_table(mtcars)
> tab$metadata$r$attributes$.group_vars <- "cyl"
> tab
Table
32 rows x 11 columns
$mpg <double>
$cyl <double>
$disp <double>
$hp <double>
$drat <double>
$wt <double>
$qsec <double>
$vs <double>
$am <double>
$gear <double>
$carb <double>
See $metadata for additional Schema metadata
```
I wonder what else is lost or concealed by this change. This feature of reading groups from `$metadata$r$attributes$.group_vars` was a bit of a hack and not something we're generally relying on, so I don't think it's a simple change. Or at least, it's not something we can just rely that the test suite already covers everything we care about. (Search for ".group_vars" in the code, it's not touched in many places, and only in one test.)
Not a reason not to do it, just need to be thorough.
--
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
[GitHub] [arrow] ursabot commented on pull request #14160: ARROW-17738: [R] dplyr::compute should convert from grouped arrow_dplyr_query to arrow Table
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #14160:
URL: https://github.com/apache/arrow/pull/14160#issuecomment-1272147591
Benchmark runs are scheduled for baseline = 45a008d7105bc0931dc56648ded8b388fa470800 and contender = 12667cd06046070c397ea4b90193f3f5ec3fa997. 12667cd06046070c397ea4b90193f3f5ec3fa997 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/8cb305d583d94752936fb22573dbc198...55c1ecb3c5b846e9a449b167a9f4e720/)
[Failed :arrow_down:0.0% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/9b15c56c0d7b49488a15f4694a7fd320...d8409db5288047b69d5d0e2a54a561ca/)
[Failed :arrow_down:1.1% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/5dacb31cf02b44caaa45a1e7c9dd78eb...2faaf41a88074756ac171b554becf499/)
[Finished :arrow_down:0.36% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/64b6ea1adb8c43d78c2754f16c244759...50b4a2eaf069478d80d0be3cbc3d1763/)
Buildkite builds:
[Finished] [`12667cd0` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1645)
[Failed] [`12667cd0` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1664)
[Failed] [`12667cd0` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1647)
[Finished] [`12667cd0` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1658)
[Finished] [`45a008d7` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1644)
[Failed] [`45a008d7` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1663)
[Failed] [`45a008d7` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1646)
[Finished] [`45a008d7` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1657)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #14160: ARROW-17738: [R] dplyr::compute does not work for grouped arrow dplyr query
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14160:
URL: https://github.com/apache/arrow/pull/14160#issuecomment-1250091117
:warning: Ticket **has not been started in JIRA**, please click 'Start Progress'.
--
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
[GitHub] [arrow] eitsupi commented on pull request #14160: ARROW-17738: [R] dplyr::compute should convert from grouped arrow_dplyr_query to arrow Table
Posted by GitBox <gi...@apache.org>.
eitsupi commented on PR #14160:
URL: https://github.com/apache/arrow/pull/14160#issuecomment-1272081263
Thanks for merging!
--
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
[GitHub] [arrow] eitsupi commented on a diff in pull request #14160: ARROW-17738: [R] dplyr::compute should convert from grouped arrow_dplyr_query to arrow Table
Posted by GitBox <gi...@apache.org>.
eitsupi commented on code in PR #14160:
URL: https://github.com/apache/arrow/pull/14160#discussion_r975199513
##########
r/tests/testthat/test-dplyr-query.R:
##########
@@ -119,7 +119,7 @@ test_that("collect(as_data_frame=FALSE)", {
filter(int > 5) %>%
group_by(int) %>%
collect(as_data_frame = FALSE)
- expect_s3_class(b4, "arrow_dplyr_query")
Review Comment:
Thanks for your review.
> I think we need a test (and matching code change) that a Table with groups in the `$metadata` prints that it is grouped. Currently, the print method does not reveal that you have groupings, even though they get restored when you `as.data.frame()`:
I agree that the print results need improvement.
But would it be better to create a separate ticket and do it there?
This occurs even in arrow 9.0.0, regardless of the behavior with `compute`.
``` r
mtcars |> dplyr::group_by(cyl) |> arrow::arrow_table()
#> Table
#> 32 rows x 11 columns
#> $mpg <double>
#> $cyl <double>
#> $disp <double>
#> $hp <double>
#> $drat <double>
#> $wt <double>
#> $qsec <double>
#> $vs <double>
#> $am <double>
#> $gear <double>
#> $carb <double>
#>
#> See $metadata for additional Schema metadata
```
<sup>Created on 2022-09-20 with [reprex v2.0.2](https://reprex.tidyverse.org)</sup>
> This feature of reading groups from `$metadata$r$attributes$.group_vars` was a bit of a hack and not something we're generally relying on, so I don't think it's a simple change. Or at least, it's not something we can just rely that the test suite already covers everything we care about.
Since `dplyr::group_vars` reads this field, isn't this function generally used?
--
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
[GitHub] [arrow] eitsupi commented on pull request #14160: ARROW-17738: [R] dplyr::compute should convert from grouped arrow_dplyr_query to arrow Table
Posted by GitBox <gi...@apache.org>.
eitsupi commented on PR #14160:
URL: https://github.com/apache/arrow/pull/14160#issuecomment-1269818990
> In that case, I think the change that is needed is a `test_that("collect() is identical to compute() %>% collect()", {})` with grouped and ungrouped Tables and Datasets. Does that sound reasonable?
Thank you for your suggestion. I have added tests.
Is this also necessary for datasets?
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #14160: ARROW-17738: [R] dplyr::compute does not work for grouped arrow dplyr query
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14160:
URL: https://github.com/apache/arrow/pull/14160#issuecomment-1250091107
https://issues.apache.org/jira/browse/ARROW-17738
--
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
[GitHub] [arrow] paleolimbot merged pull request #14160: ARROW-17738: [R] dplyr::compute should convert from grouped arrow_dplyr_query to arrow Table
Posted by GitBox <gi...@apache.org>.
paleolimbot merged PR #14160:
URL: https://github.com/apache/arrow/pull/14160
--
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