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/03/10 23:23:43 UTC
[GitHub] [arrow] boshek opened a new pull request #9674: ARROW-11925 [R]: Add `between` method for arrow_dplyr_query
boshek opened a new pull request #9674:
URL: https://github.com/apache/arrow/pull/9674
Closes https://issues.apache.org/jira/browse/ARROW-11925
----------------------------------------------------------------
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] boshek commented on a change in pull request #9674: ARROW-11925 [R]: Add `between` method for arrow_dplyr_query
Posted by GitBox <gi...@apache.org>.
boshek commented on a change in pull request #9674:
URL: https://github.com/apache/arrow/pull/9674#discussion_r594665675
##########
File path: r/tests/testthat/test-dplyr-filter.R
##########
@@ -155,6 +155,59 @@ test_that("filter() with %in%", {
)
})
+test_that("filter() with between()", {
+ expect_dplyr_equal(
+ input %>%
+ filter(between(dbl, 1, 2)) %>%
+ collect(),
+ tbl
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ filter(between(dbl, 0.5, 2)) %>%
+ collect(),
+ tbl
+ )
+
+ expect_dplyr_equal(
Review comment:
Ok tests passing now. A few iterations to figure out your test suite 🤕 .
----------------------------------------------------------------
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] boshek commented on a change in pull request #9674: ARROW-11925 [R]: Add `between` method for arrow_dplyr_query
Posted by GitBox <gi...@apache.org>.
boshek commented on a change in pull request #9674:
URL: https://github.com/apache/arrow/pull/9674#discussion_r592569210
##########
File path: r/tests/testthat/test-dplyr-filter.R
##########
@@ -155,6 +155,15 @@ test_that("filter() with %in%", {
)
})
+test_that("filter() with between()", {
+ expect_dplyr_equal(
+ input %>%
+ filter(between(dbl, 1, 2)) %>%
+ collect(),
+ tbl
+ )
+})
+
Review comment:
Great points. In terms of vector-wise comparisons I think that is a great idea. However given the original dplyr method doesn't support that, does it makes sense to add that to arrow? Honest question.
I'm struggling a bit with the input validation for two reasons (aside from my limited understanding). First I am not totally clear how to check the type of a field in a `FileSystemDataset` object. If I add something like this into `between`:
```r
if (!is.double(x)) {
x <- as.numeric(x)
}
```
I get this error presumably because it is prematurely trying to bring the data into R
> Error: Filter expression not supported for Arrow Datasets: between(x, 0.5, 1.4)
Call collect() first to pull data into R.
Is there a way to check for types that I am missing?
Second, `dplyr::between` allows this:
```r
iris %>%
filter(between(Sepal.Length, 3, "5"))
```
That seems.... weird? IMO `arrow` actually handles this better with a more informative error message:
```r
open_dataset("foo") %>%
filter(between(x, 0.5, "5")) %>%
collect()
Error: NotImplemented: Function less_equal has no kernel matching input types (array[double], scalar[string])
```
Probably super simple questions but I am struggling with how to enforce the input within the function with `FileSystemDataset` objects, particular with these direct R mapping type ones. Once I have that figured out then I think I can sort out the validation test.
----------------------------------------------------------------
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] nealrichardson commented on a change in pull request #9674: ARROW-11925 [R]: Add `between` method for arrow_dplyr_query
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9674:
URL: https://github.com/apache/arrow/pull/9674#discussion_r593532623
##########
File path: r/tests/testthat/test-dplyr-filter.R
##########
@@ -155,6 +155,59 @@ test_that("filter() with %in%", {
)
})
+test_that("filter() with between()", {
+ expect_dplyr_equal(
+ input %>%
+ filter(between(dbl, 1, 2)) %>%
+ collect(),
+ tbl
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ filter(between(dbl, 0.5, 2)) %>%
+ collect(),
+ tbl
+ )
+
+ expect_dplyr_equal(
Review comment:
This is your failing test: because `dplyr` doesn't support the vector-wise comparison. You can rewrite it without `expect_dplyr_equal`, just do something like:
```
expect_identical(
tbl %>%
record_batch() %>%
filter(between(dbl, int, dbl2)) %>%
collect(),
tbl %>%
filter(dbl >= int, dbl <= dbl2)
)
```
or something like that.
----------------------------------------------------------------
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] nealrichardson commented on a change in pull request #9674: ARROW-11925 [R]: Add `between` method for arrow_dplyr_query
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9674:
URL: https://github.com/apache/arrow/pull/9674#discussion_r594443332
##########
File path: r/tests/testthat/test-dplyr-filter.R
##########
@@ -155,6 +155,59 @@ test_that("filter() with %in%", {
)
})
+test_that("filter() with between()", {
+ expect_dplyr_equal(
+ input %>%
+ filter(between(dbl, 1, 2)) %>%
+ collect(),
+ tbl
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ filter(between(dbl, 0.5, 2)) %>%
+ collect(),
+ tbl
+ )
+
+ expect_dplyr_equal(
Review comment:
You should just use regular `expect_error` here since the arrow version behaves slightly differently (even when the dplyr version and this version both error, they give different validation messages). `expect_dplyr_error` asserts that they give the exact same error message: https://github.com/apache/arrow/blob/master/r/tests/testthat/helper-expectation.R#L98-L124
----------------------------------------------------------------
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] nealrichardson commented on a change in pull request #9674: ARROW-11925 [R]: Add `between` method for arrow_dplyr_query
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9674:
URL: https://github.com/apache/arrow/pull/9674#discussion_r591969663
##########
File path: r/tests/testthat/test-dplyr-filter.R
##########
@@ -155,6 +155,15 @@ test_that("filter() with %in%", {
)
})
+test_that("filter() with between()", {
+ expect_dplyr_equal(
+ input %>%
+ filter(between(dbl, 1, 2)) %>%
+ collect(),
+ tbl
+ )
+})
+
Review comment:
Should also add a test for the input validation.
When you do, one question: what happens if you remove the input validation and let the Arrow C++ library handle it? Are the error messages clear enough?
Removing the R validation would allow `left` and `right` to be other kinds of objects (such as Arrow objects). It would also support vector-wise comparison (i.e. if `left` and `right` were columns in the data), which might be nice.
##########
File path: r/R/dplyr.R
##########
@@ -299,6 +299,15 @@ build_function_list <- function(FUN) {
both = FUN("utf8_trim_whitespace", string)
)
},
+ between = function(x, left, right) {
+ if (length(left) != 1) {
+ rlang::abort("`left` must be length 1")
Review comment:
We import `abort` from `rlang` so you can drop `rlang::`
----------------------------------------------------------------
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] nealrichardson commented on a change in pull request #9674: ARROW-11925 [R]: Add `between` method for arrow_dplyr_query
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9674:
URL: https://github.com/apache/arrow/pull/9674#discussion_r592785778
##########
File path: r/tests/testthat/test-dplyr-filter.R
##########
@@ -155,6 +155,15 @@ test_that("filter() with %in%", {
)
})
+test_that("filter() with between()", {
+ expect_dplyr_equal(
+ input %>%
+ filter(between(dbl, 1, 2)) %>%
+ collect(),
+ tbl
+ )
+})
+
Review comment:
Yeah, that's why I was suggesting that you consider removing the validation code and seeing how the Arrow C++ library handles bad inputs. If the arrow error messages are clear enough (looks like they may be, though they're slightly non-obvious because of how `between` is translated into expressions), it may be worthwhile to let the C++ library handle what it can support and tell us if it can't, rather than try to validate twice.
I believe that a side effect of removing the validation is that it would allow the vector-wise operation--which you're right, it's a fair question as to whether it should be supported, but if we get it for free, it doesn't seem unreasonable to add. But IMO the main reason to remove the validation would be to avoid some of the complexity of type-checking arrow expressions that you experienced. You definitely can't type-check `x` if that's supposed to be a field in the dataset; it would be possible to check its type in the dataset's schema if it exists there (though the usual R functions like `is.numeric` and `as.numeric` etc. won't make sense), but because `x` could be the result of a previous `mutate()` step, we wouldn't know until it's evaluated what type it will be.
I'd suggest adding a bunch of tests with garbage outputs ("QA engineer walks into a bar and orders a beer. Orders -1 beers. Orders 0.99 beers. Orders NULL beer. etc.") and see how the C++ library handles them. If it's good enough, I'd drop the R validation.
----------------------------------------------------------------
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 #9674: ARROW-11925 [R]: Add `between` method for arrow_dplyr_query
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9674:
URL: https://github.com/apache/arrow/pull/9674#issuecomment-796280212
https://issues.apache.org/jira/browse/ARROW-11925
----------------------------------------------------------------
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] nealrichardson closed pull request #9674: ARROW-11925 [R]: Add `between` method for arrow_dplyr_query
Posted by GitBox <gi...@apache.org>.
nealrichardson closed pull request #9674:
URL: https://github.com/apache/arrow/pull/9674
----------------------------------------------------------------
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] nealrichardson commented on a change in pull request #9674: ARROW-11925 [R]: Add `between` method for arrow_dplyr_query
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9674:
URL: https://github.com/apache/arrow/pull/9674#discussion_r594440421
##########
File path: r/tests/testthat/test-dplyr-filter.R
##########
@@ -155,6 +155,59 @@ test_that("filter() with %in%", {
)
})
+test_that("filter() with between()", {
+ expect_dplyr_equal(
+ input %>%
+ filter(between(dbl, 1, 2)) %>%
+ collect(),
+ tbl
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ filter(between(dbl, 0.5, 2)) %>%
+ collect(),
+ tbl
+ )
+
+ expect_dplyr_equal(
Review comment:
Looks like fixing that let the next tests run, and some of them fail 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
[GitHub] [arrow] boshek commented on a change in pull request #9674: ARROW-11925 [R]: Add `between` method for arrow_dplyr_query
Posted by GitBox <gi...@apache.org>.
boshek commented on a change in pull request #9674:
URL: https://github.com/apache/arrow/pull/9674#discussion_r593477389
##########
File path: r/tests/testthat/test-dplyr-filter.R
##########
@@ -155,6 +155,15 @@ test_that("filter() with %in%", {
)
})
+test_that("filter() with between()", {
+ expect_dplyr_equal(
+ input %>%
+ filter(between(dbl, 1, 2)) %>%
+ collect(),
+ tbl
+ )
+})
+
Review comment:
I've gone ahead and actually removed all input validation as you suggested. This gives us a vectorized `between` which I've added some tests form. IMO the Arrow C++ error messages are fine here. I had to add another dbl column to the test data to directly test the vectorized operation. Then I just tried to think of some reasonable use cases and test for those.
----------------------------------------------------------------
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