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 2021/11/04 19:38:56 UTC
[GitHub] [arrow] paleolimbot opened a new pull request #11612: ARROW-14051 [R] Handle conditionals enclosing aggregate expressions
paleolimbot opened a new pull request #11612:
URL: https://github.com/apache/arrow/pull/11612
This PR more correctly passes on type information for the temporary columns that are created when nested expressions exist. This isn't needed often, but occasionally the expression type is checked for the purposes of erroring or warning. The two examples where this happens are `ifelse()` and `case_when()`, which resulted in the following (valid) code pulling data into R with a confusing message:
``` r
library(arrow, warn.conflicts = FALSE)
library(dplyr, warn.conflicts = FALSE)
# motvating example
RecordBatch$create(x = c(0, 1, 1), y = c(2, 3, 5), z = c(8, 13, 21)) %>%
group_by(x) %>%
summarise(r = ifelse(n() > 1, mean(y), mean(z))) %>%
collect()
#> Warning: Error : Expression ifelse(..temp0 > 1, ..temp1, ..temp2) not supported
#> in Arrow; pulling data into R
#> # A tibble: 2 × 2
#> x r
#> <dbl> <dbl>
#> 1 0 8
#> 2 1 4
```
After this PR, the above works without warning:
``` r
library(arrow, warn.conflicts = FALSE)
library(dplyr, warn.conflicts = FALSE)
# motvating example
RecordBatch$create(x = c(0, 1, 1), y = c(2, 3, 5), z = c(8, 13, 21)) %>%
group_by(x) %>%
summarise(r = ifelse(n() > 1, mean(y), mean(z))) %>%
collect()
#> # A tibble: 2 × 2
#> x r
#> <dbl> <dbl>
#> 1 0 8
#> 2 1 4
```
I imagine I'm missing some of the complexity here...feel free to let me know!
--
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 edited a comment on pull request #11612: ARROW-14051 [R] Handle conditionals enclosing aggregate expressions
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11612:
URL: https://github.com/apache/arrow/pull/11612#issuecomment-964195863
Benchmark runs are scheduled for baseline = caf1e1ed459e2c6ff66255fc915a5a7b16c0f5c5 and contender = 5c936560c1da003baf714d67dc92f25670730c84. 5c936560c1da003baf714d67dc92f25670730c84 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/a32ec6a017f3475aba398a35a9248240...7e2a31fb4409442e93a77dfaabe7c39a/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/1a88c8de5ec34d129f7b198542286a27...b7bc0b0371a2428b81e2b97bae35ae97/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/d76c3bf06b9b4a4b834b153f2fef5c1a...910dfdee488846d6ab422b1f5df8e9f6/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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 commented on a change in pull request #11612: ARROW-14051 [R] Handle conditionals enclosing aggregate expressions
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #11612:
URL: https://github.com/apache/arrow/pull/11612#discussion_r744636509
##########
File path: r/tests/testthat/test-dplyr-summarize.R
##########
@@ -879,3 +879,20 @@ test_that("summarize() handles group_by .drop", {
)
)
})
+
+test_that("summarise() passes through type information for temporary columns", {
+ # applies to ifelse and case_when(), in which argument types are checked
+ # within a translated function (previously this failed because the appropriate
+ # schema was not available for n() > 1, mean(y), and mean(z))
+ compare_dplyr_binding(
+ .input %>%
+ group_by(x) %>%
+ summarise(r = ifelse(n() > 1, mean(y), mean(z))) %>%
+ collect(),
+ tibble(
+ x = c(0, 1, 1),
+ y = c(2, 3, 5),
+ z = c(8, 13, 21)
+ )
+ )
Review comment:
Generating better errors for aggregate expressions that aren't supported is already a ticket (ARROW-14586)...I'd be happy to add that example!
--
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] jonkeane commented on a change in pull request #11612: ARROW-14051 [R] Handle conditionals enclosing aggregate expressions
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #11612:
URL: https://github.com/apache/arrow/pull/11612#discussion_r745659614
##########
File path: r/tests/testthat/test-dplyr-summarize.R
##########
@@ -879,3 +879,20 @@ test_that("summarize() handles group_by .drop", {
)
)
})
+
+test_that("summarise() passes through type information for temporary columns", {
+ # applies to ifelse and case_when(), in which argument types are checked
+ # within a translated function (previously this failed because the appropriate
+ # schema was not available for n() > 1, mean(y), and mean(z))
+ compare_dplyr_binding(
+ .input %>%
+ group_by(x) %>%
+ summarise(r = ifelse(n() > 1, mean(y), mean(z))) %>%
+ collect(),
+ tibble(
+ x = c(0, 1, 1),
+ y = c(2, 3, 5),
+ z = c(8, 13, 21)
+ )
+ )
Review comment:
Ah, fantastic!
--
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] jonkeane commented on a change in pull request #11612: ARROW-14051 [R] Handle conditionals enclosing aggregate expressions
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #11612:
URL: https://github.com/apache/arrow/pull/11612#discussion_r745147033
##########
File path: r/R/dplyr-summarize.R
##########
@@ -218,8 +218,16 @@ summarize_eval <- function(name, quosure, ctx, hash, recurse = FALSE) {
} else if (all(inner_agg_exprs)) {
# Something like: fun(agg(x), agg(y))
# So based on the aggregations that have been extracted, mutate after
+ agg_field_refs <- make_field_refs(names(ctx$aggregations))
+ agg_field_types <- lapply(ctx$aggregations, function(x) x$data$type())
Review comment:
Ah, ok that's helpful. I was a bit hopeful that it was one of our R6 objects that might already have (or we could easily extend to have) a `$type()` method, but it looks like not.
--
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 #11612: ARROW-14051 [R] Handle conditionals enclosing aggregate expressions
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11612:
URL: https://github.com/apache/arrow/pull/11612#issuecomment-961358716
--
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] jonkeane closed pull request #11612: ARROW-14051 [R] Handle conditionals enclosing aggregate expressions
Posted by GitBox <gi...@apache.org>.
jonkeane closed pull request #11612:
URL: https://github.com/apache/arrow/pull/11612
--
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 #11612: ARROW-14051 [R] Handle conditionals enclosing aggregate expressions
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11612:
URL: https://github.com/apache/arrow/pull/11612#issuecomment-961358716
--
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 commented on a change in pull request #11612: ARROW-14051 [R] Handle conditionals enclosing aggregate expressions
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #11612:
URL: https://github.com/apache/arrow/pull/11612#discussion_r744631444
##########
File path: r/tests/testthat/test-dplyr-summarize.R
##########
@@ -879,3 +879,20 @@ test_that("summarize() handles group_by .drop", {
)
)
})
+
+test_that("summarise() passes through type information for temporary columns", {
+ # applies to ifelse and case_when(), in which argument types are checked
+ # within a translated function (previously this failed because the appropriate
+ # schema was not available for n() > 1, mean(y), and mean(z))
+ compare_dplyr_binding(
+ .input %>%
+ group_by(x) %>%
+ summarise(r = ifelse(n() > 1, mean(y), mean(z))) %>%
+ collect(),
+ tibble(
+ x = c(0, 1, 1),
+ y = c(2, 3, 5),
+ z = c(8, 13, 21)
+ )
+ )
Review comment:
It doesn't, and I wonder if all the elements of inner expressions have to be aggregate expressions? FWIW I get:
``` r
library(arrow, warn.conflicts = FALSE)
library(dplyr, warn.conflicts = FALSE)
RecordBatch$create(x = c(0, 1, 1), y = c(2, 3, 5), z = c(8, 13, 21)) %>%
mutate(new_col = x + 0.1) %>%
group_by(x) %>%
summarise(r = ifelse(new_col > 1, mean(y), mean(z))) %>%
collect()
#> Warning: Error : Expression ifelse(new_col > 1, mean(y), mean(z)) not supported
#> in Arrow; pulling data into R
#> `summarise()` has grouped output by 'x'. You can override using the `.groups` argument.
#> # A tibble: 3 × 2
#> # Groups: x [2]
#> x r
#> <dbl> <dbl>
#> 1 0 8
#> 2 1 4
#> 3 1 4
```
--
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 edited a comment on pull request #11612: ARROW-14051 [R] Handle conditionals enclosing aggregate expressions
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11612:
URL: https://github.com/apache/arrow/pull/11612#issuecomment-964195863
Benchmark runs are scheduled for baseline = caf1e1ed459e2c6ff66255fc915a5a7b16c0f5c5 and contender = 5c936560c1da003baf714d67dc92f25670730c84. 5c936560c1da003baf714d67dc92f25670730c84 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/a32ec6a017f3475aba398a35a9248240...7e2a31fb4409442e93a77dfaabe7c39a/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/1a88c8de5ec34d129f7b198542286a27...b7bc0b0371a2428b81e2b97bae35ae97/)
[Finished :arrow_down:0.13% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/d76c3bf06b9b4a4b834b153f2fef5c1a...910dfdee488846d6ab422b1f5df8e9f6/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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] jonkeane commented on a change in pull request #11612: ARROW-14051 [R] Handle conditionals enclosing aggregate expressions
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #11612:
URL: https://github.com/apache/arrow/pull/11612#discussion_r745146259
##########
File path: r/tests/testthat/test-dplyr-summarize.R
##########
@@ -879,3 +879,20 @@ test_that("summarize() handles group_by .drop", {
)
)
})
+
+test_that("summarise() passes through type information for temporary columns", {
+ # applies to ifelse and case_when(), in which argument types are checked
+ # within a translated function (previously this failed because the appropriate
+ # schema was not available for n() > 1, mean(y), and mean(z))
+ compare_dplyr_binding(
+ .input %>%
+ group_by(x) %>%
+ summarise(r = ifelse(n() > 1, mean(y), mean(z))) %>%
+ collect(),
+ tibble(
+ x = c(0, 1, 1),
+ y = c(2, 3, 5),
+ z = c(8, 13, 21)
+ )
+ )
Review comment:
Hmm ok I see more of what's going on here. My first question was a bit off (since at the `ifelse()` stage we really should be evaluating aggregates in it's first argument — like in the example you gave above + you used in the tests where we're looking at `n()` for the group).
But I'm not 100% sure if the aggregate aggregate expressions are the problem here.
This new column column is admittedly a little strange (though I can imagine situations where this is ~something someone wants to do!) but I'm surprised that this still errors:
``` r
library(arrow, warn.conflicts = FALSE)
library(dplyr, warn.conflicts = FALSE)
tab <- Table$create(starwars)
tab %>%
group_by(gender) %>%
summarise(
height_mean = mean(height, na.rm = TRUE),
height_median = median(height, na.rm = TRUE),
height_evil_avg = if_else(
height_mean > 178,
mean(height, na.rm = TRUE),
as.double(median(height, na.rm = TRUE))
)) %>%
collect()
#> Warning: median() currently returns an approximate median in Arrow
#> Warning: median() currently returns an approximate median in Arrow
#> Warning: Error : Expression if_else(height_mean > 178, ..temp2,
#> as.double(..temp3)) not supported in Arrow; pulling data into R
#> # A tibble: 3 × 4
#> gender height_mean height_median height_evil_avg
#> <chr> <dbl> <dbl> <dbl>
#> 1 feminine 165. 166. 166.
#> 2 masculine 177. 183 183
#> 3 <NA> 181. 183 181.
```
It's not immediately clear to me that it's the same thing as ARROW-14586, but maybe they are interrelated? Anyway, if there's not an obvious way to get that working here that's totally fine, but maybe we should add this example to ARROW-14586 or we can create a new jira for trying to make this type work.
--
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 #11612: ARROW-14051 [R] Handle conditionals enclosing aggregate expressions
Posted by GitBox <gi...@apache.org>.
ursabot commented on pull request #11612:
URL: https://github.com/apache/arrow/pull/11612#issuecomment-964195863
Benchmark runs are scheduled for baseline = caf1e1ed459e2c6ff66255fc915a5a7b16c0f5c5 and contender = 5c936560c1da003baf714d67dc92f25670730c84. 5c936560c1da003baf714d67dc92f25670730c84 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/a32ec6a017f3475aba398a35a9248240...7e2a31fb4409442e93a77dfaabe7c39a/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/1a88c8de5ec34d129f7b198542286a27...b7bc0b0371a2428b81e2b97bae35ae97/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/d76c3bf06b9b4a4b834b153f2fef5c1a...910dfdee488846d6ab422b1f5df8e9f6/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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] jonkeane commented on a change in pull request #11612: ARROW-14051 [R] Handle conditionals enclosing aggregate expressions
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #11612:
URL: https://github.com/apache/arrow/pull/11612#discussion_r743946777
##########
File path: r/tests/testthat/test-dplyr-summarize.R
##########
@@ -879,3 +879,20 @@ test_that("summarize() handles group_by .drop", {
)
)
})
+
+test_that("summarise() passes through type information for temporary columns", {
+ # applies to ifelse and case_when(), in which argument types are checked
+ # within a translated function (previously this failed because the appropriate
+ # schema was not available for n() > 1, mean(y), and mean(z))
+ compare_dplyr_binding(
+ .input %>%
+ group_by(x) %>%
+ summarise(r = ifelse(n() > 1, mean(y), mean(z))) %>%
+ collect(),
+ tibble(
+ x = c(0, 1, 1),
+ y = c(2, 3, 5),
+ z = c(8, 13, 21)
+ )
+ )
Review comment:
Would it be good to test something like the following?
```
.input %>%
mutate(new_col = ...) %>%
group_by(x) %>%
summarise(r = ifelse(New_col > 1, mean(y), mean(z))) %>%
collect()
```
Tt should just work, but would be good to check.
##########
File path: r/R/dplyr-summarize.R
##########
@@ -218,8 +218,16 @@ summarize_eval <- function(name, quosure, ctx, hash, recurse = FALSE) {
} else if (all(inner_agg_exprs)) {
# Something like: fun(agg(x), agg(y))
# So based on the aggregations that have been extracted, mutate after
+ agg_field_refs <- make_field_refs(names(ctx$aggregations))
+ agg_field_types <- lapply(ctx$aggregations, function(x) x$data$type())
Review comment:
`ctx$aggregations` are (a list of) expressions at this point, yeah?
##########
File path: r/tests/testthat/test-dplyr-summarize.R
##########
@@ -879,3 +879,20 @@ test_that("summarize() handles group_by .drop", {
)
)
})
+
+test_that("summarise() passes through type information for temporary columns", {
+ # applies to ifelse and case_when(), in which argument types are checked
+ # within a translated function (previously this failed because the appropriate
+ # schema was not available for n() > 1, mean(y), and mean(z))
+ compare_dplyr_binding(
+ .input %>%
+ group_by(x) %>%
+ summarise(r = ifelse(n() > 1, mean(y), mean(z))) %>%
Review comment:
This is super-duper minor, but I wonder if we should use `if_else()` here since that is a bit more strict about the types of the `true` and `false` arguments. IIRC, our arrow `ifelse()` is just as strict (since it uses the exact same implementation), but the intention of the test + testing the types of those would be a little bit clear with it.
--
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 commented on a change in pull request #11612: ARROW-14051 [R] Handle conditionals enclosing aggregate expressions
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #11612:
URL: https://github.com/apache/arrow/pull/11612#discussion_r745594596
##########
File path: r/tests/testthat/test-dplyr-summarize.R
##########
@@ -879,3 +879,20 @@ test_that("summarize() handles group_by .drop", {
)
)
})
+
+test_that("summarise() passes through type information for temporary columns", {
+ # applies to ifelse and case_when(), in which argument types are checked
+ # within a translated function (previously this failed because the appropriate
+ # schema was not available for n() > 1, mean(y), and mean(z))
+ compare_dplyr_binding(
+ .input %>%
+ group_by(x) %>%
+ summarise(r = ifelse(n() > 1, mean(y), mean(z))) %>%
+ collect(),
+ tibble(
+ x = c(0, 1, 1),
+ y = c(2, 3, 5),
+ z = c(8, 13, 21)
+ )
+ )
Review comment:
Your example is fixed by this PR! The first example with `new_col` makes sense to fail because it would produce a length != 1 result. I'll add the first example to the issue about error messages in summarise.
<details>
``` r
# remotes::install_github("apache/arrow/r#11612)
library(arrow, warn.conflicts = FALSE)
library(dplyr, warn.conflicts = FALSE)
tab <- Table$create(starwars)
tab %>%
group_by(gender) %>%
summarise(
height_mean = mean(height, na.rm = TRUE),
height_median = median(height, na.rm = TRUE),
height_evil_avg = if_else(
height_mean > 178,
mean(height, na.rm = TRUE),
as.double(median(height, na.rm = TRUE))
)) %>%
collect()
#> Warning: median() currently returns an approximate median in Arrow
#> Warning: median() currently returns an approximate median in Arrow
#> # A tibble: 3 × 4
#> gender height_mean height_median height_evil_avg
#> <chr> <dbl> <dbl> <dbl>
#> 1 masculine 177. 183 183
#> 2 feminine 165. 166. 166.
#> 3 <NA> 181. 183 181.
```
<sup>Created on 2021-11-09 by the [reprex package](https://reprex.tidyverse.org) (v2.0.1)</sup>
</details>
--
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 #11612: ARROW-14051 [R] Handle conditionals enclosing aggregate expressions
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11612:
URL: https://github.com/apache/arrow/pull/11612#issuecomment-961358716
--
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 commented on a change in pull request #11612: ARROW-14051 [R] Handle conditionals enclosing aggregate expressions
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #11612:
URL: https://github.com/apache/arrow/pull/11612#discussion_r744634095
##########
File path: r/R/dplyr-summarize.R
##########
@@ -218,8 +218,16 @@ summarize_eval <- function(name, quosure, ctx, hash, recurse = FALSE) {
} else if (all(inner_agg_exprs)) {
# Something like: fun(agg(x), agg(y))
# So based on the aggregations that have been extracted, mutate after
+ agg_field_refs <- make_field_refs(names(ctx$aggregations))
+ agg_field_types <- lapply(ctx$aggregations, function(x) x$data$type())
Review comment:
Almost...it's more like `list(..temp0 = list(fun = "sum", data = Expression(...), options = list()))`
<details>
``` r
library(arrow, warn.conflicts = FALSE)
library(dplyr, warn.conflicts = FALSE)
x <- RecordBatch$create(x = c(0, 1, 1), y = c(2, 3, 5), z = c(8, 13, 21)) %>%
mutate(new_col = x + 0.1) %>%
group_by(x) %>%
summarise(r = ifelse(n() > 1, mean(y), mean(z)))
str(x$.data$aggregations)
#> List of 3
#> $ ..temp0:List of 3
#> ..$ fun : chr "sum"
#> ..$ data :Classes 'Expression', 'ArrowObject', 'R6' <Expression>
#> Inherits from: <ArrowObject>
#> Public:
#> .:xp:.: externalptr
#> cast: function (to_type, safe = TRUE, ...)
#> clone: function (deep = FALSE)
#> Equals: function (other, ...)
#> field_name: active binding
#> initialize: function (xp)
#> invalidate: function ()
#> pointer: function ()
#> print: function (...)
#> schema: Schema, ArrowObject, R6
#> set_pointer: function (xp)
#> ToString: function ()
#> type: function (schema = self$schema)
#> type_id: function (schema = self$schema)
#> ..$ options: list()
#> $ ..temp1:List of 3
#> ..$ fun : chr "mean"
#> ..$ data :Classes 'Expression', 'ArrowObject', 'R6' <Expression>
#> Inherits from: <ArrowObject>
#> Public:
#> .:xp:.: externalptr
#> cast: function (to_type, safe = TRUE, ...)
#> clone: function (deep = FALSE)
#> Equals: function (other, ...)
#> field_name: active binding
#> initialize: function (xp)
#> invalidate: function ()
#> pointer: function ()
#> print: function (...)
#> schema: Schema, ArrowObject, R6
#> set_pointer: function (xp)
#> ToString: function ()
#> type: function (schema = self$schema)
#> type_id: function (schema = self$schema)
#> ..$ options:List of 2
#> .. ..$ skip_nulls: logi FALSE
#> .. ..$ min_count : int 0
#> $ ..temp2:List of 3
#> ..$ fun : chr "mean"
#> ..$ data :Classes 'Expression', 'ArrowObject', 'R6' <Expression>
#> Inherits from: <ArrowObject>
#> Public:
#> .:xp:.: externalptr
#> cast: function (to_type, safe = TRUE, ...)
#> clone: function (deep = FALSE)
#> Equals: function (other, ...)
#> field_name: active binding
#> initialize: function (xp)
#> invalidate: function ()
#> pointer: function ()
#> print: function (...)
#> schema: Schema, ArrowObject, R6
#> set_pointer: function (xp)
#> ToString: function ()
#> type: function (schema = self$schema)
#> type_id: function (schema = self$schema)
#> ..$ options:List of 2
#> .. ..$ skip_nulls: logi FALSE
#> .. ..$ min_count : int 0
```
<sup>Created on 2021-11-08 by the [reprex package](https://reprex.tidyverse.org) (v2.0.1)</sup>
</details>
--
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 commented on a change in pull request #11612: ARROW-14051 [R] Handle conditionals enclosing aggregate expressions
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #11612:
URL: https://github.com/apache/arrow/pull/11612#discussion_r745594596
##########
File path: r/tests/testthat/test-dplyr-summarize.R
##########
@@ -879,3 +879,20 @@ test_that("summarize() handles group_by .drop", {
)
)
})
+
+test_that("summarise() passes through type information for temporary columns", {
+ # applies to ifelse and case_when(), in which argument types are checked
+ # within a translated function (previously this failed because the appropriate
+ # schema was not available for n() > 1, mean(y), and mean(z))
+ compare_dplyr_binding(
+ .input %>%
+ group_by(x) %>%
+ summarise(r = ifelse(n() > 1, mean(y), mean(z))) %>%
+ collect(),
+ tibble(
+ x = c(0, 1, 1),
+ y = c(2, 3, 5),
+ z = c(8, 13, 21)
+ )
+ )
Review comment:
Your example is fixed by this PR! The first example with `new_col` makes sense to fail because it would produce a length != 1 result. I'll add the first example to the issue about error messages in summarise.
<details>
``` r
# remotes::install_github("apache/arrow/r#11612)
library(arrow, warn.conflicts = FALSE)
library(dplyr, warn.conflicts = FALSE)
tab <- Table$create(starwars)
tab %>%
group_by(gender) %>%
summarise(
height_mean = mean(height, na.rm = TRUE),
height_median = median(height, na.rm = TRUE),
height_evil_avg = if_else(
height_mean > 178,
mean(height, na.rm = TRUE),
as.double(median(height, na.rm = TRUE))
)) %>%
collect()
#> Warning: median() currently returns an approximate median in Arrow
#> Warning: median() currently returns an approximate median in Arrow
#> # A tibble: 3 × 4
#> gender height_mean height_median height_evil_avg
#> <chr> <dbl> <dbl> <dbl>
#> 1 masculine 177. 183 183
#> 2 feminine 165. 166. 166.
#> 3 <NA> 181. 183 181.
```
</details>
--
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 edited a comment on pull request #11612: ARROW-14051 [R] Handle conditionals enclosing aggregate expressions
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11612:
URL: https://github.com/apache/arrow/pull/11612#issuecomment-964195863
Benchmark runs are scheduled for baseline = caf1e1ed459e2c6ff66255fc915a5a7b16c0f5c5 and contender = 5c936560c1da003baf714d67dc92f25670730c84. 5c936560c1da003baf714d67dc92f25670730c84 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/a32ec6a017f3475aba398a35a9248240...7e2a31fb4409442e93a77dfaabe7c39a/)
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/1a88c8de5ec34d129f7b198542286a27...b7bc0b0371a2428b81e2b97bae35ae97/)
[Finished :arrow_down:0.13% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/d76c3bf06b9b4a4b834b153f2fef5c1a...910dfdee488846d6ab422b1f5df8e9f6/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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