jonkeane commented on code in PR #12818:
URL: https://github.com/apache/arrow/pull/12818#discussion_r844432758
##########
r/R/dplyr-mutate.R:
##########
@@ -94,7 +94,8 @@ mutate.arrow_dplyr_query <- function(.data,
# Respect .keep
if (.keep == "none") {
- .data$selected_columns <- .data$selected_columns[new_vars]
+ new_cols_last <- c(intersect(old_vars, new_vars), setdiff(new_vars,
old_vars))
+ .data$selected_columns <- .data$selected_columns[new_cols_last]
Review Comment:
It might be nice to have a comment explaining what we're doing here
##########
r/R/dplyr-mutate.R:
##########
@@ -112,7 +113,15 @@ mutate.Dataset <- mutate.ArrowTabular <-
mutate.RecordBatchReader <- mutate.arro
transmute.arrow_dplyr_query <- function(.data, ...) {
dots <- check_transmute_args(...)
- dplyr::mutate(.data, !!!dots, .keep = "none")
+ has_null <- vapply(dots, quo_is_null, logical(1))
Review Comment:
This is rather stylistic, but I think `map_lgl(dots, quo_is_null)` would fit
a bit more with that package style (I see quite a few more `purrr::map`s
compared to `vapply`s)
##########
r/R/dplyr-mutate.R:
##########
@@ -112,7 +113,15 @@ mutate.Dataset <- mutate.ArrowTabular <-
mutate.RecordBatchReader <- mutate.arro
transmute.arrow_dplyr_query <- function(.data, ...) {
dots <- check_transmute_args(...)
- dplyr::mutate(.data, !!!dots, .keep = "none")
+ has_null <- vapply(dots, quo_is_null, logical(1))
+ .data <- dplyr::mutate(.data, !!!dots, .keep = "none")
+ if (is_empty(dots) | any(has_null)) return(.data)
+
+ ## keeping with: https://github.com/tidyverse/dplyr/issues/6086
+ cur_exprs <- vapply(dots, as_label, character(1))
+ new_col_names <- names(dots)
+ transmute_order <- ifelse(nzchar(new_col_names), new_col_names, cur_exprs)
Review Comment:
I'm having a bit of trouble following what this line is doing here — what
behavior are we trying to catch here?
##########
r/tests/testthat/test-dplyr-mutate.R:
##########
@@ -329,13 +339,13 @@ test_that("dplyr::mutate's examples", {
#> <chr> <chr> <dbl>
#> 1 a b 3
compare_dplyr_binding(
- .input %>% mutate(z = x + y, .keep = "none") %>% collect(), # same as
transmute()
+ .input %>% mutate(z = x + y, x, .keep = "none") %>% collect(), # same as
transmute()
df
)
- #> # A tibble: 1 x 1
- #> z
- #> <dbl>
- #> 1 3
+ #> # A tibble: 1 × 2
+ #> x z
+ #> <dbl> <dbl>
+ #> 1 1 3
Review Comment:
Thanks for updating all of this!
##########
r/R/dplyr-mutate.R:
##########
@@ -112,7 +113,15 @@ mutate.Dataset <- mutate.ArrowTabular <-
mutate.RecordBatchReader <- mutate.arro
transmute.arrow_dplyr_query <- function(.data, ...) {
dots <- check_transmute_args(...)
- dplyr::mutate(.data, !!!dots, .keep = "none")
+ has_null <- vapply(dots, quo_is_null, logical(1))
+ .data <- dplyr::mutate(.data, !!!dots, .keep = "none")
+ if (is_empty(dots) | any(has_null)) return(.data)
Review Comment:
This is a style nit-pick, but could we put the `return(.data)` on it's own
line? (cf https://style.tidyverse.org/functions.html#return)
##########
r/tests/testthat/test-dplyr-mutate.R:
##########
@@ -74,6 +74,16 @@ test_that("transmute", {
)
})
+test_that("transmute respect bespoke dplyr implementation", {
+ ## see: https://github.com/tidyverse/dplyr/issues/6086
+ compare_dplyr_binding(
+ .input %>%
+ transmute(dbl, int = int + 6L) %>%
+ collect(),
+ tbl
+ )
+})
Review Comment:
Could we add a test with unnamed arguments to `transmute()` here as well?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
