[GitHub] [arrow] jonkeane commented on a diff in pull request #12818: ARROW-16038: [R] different behavior from dplyr when mutate's `.keep` option is set

[GitBox <gi...@apache.org>]

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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org