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/04/27 18:43:59 UTC
[PR] MINOR: [R] refactor arrow_mask to include ..aggregations list [arrow]
nealrichardson opened a new pull request, #41414:
URL: https://github.com/apache/arrow/pull/41414
### Rationale for this change
Keeping the `..aggregations` list in parent.frame felt a little wrong. As we're starting to use this in more places (like mutate in #41350, and potentially more places), I wanted to try to improve this. I tried a bunch of things before to put it somewhere better (like in the mask) but failed. Finally I found one that worked.
### What changes are included in this PR?
Just a refactor
### Are these changes tested?
Existing tests pass.
### Are there any user-facing changes?
Nope.
--
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] MINOR: [R] refactor arrow_mask to include ..aggregations list [arrow]
Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane commented on code in PR #41414:
URL: https://github.com/apache/arrow/pull/41414#discussion_r1581975938
##########
r/R/dplyr-eval.R:
##########
@@ -156,6 +152,8 @@ arrow_mask <- function(.data, aggregation = FALSE) {
# TODO: figure out what rlang::as_data_pronoun does/why we should use it
# (because if we do we get `Error: Can't modify the data pronoun` in mutate())
out$.data <- .data$selected_columns
+ # Add the aggregations list to collect any that get pulled out when evaluating
+ out$..aggregations <- empty_named_list()
Review Comment:
Any reason to have this be `..aggregations` instead of `.aggregations` like `.data`? Is it intended to be a signifier it's more internal than the `.data` is?
It's no big deal either way, just a little funny looking now that it's here.
--
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] MINOR: [R] refactor arrow_mask to include aggregations list [arrow]
Posted by "nealrichardson (via GitHub)" <gi...@apache.org>.
nealrichardson merged PR #41414:
URL: https://github.com/apache/arrow/pull/41414
--
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] MINOR: [R] refactor arrow_mask to include ..aggregations list [arrow]
Posted by "nealrichardson (via GitHub)" <gi...@apache.org>.
nealrichardson commented on code in PR #41414:
URL: https://github.com/apache/arrow/pull/41414#discussion_r1581999889
##########
r/R/dplyr-eval.R:
##########
@@ -156,6 +152,8 @@ arrow_mask <- function(.data, aggregation = FALSE) {
# TODO: figure out what rlang::as_data_pronoun does/why we should use it
# (because if we do we get `Error: Can't modify the data pronoun` in mutate())
out$.data <- .data$selected_columns
+ # Add the aggregations list to collect any that get pulled out when evaluating
+ out$..aggregations <- empty_named_list()
Review Comment:
Double secret ;) IDK, you're right, I can delete a dot.
--
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] MINOR: [R] refactor arrow_mask to include aggregations list [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #41414:
URL: https://github.com/apache/arrow/pull/41414#issuecomment-2083441191
After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit c87073737b6ffef9715549a199499b92630e8e5f.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/24391792915) 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