You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/06/14 07:26:30 UTC
[GitHub] [arrow] AlenkaF opened a new pull request #10519: ARROW-12867: [R] Bindings for abs()
AlenkaF opened a new pull request #10519:
URL: https://github.com/apache/arrow/pull/10519
--
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
[GitHub] [arrow] AlenkaF commented on pull request #10519: ARROW-12867: [R] Bindings for abs()
Posted by GitBox <gi...@apache.org>.
AlenkaF commented on pull request #10519:
URL: https://github.com/apache/arrow/pull/10519#issuecomment-861400881
@thisisnic I think it's ready for another round of review ;)
--
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
[GitHub] [arrow] jonkeane commented on a change in pull request #10519: ARROW-12867: [R] Bindings for abs()
Posted by 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
[GitHub] [arrow] AlenkaF commented on a change in pull request #10519: ARROW-12867: [R] Bindings for abs()
Posted by GitBox <gi...@apache.org>.
AlenkaF commented on a change in pull request #10519:
URL: https://github.com/apache/arrow/pull/10519#discussion_r650068914
##########
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){
+ Expression$create("abs_checked", x)
Review comment:
I used `abs_checked` to be in sync with [ARROW-12575: [R] Use unary negative kernel](https://github.com/apache/arrow/pull/10196), see [discussion](https://github.com/apache/arrow/pull/10196/files/288e06ed681c170de587591b88c6b37f96a3eddd#r625885713).
--
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
[GitHub] [arrow] AlenkaF commented on a change in pull request #10519: ARROW-12867: [R] Bindings for abs()
Posted by GitBox <gi...@apache.org>.
AlenkaF commented on a change in pull request #10519:
URL: https://github.com/apache/arrow/pull/10519#discussion_r654431915
##########
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:
@thisisnic if I understand correctly it should be something in line off:
```R
dummy_function <- function(x){
if(assert_is(x, c("numeric", "integer"))) warning()
x
}
expect_warning(
expect_equal(
tbl %>%
Table$create() %>%
arrange(dummy_function(int), dbl) %>%
collect(),
tbl %>%
arrange(dummy_function(int), dbl) %>%
collect()
),
"not supported in Arrow",
fixed = TRUE
)
```
Does it make sense?
##########
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:
@thisisnic @jonkeane should I put the correction of the `arrange` test - where a solution needs to be found for confirming the warning - in a separate issue and we close this PR?
--
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
[GitHub] [arrow] AlenkaF commented on a change in pull request #10519: ARROW-12867: [R] Bindings for abs()
Posted by GitBox <gi...@apache.org>.
AlenkaF commented on a change in pull request #10519:
URL: https://github.com/apache/arrow/pull/10519#discussion_r652542859
##########
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:
Thought about this also @thisisnic but then it wouldn't get pull into R or would it?
--
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
[GitHub] [arrow] AlenkaF commented on pull request #10519: ARROW-12867: [R] Bindings for abs()
Posted by GitBox <gi...@apache.org>.
AlenkaF commented on pull request #10519:
URL: https://github.com/apache/arrow/pull/10519#issuecomment-860533901
Thank you for the comment @thisisnic! Of course, now I understand. I will remove `expect_warning` from `test-dplyr-arrange.R` right away.
--
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
[GitHub] [arrow] jonkeane closed pull request #10519: ARROW-12867: [R] Bindings for abs()
Posted by GitBox <gi...@apache.org>.
jonkeane closed pull request #10519:
URL: https://github.com/apache/arrow/pull/10519
--
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
[GitHub] [arrow] thisisnic edited a comment on pull request #10519: ARROW-12867: [R] Bindings for abs()
Posted by GitBox <gi...@apache.org>.
thisisnic edited a comment on pull request #10519:
URL: https://github.com/apache/arrow/pull/10519#issuecomment-860526967
Nice, thanks for submitting this PR! There is a test failure caused by the calls to `abs()` on lines 146 and 149 in `test-dplyr-arrange.R` - the test they're part of previously was expected to result in a warning message as the function `abs` was not supported, but now you've added this functionality, no warning is issued and so the test fails.
--
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
[GitHub] [arrow] thisisnic commented on a change in pull request #10519: ARROW-12867: [R] Bindings for abs()
Posted by GitBox <gi...@apache.org>.
thisisnic commented on a change in pull request #10519:
URL: https://github.com/apache/arrow/pull/10519#discussion_r652529386
##########
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:
I was thinking about this myself, and am wondering if that could be achieved via a user-defined function called something ridiculous like `not_a_function` or similar?
--
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
[GitHub] [arrow] ianmcook commented on a change in pull request #10519: ARROW-12867: [R] Bindings for abs()
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #10519:
URL: https://github.com/apache/arrow/pull/10519#discussion_r654734633
##########
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:
The way `arrange()` works, it should be able to take any valid expressions as arguments, and assuming we have the translations for all the functions in the expressions implemented here in the arrow package, it should work. The `desc()` function is a special case—it is _not_ implemented generally for other dplyr verbs and it only works within `arrange()`.
So I'm confused; is `arrange(abs(int), dbl))` not working? It should work.
--
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
[GitHub] [arrow] ianmcook commented on a change in pull request #10519: ARROW-12867: [R] Bindings for abs()
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #10519:
URL: https://github.com/apache/arrow/pull/10519#discussion_r654942449
##########
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:
Oh, I understand now 😄 Sorry for my earlier confusion @AlenkaF!
Here is what I would recommend doing:
1. Remove the test code beginning on line 142, and do not replace it with anything. As @jonkeane said in the first comment in this thread:
> I'm not certain we really need to have this test since we test that `abs()` works in test-dplyr.R
I agree with that. Actually, I am certain we do not need to have this test.
2. Set aside the questions about how best to write tests for unimplemented functions. As @thisisnic says
>What I had in mind was a more general way of testing functions that shouldn't work in the dplyr pipelines... However, ... this already seems to exist
We can revisit that issue separately from this PR, if needed.
Thanks!
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #10519: ARROW-12867: [R] Bindings for abs()
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10519:
URL: https://github.com/apache/arrow/pull/10519#issuecomment-859534414
https://issues.apache.org/jira/browse/ARROW-12867
--
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
[GitHub] [arrow] ianmcook commented on a change in pull request #10519: ARROW-12867: [R] Bindings for abs()
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #10519:
URL: https://github.com/apache/arrow/pull/10519#discussion_r654734633
##########
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:
The way `arrange()` works, it should be able to take any valid expressions as arguments, and assuming we have the translations for those functions implemented here in the arrow package, it should work. The `desc()` function is a special case—it is _not_ implemented generally for other dplyr verbs and it only works within `arrange()`.
So I'm confused; is `arrange(abs(int), dbl))` not working? It should work.
--
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
[GitHub] [arrow] AlenkaF commented on a change in pull request #10519: ARROW-12867: [R] Bindings for abs()
Posted by GitBox <gi...@apache.org>.
AlenkaF commented on a change in pull request #10519:
URL: https://github.com/apache/arrow/pull/10519#discussion_r652542859
##########
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:
Thought about this also @thisisnic but then it wouldn't get pulled into R or would it?
--
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
[GitHub] [arrow] ianmcook commented on a change in pull request #10519: ARROW-12867: [R] Bindings for abs()
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #10519:
URL: https://github.com/apache/arrow/pull/10519#discussion_r654734633
##########
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:
The way `arrange()` works, it should be able to take any valid expressions as arguments, and assuming we have the translations for all the functions in the expressions implemented here in the arrow package, it should work. The `desc()` function is a special case—it is _not_ implemented generally for other dplyr verbs and it only works within `arrange()`.
So I'm confused; is `arrange(abs(int), dbl)` not working? It should work.
--
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
[GitHub] [arrow] thisisnic commented on a change in pull request #10519: ARROW-12867: [R] Bindings for abs()
Posted by GitBox <gi...@apache.org>.
thisisnic commented on a change in pull request #10519:
URL: https://github.com/apache/arrow/pull/10519#discussion_r652529386
##########
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:
I was thinking about this myself, and am wondering if that could be achieved via a function we define purely for the purpose of running this test, called something ridiculous like `not_a_function` or similar so it won't overlap with anything real?
--
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
[GitHub] [arrow] thisisnic commented on pull request #10519: ARROW-12867: [R] Bindings for abs()
Posted by GitBox <gi...@apache.org>.
thisisnic commented on pull request #10519:
URL: https://github.com/apache/arrow/pull/10519#issuecomment-860526967
Nice, thanks for submitting this PR! There is a test failure caused by the calls to `abs()` on lines 146 and 149 in `test-dplyr-arrange.R` - the test they're part of previously was expected to result in a warning message as the function `abs` was not supported, but now you've added this functionality, no warning is issued.
--
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
[GitHub] [arrow] AlenkaF commented on a change in pull request #10519: ARROW-12867: [R] Bindings for abs()
Posted by GitBox <gi...@apache.org>.
AlenkaF commented on a change in pull request #10519:
URL: https://github.com/apache/arrow/pull/10519#discussion_r654745906
##########
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:
Thank you for looking into this @ianmcook. `arrange(abs(int), dbl)` in the test works. But it looks as if this part of the test is trying to confirm a warning and in this case `abs()` should be changed?
--
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
[GitHub] [arrow] AlenkaF commented on a change in pull request #10519: ARROW-12867: [R] Bindings for abs()
Posted by GitBox <gi...@apache.org>.
AlenkaF commented on a change in pull request #10519:
URL: https://github.com/apache/arrow/pull/10519#discussion_r652435928
##########
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:
Looking into `dplyr-arrange.R` I would agree that one test needs to be left as there is a functionality in `arrange()` to check for things not supported by Arrow and this seems to be the only case testing it.
https://github.com/apache/arrow/blob/f8661e032902a963b0a6a46077d72e804d22560d/r/R/dplyr-arrange.R#L39-L46
And so it doesn't make sense to keep `abs()` as it is now supported and it was intended to test for `"try-error"` by `arrow_eval()`.
https://github.com/apache/arrow/blob/f8661e032902a963b0a6a46077d72e804d22560d/r/R/dplyr-eval.R#L18-L48
We could change `abs()` to for example `ceiling()`. But if it will ever be supported in Arrow this will have to be changed again? And I am not sure how to find a bulletproof function.
--
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
[GitHub] [arrow] jonkeane closed pull request #10519: ARROW-12867: [R] Bindings for abs()
Posted by GitBox <gi...@apache.org>.
jonkeane closed pull request #10519:
URL: https://github.com/apache/arrow/pull/10519
--
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
[GitHub] [arrow] thisisnic commented on pull request #10519: ARROW-12867: [R] Bindings for abs()
Posted by GitBox <gi...@apache.org>.
thisisnic commented on pull request #10519:
URL: https://github.com/apache/arrow/pull/10519#issuecomment-860792424
Awesome, thanks! Another really minor change to suggest: the approach to your unit tests is great; however, there's a helper function in the Arrow package called `expect_dplyr_equal`. It takes an R expression and a tibble as inputs, and then directly compares the results of executing the `arrow` version with the results of executing the `dplyr` version. The benefit of using this function is that it will compare how things work both on Arrow tables *and* Arrow record batches at the same time.
You can see it in action here: https://github.com/apache/arrow/blob/b81fcf73ee7722147868e94f0cc1040f7eb51c79/r/tests/testthat/test-dplyr-mutate.R#L37-L46
The code for the function is here (though you don't have to read it to be able to use it!): https://github.com/apache/arrow/blob/b81fcf73ee7722147868e94f0cc1040f7eb51c79/r/tests/testthat/helper-expectation.R#L73-L106
Please could you update the test you added to use `expect_dplyr_equal` instead of `expect_equal`? It took me a couple of tries to fully understand how `expect_dplyr_equal` works, so if you have any questions, let me know! :)
--
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
[GitHub] [arrow] AlenkaF commented on pull request #10519: ARROW-12867: [R] Bindings for abs()
Posted by GitBox <gi...@apache.org>.
AlenkaF commented on pull request #10519:
URL: https://github.com/apache/arrow/pull/10519#issuecomment-860853969
Sure, I am happy to do it. Thank you so much @thisisnic! Will let you know if I get stuck ;)
--
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
[GitHub] [arrow] thisisnic commented on a change in pull request #10519: ARROW-12867: [R] Bindings for abs()
Posted by GitBox <gi...@apache.org>.
thisisnic commented on a change in pull request #10519:
URL: https://github.com/apache/arrow/pull/10519#discussion_r654538420
##########
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:
Hi @AlenkaF - I think I owe you an apology!
What I had in mind was a more general way of testing that functions that shouldn't work in the dplyr pipelines, and the code example I sketched out myself uses `mutate` and a function similar to yours there but which results in an error.
However, looking at the tests for `mutate`, this already seems to exist and just uses a function that we don't implement all the arguments for. https://github.com/apache/arrow/blob/f1e3c2e557bb3622b203fb270bf0329f53f1e617/r/tests/testthat/test-dplyr-mutate.R#L121-L137
I don't think this actually can be done in `arrange` as `desc` is already implemented and as far as I'm aware that's the only function you can supply to `arrange` to change the ordering.
Sorry about that, I appreciate you've spent time looking into this!
--
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
[GitHub] [arrow] thisisnic commented on pull request #10519: ARROW-12867: [R] Bindings for abs()
Posted by GitBox <gi...@apache.org>.
thisisnic commented on pull request #10519:
URL: https://github.com/apache/arrow/pull/10519#issuecomment-861406893
@AlenkaF Thanks for that update, looks good to me!
@jonkeane - please can you take a look at this for a final review, in case I've missed anything? Thank you!
--
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