[GitHub] [arrow] nealrichardson commented on a change in pull request #10724: ARROW-12964: [R] Add bindings for ifelse() and if_else()

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

nealrichardson commented on a change in pull request #10724:
URL: https://github.com/apache/arrow/pull/10724#discussion_r670704661



##########
File path: r/R/dplyr-functions.R
##########
@@ -559,3 +559,24 @@ nse_funcs$wday <- function(x, label = FALSE, abbr = TRUE, week_start = getOption
   Expression$create("day_of_week", x, options = list(one_based_numbering = TRUE, week_start = week_start))
 
 }
+
+nse_funcs$if_else <- function(condition, true, false, missing = NULL){
+  # We ought to assert that the types of the true and false conditions will result
+  # in the same types. We can't compare the objects themselves directly because
+  # they might be expressions (that will result in a type) or R objects that will
+  # need to be compared to see if they are compatible with arrow types.
+  # ARROW-13186 might make this easier with a more robust way.
+  # TODO: do this ^^^
+
+  if (inherits(true, "character") || inherits(false, "character")) {
+    stop("`true` and `false` character values not yet supported in Arrow")

Review comment:
       Why?

##########
File path: r/tests/testthat/test-dplyr.R
##########
@@ -945,3 +945,63 @@ test_that("abs()", {
     df
   )
 })
+
+test_that("if_else and ifelse", {
+  df <- tibble(x = c(-127, -10, -1, -0 , 0, 1, 10, 127, NA))

Review comment:
       Is there a reason we only test with type numeric? Do we trust that all types are faithfully tested in C++?

##########
File path: r/tests/testthat/test-dplyr.R
##########
@@ -945,3 +945,63 @@ test_that("abs()", {
     df
   )
 })
+
+test_that("if_else and ifelse", {
+  df <- tibble(x = c(-127, -10, -1, -0 , 0, 1, 10, 127, NA))
+
+  expect_dplyr_equal(
+    input %>%
+      mutate(
+        y = if_else(x > 0, 1, 0)
+      ) %>% collect(),
+    df
+  )
+
+  expect_dplyr_equal(
+    input %>%
+      mutate(
+        y = if_else(x > 0, x, 0)
+      ) %>% collect(),
+    df
+  )
+
+  expect_error(
+    Table$create(df) %>%
+      mutate(
+        y = if_else(x > 0, 1, FALSE)
+      ) %>% collect(),
+    'NotImplemented: Function if_else has no kernel matching input types'
+  )
+
+  expect_dplyr_equal(
+    input %>%
+      mutate(
+        y = if_else(x > 0, 1, NA_real_)
+      ) %>% collect(),
+    df
+  )
+
+  expect_dplyr_equal(
+    input %>%
+      mutate(
+        y = ifelse(x > 0, 1, 0)
+      ) %>% collect(),
+    df
+  )
+
+  expect_dplyr_equal(
+    input %>%
+      mutate(
+        y = ifelse(x > 0, x, 0)
+      ) %>% collect(),
+    df
+  )
+
+  skip("TODO: could? should? we support the autocasting in ifelse")

Review comment:
       Every compute function should autopromote the same way right? How does this fail?

##########
File path: r/tests/testthat/test-dplyr.R
##########
@@ -945,3 +945,63 @@ test_that("abs()", {
     df
   )
 })
+
+test_that("if_else and ifelse", {
+  df <- tibble(x = c(-127, -10, -1, -0 , 0, 1, 10, 127, NA))
+
+  expect_dplyr_equal(
+    input %>%
+      mutate(
+        y = if_else(x > 0, 1, 0)
+      ) %>% collect(),
+    df
+  )
+
+  expect_dplyr_equal(
+    input %>%
+      mutate(
+        y = if_else(x > 0, x, 0)
+      ) %>% collect(),
+    df
+  )
+
+  expect_error(
+    Table$create(df) %>%
+      mutate(
+        y = if_else(x > 0, 1, FALSE)
+      ) %>% collect(),
+    'NotImplemented: Function if_else has no kernel matching input types'
+  )
+
+  expect_dplyr_equal(
+    input %>%
+      mutate(
+        y = if_else(x > 0, 1, NA_real_)
+      ) %>% collect(),
+    df
+  )
+
+  expect_dplyr_equal(
+    input %>%
+      mutate(
+        y = ifelse(x > 0, 1, 0)
+      ) %>% collect(),
+    df
+  )
+
+  expect_dplyr_equal(
+    input %>%
+      mutate(
+        y = ifelse(x > 0, x, 0)
+      ) %>% collect(),
+    df
+  )
+
+  skip("TODO: could? should? we support the autocasting in ifelse")

Review comment:
       Oh I see above. 




-- 
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