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/07/22 22:49:04 UTC
[GitHub] [arrow] jonkeane opened a new pull request #10785: ARROW-13434: [R] group_by() with an unnammed expression
jonkeane opened a new pull request #10785:
URL: https://github.com/apache/arrow/pull/10785
--
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 #10785: ARROW-13434: [R] group_by() with an unnammed expression
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #10785:
URL: https://github.com/apache/arrow/pull/10785#discussion_r675716423
##########
File path: r/R/dplyr-group-by.R
##########
@@ -18,16 +18,33 @@
# The following S3 methods are registered on load if dplyr is present
+#' @importFrom rlang quo_name
group_by.arrow_dplyr_query <- function(.data,
...,
.add = FALSE,
add = .add,
.drop = dplyr::group_by_drop_default(.data)) {
.data <- arrow_dplyr_query(.data)
- # ... can contain expressions (i.e. can add (or rename?) columns)
- # Check for those (they show up as named expressions)
new_groups <- enquos(...)
- new_groups <- new_groups[nzchar(names(new_groups))]
+ # ... can contain expressions (i.e. can add (or rename?) columns) and so we
+ # need to identify those and add them on to the query with mutate. Specifically,
+ # we want to mark as new:
+ # * expressions (named or otherwise)
+ # * variables that have new names
+ # All others (i.e. simple references to variables) should not be (re)-added
+ new_group_ind <- purrr::map_lgl(new_groups, ~!(quo_name(.x) %in% names(.data)))
+ named_group_ind <- purrr::map_lgl(names(new_groups), nzchar)
+ new_groups <- new_groups[new_group_ind | named_group_ind]
+
+ # now either use the name that was given in ... or if that is "" then use the expr
+ names(new_groups) <- purrr::map_chr(seq_along(new_groups), function(i) {
+ name <- names(new_groups)[[i]]
+ if (name == "") {
+ quo_name(new_groups[[i]])
+ } else {
+ name
+ }
+ })
Review comment:
OOOH TIL about `imap_chr()` that's 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] thisisnic commented on a change in pull request #10785: ARROW-13434: [R] group_by() with an unnammed expression
Posted by GitBox <gi...@apache.org>.
thisisnic commented on a change in pull request #10785:
URL: https://github.com/apache/arrow/pull/10785#discussion_r675511834
##########
File path: r/R/dplyr-group-by.R
##########
@@ -18,16 +18,33 @@
# The following S3 methods are registered on load if dplyr is present
+#' @importFrom rlang quo_name
Review comment:
Just checking for my own understanding - we have some function imports specified in arrow-package.R, and others next to the function that calls them - what determines this?
--
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] thisisnic commented on a change in pull request #10785: ARROW-13434: [R] group_by() with an unnammed expression
Posted by GitBox <gi...@apache.org>.
thisisnic commented on a change in pull request #10785:
URL: https://github.com/apache/arrow/pull/10785#discussion_r675509874
##########
File path: r/tests/testthat/test-dplyr-group-by.R
##########
@@ -33,7 +33,7 @@ test_that("group_by groupings are recorded", {
)
})
-test_that("group_by doesn't yet support creating/renaming", {
+test_that("group_by does now support creating/renaming", {
Review comment:
```suggestion
test_that("group_by supports creating/renaming", {
```
--
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] thisisnic commented on a change in pull request #10785: ARROW-13434: [R] group_by() with an unnammed expression
Posted by GitBox <gi...@apache.org>.
thisisnic commented on a change in pull request #10785:
URL: https://github.com/apache/arrow/pull/10785#discussion_r675511834
##########
File path: r/R/dplyr-group-by.R
##########
@@ -18,16 +18,33 @@
# The following S3 methods are registered on load if dplyr is present
+#' @importFrom rlang quo_name
Review comment:
Just checking for my own understanding - we have some function imported specified in arrow-package.R, and others next to the function that calls them - what determines this?
--
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 #10785: ARROW-13434: [R] group_by() with an unnammed expression
Posted by GitBox <gi...@apache.org>.
jonkeane closed pull request #10785:
URL: https://github.com/apache/arrow/pull/10785
--
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 #10785: ARROW-13434: [R] group_by() with an unnammed expression
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #10785:
URL: https://github.com/apache/arrow/pull/10785#discussion_r675547210
##########
File path: r/R/dplyr-group-by.R
##########
@@ -18,16 +18,33 @@
# The following S3 methods are registered on load if dplyr is present
+#' @importFrom rlang quo_name
Review comment:
My read on how we do it in the Arrow package is that we import if:
* it's in the package (i.e. under the R/ directory, as opposed to in tests)
* the dependency is _not_ a `SUGGEST`
otherwise we (tend to) `pkg::` (e.g. a bunch of `rlang::eval_tidy` in `helper-expectation.R`. I'm sure there are exceptions to this and looking for `::` in `R/` I see a few just scanning.
I've seen others recommend that using a function a small number of times it's better to `pkg::` that and once some threshold is reached then switch to importing. Or use `pkg::` for the first call in a file / package to make it clear that's what one is referring to.
--
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 change in pull request #10785: ARROW-13434: [R] group_by() with an unnammed expression
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #10785:
URL: https://github.com/apache/arrow/pull/10785#discussion_r675593809
##########
File path: r/R/dplyr-group-by.R
##########
@@ -18,16 +18,33 @@
# The following S3 methods are registered on load if dplyr is present
+#' @importFrom rlang quo_name
Review comment:
I think this is about the `@importFrom`, which Nic is right, we have most of them grouped in one place. I'm indifferent about how we do it but probably makes sense to follow convention and for these rlang ones at least, which tend to get used in multiple places, keep them grouped in arrow-package.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] thisisnic commented on pull request #10785: ARROW-13434: [R] group_by() with an unnammed expression
Posted by GitBox <gi...@apache.org>.
thisisnic commented on pull request #10785:
URL: https://github.com/apache/arrow/pull/10785#issuecomment-885591068
Also, nice catch 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
[GitHub] [arrow] nealrichardson commented on a change in pull request #10785: ARROW-13434: [R] group_by() with an unnammed expression
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #10785:
URL: https://github.com/apache/arrow/pull/10785#discussion_r675596476
##########
File path: r/R/dplyr-group-by.R
##########
@@ -18,16 +18,33 @@
# The following S3 methods are registered on load if dplyr is present
+#' @importFrom rlang quo_name
group_by.arrow_dplyr_query <- function(.data,
...,
.add = FALSE,
add = .add,
.drop = dplyr::group_by_drop_default(.data)) {
.data <- arrow_dplyr_query(.data)
- # ... can contain expressions (i.e. can add (or rename?) columns)
- # Check for those (they show up as named expressions)
new_groups <- enquos(...)
- new_groups <- new_groups[nzchar(names(new_groups))]
+ # ... can contain expressions (i.e. can add (or rename?) columns) and so we
+ # need to identify those and add them on to the query with mutate. Specifically,
+ # we want to mark as new:
+ # * expressions (named or otherwise)
+ # * variables that have new names
+ # All others (i.e. simple references to variables) should not be (re)-added
+ new_group_ind <- purrr::map_lgl(new_groups, ~!(quo_name(.x) %in% names(.data)))
Review comment:
No need to `purrr::` these, we import them
##########
File path: r/R/dplyr-group-by.R
##########
@@ -18,16 +18,33 @@
# The following S3 methods are registered on load if dplyr is present
+#' @importFrom rlang quo_name
Review comment:
To your points Jon, IMO any function from a package we Import should be in `importsFrom` and be used without `::`, and we only use `::` for Suggested packages.
##########
File path: r/R/dplyr-group-by.R
##########
@@ -18,16 +18,33 @@
# The following S3 methods are registered on load if dplyr is present
+#' @importFrom rlang quo_name
group_by.arrow_dplyr_query <- function(.data,
...,
.add = FALSE,
add = .add,
.drop = dplyr::group_by_drop_default(.data)) {
.data <- arrow_dplyr_query(.data)
- # ... can contain expressions (i.e. can add (or rename?) columns)
- # Check for those (they show up as named expressions)
new_groups <- enquos(...)
- new_groups <- new_groups[nzchar(names(new_groups))]
+ # ... can contain expressions (i.e. can add (or rename?) columns) and so we
+ # need to identify those and add them on to the query with mutate. Specifically,
+ # we want to mark as new:
+ # * expressions (named or otherwise)
+ # * variables that have new names
+ # All others (i.e. simple references to variables) should not be (re)-added
+ new_group_ind <- purrr::map_lgl(new_groups, ~!(quo_name(.x) %in% names(.data)))
+ named_group_ind <- purrr::map_lgl(names(new_groups), nzchar)
+ new_groups <- new_groups[new_group_ind | named_group_ind]
+
+ # now either use the name that was given in ... or if that is "" then use the expr
+ names(new_groups) <- purrr::map_chr(seq_along(new_groups), function(i) {
+ name <- names(new_groups)[[i]]
+ if (name == "") {
+ quo_name(new_groups[[i]])
+ } else {
+ name
+ }
+ })
Review comment:
I figured there must be a `purrr` function do to this (the equivalent of python's `enumerate()`) and [it turns out there is](https://purrr.tidyverse.org/reference/imap.html). I think it would look like this:
```suggestion
names(new_groups) <- imap_chr(new_groups, ~ ifelse(.y == "", quo_name(.x), .y))
```
(Also need to add `imap_chr` to `@importFrom`)
##########
File path: r/R/dplyr-group-by.R
##########
@@ -18,16 +18,33 @@
# The following S3 methods are registered on load if dplyr is present
+#' @importFrom rlang quo_name
group_by.arrow_dplyr_query <- function(.data,
...,
.add = FALSE,
add = .add,
.drop = dplyr::group_by_drop_default(.data)) {
.data <- arrow_dplyr_query(.data)
- # ... can contain expressions (i.e. can add (or rename?) columns)
- # Check for those (they show up as named expressions)
new_groups <- enquos(...)
- new_groups <- new_groups[nzchar(names(new_groups))]
+ # ... can contain expressions (i.e. can add (or rename?) columns) and so we
+ # need to identify those and add them on to the query with mutate. Specifically,
+ # we want to mark as new:
+ # * expressions (named or otherwise)
+ # * variables that have new names
+ # All others (i.e. simple references to variables) should not be (re)-added
+ new_group_ind <- purrr::map_lgl(new_groups, ~!(quo_name(.x) %in% names(.data)))
+ named_group_ind <- purrr::map_lgl(names(new_groups), nzchar)
+ new_groups <- new_groups[new_group_ind | named_group_ind]
+
+ # now either use the name that was given in ... or if that is "" then use the expr
+ names(new_groups) <- purrr::map_chr(seq_along(new_groups), function(i) {
+ name <- names(new_groups)[[i]]
+ if (name == "") {
+ quo_name(new_groups[[i]])
+ } else {
+ name
+ }
+ })
Review comment:
I'd also put this inside the `if (length(new_groups))` below, just to be clear that this only matters if we have new groups
--
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 #10785: ARROW-13434: [R] group_by() with an unnammed expression
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #10785:
URL: https://github.com/apache/arrow/pull/10785#discussion_r675595968
##########
File path: r/R/dplyr-group-by.R
##########
@@ -18,16 +18,33 @@
# The following S3 methods are registered on load if dplyr is present
+#' @importFrom rlang quo_name
Review comment:
OOOH, I see, I misread it at first. Yeah I can move it over to arrow-package.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] thisisnic commented on a change in pull request #10785: ARROW-13434: [R] group_by() with an unnammed expression
Posted by GitBox <gi...@apache.org>.
thisisnic commented on a change in pull request #10785:
URL: https://github.com/apache/arrow/pull/10785#discussion_r675599160
##########
File path: r/R/dplyr-group-by.R
##########
@@ -18,16 +18,33 @@
# The following S3 methods are registered on load if dplyr is present
+#' @importFrom rlang quo_name
Review comment:
Yeah, I could have been more clear in how I phrased that, thanks for clarifying there Neal!
--
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 #10785: ARROW-13434: [R] group_by() with an unnammed expression
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10785:
URL: https://github.com/apache/arrow/pull/10785#issuecomment-885284831
https://issues.apache.org/jira/browse/ARROW-13434
--
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