[GitHub] [arrow] nealrichardson commented on a change in pull request #9294: ARROW-8919: [C++][Compute][Dataset] Add Function::DispatchBest to accomodate implicit casts

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

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



##########
File path: r/tests/testthat/test-compute-arith.R
##########
@@ -18,32 +18,31 @@
 test_that("Addition", {
   a <- Array$create(c(1:4, NA_integer_))
   expect_type_equal(a, int32())
-  expect_type_equal(a + 4, int32())
-  expect_equal(a + 4, Array$create(c(5:8, NA_integer_)))
-  expect_identical(as.vector(a + 4), c(5:8, NA_integer_))
+  expect_type_equal(a + 4L, int32())
+  expect_type_equal(a + 4, float64())
+  expect_equal(a + 4L, Array$create(c(5:8, NA_integer_)))
+  expect_identical(as.vector(a + 4L), c(5:8, NA_integer_))
   expect_equal(a + 4L, Array$create(c(5:8, NA_integer_)))
   expect_vector(a + 4L, c(5:8, NA_integer_))
   expect_equal(a + NA_integer_, Array$create(rep(NA_integer_, 5)))
 
-  # overflow errors — this is slightly different from R's `NA` coercion when
-  # overflowing, but better than the alternative of silently restarting
-  casted <- a$cast(int8())
-  expect_error(casted + 127)
-  expect_error(casted + 200)
+  a8 <- a$cast(int8())
+  expect_type_equal(a8 + Scalar$create(1, int8()), int8())
+  expect_type_equal(a8 + 127L, int32())
+  expect_type_equal(a8 + 200L, int32())
 
-  skip("autocasting should happen in compute kernels; R workaround fails on this ARROW-8919")
   expect_type_equal(a + 4.1, float64())
   expect_equal(a + 4.1, Array$create(c(5.1, 6.1, 7.1, 8.1, NA_real_)))
 })
 
 test_that("Subtraction", {
   a <- Array$create(c(1:4, NA_integer_))
-  expect_equal(a - 3, Array$create(c(-2:1, NA_integer_)))
+  expect_equal(a - 3L, Array$create(c(-2:1, NA_integer_)))

Review comment:
       These tests aren't doing any autocasting so we should add some that do.
   
   Also would be interesting to add tests that do arithmetic with two arrays since we can support that more generally now.

##########
File path: r/tests/testthat/test-dataset.R
##########
@@ -554,6 +554,7 @@ test_that("filter() on timestamp columns", {
   )
 
   # Now with bare string date
+  skip("Implement more aggressive implicit casting for scalars")

Review comment:
       Is there a JIRA for this? 
   
   I'm not sure this is about scalars per se (though that may be an issue/part of the "best" solution). This seems to be about timestamp-string comparison/ops.

##########
File path: r/tests/testthat/test-compute-arith.R
##########
@@ -18,32 +18,31 @@
 test_that("Addition", {
   a <- Array$create(c(1:4, NA_integer_))
   expect_type_equal(a, int32())
-  expect_type_equal(a + 4, int32())
-  expect_equal(a + 4, Array$create(c(5:8, NA_integer_)))
-  expect_identical(as.vector(a + 4), c(5:8, NA_integer_))
+  expect_type_equal(a + 4L, int32())
+  expect_type_equal(a + 4, float64())
+  expect_equal(a + 4L, Array$create(c(5:8, NA_integer_)))
+  expect_identical(as.vector(a + 4L), c(5:8, NA_integer_))
   expect_equal(a + 4L, Array$create(c(5:8, NA_integer_)))
   expect_vector(a + 4L, c(5:8, NA_integer_))
   expect_equal(a + NA_integer_, Array$create(rep(NA_integer_, 5)))
 
-  # overflow errors — this is slightly different from R's `NA` coercion when
-  # overflowing, but better than the alternative of silently restarting
-  casted <- a$cast(int8())
-  expect_error(casted + 127)
-  expect_error(casted + 200)
+  a8 <- a$cast(int8())
+  expect_type_equal(a8 + Scalar$create(1, int8()), int8())
+  expect_type_equal(a8 + 127L, int32())
+  expect_type_equal(a8 + 200L, int32())

Review comment:
       (1) this should still have a comment explaining the different behavior
   (2) we should assert the values too 




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

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