[GitHub] [arrow] jonkeane commented on a change in pull request #10519: ARROW-12867: [R] Bindings for abs()

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

jonkeane commented on a change in pull request #10519:
URL: https://github.com/apache/arrow/pull/10519#discussion_r652126604



##########
File path: r/tests/testthat/test-dplyr-arrange.R
##########
@@ -139,18 +139,14 @@ test_that("arrange() on integer, double, and character columns", {
       collect(),
     tbl
   )
-  expect_warning(
-    expect_equal(
-      tbl %>%
-        Table$create() %>%
-        arrange(abs(int), dbl) %>%
-        collect(),
-      tbl %>%
-        arrange(abs(int), dbl) %>%
-        collect()
-    ),
-    "not supported in Arrow",
-    fixed = TRUE
+  expect_equal(

Review comment:
       If we're going to keep this, could we use `expect_dplyr_equal()` here as well?
   
   Though I'm not certain we really need to have this test since we test that `abs()` works in test-dplyr.R. It might still be good/nice to have a test that confirms the warning when a function is not supported in Arrow which in some ways looks like what this test was really intending to test. Maybe instead of using a function like this that we might make a kernel for we make up some other function that we will never use in a kernel (at least the name) and keep the test that throws a warning + pulls things into R + compares that the results are equal.

##########
File path: r/R/dplyr-functions.R
##########
@@ -108,6 +108,10 @@ nse_funcs$is.infinite <- function(x) {
   is_inf & !nse_funcs$is.na(is_inf)
 }
 
+nse_funcs$abs <- function(x){

Review comment:
       A super nit-picky whitespace suggestion:
   
   ```suggestion
   nse_funcs$abs <- function(x) {
   ```




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