ARROW-12184: [R] Bindings for na.fail, na.omit, na.exclude, na.pass #10056
Conversation
thisisnic
force-pushed
the
ARROW-12184-na.fail
branch
from
62be488 to
d07dab6
Compare
| @@ -189,3 +187,109 @@ expect_vector_equal <- function(expr, # A vectorized R expression containing `in | |||
| skip(paste(skip_msg, collpase = "\n")) | |||
| } | |||
| } | |||
|
|
|||
| expect_vector_equivalent <- function(expr, # A vectorized R expression containing `input` as its input | |||
There was a problem hiding this comment.
I think it's worthwhile to have this new helper function, but it would be better to implement it by adding an optional argument ignore_attributes = FALSE to expect_vector_equal(), using that to control whether expect_equivalent() or expect_equal() is used, and then calling that function from this one. That would reduce the amount of copied code.
You'll need to use !! to unquote expr in the call to expect_vector_equivalent() from here.
There was a problem hiding this comment.
(Please do push back if you think I'm mistaken about this or if I'm missing some important details!)
There was a problem hiding this comment.
Another argument for expect_vector_equal(ignore_attributes = TRUE) is that testthat 3e deprecates expect_equivalent() in favor of expect_equal(ignore_attr = TRUE). And if we can name the argument, I would vote for ignore_attr over ignore_attributes to match testthat (well, I guess technically it's waldo at that point).
There was a problem hiding this comment.
Agreed—best to name the attributeignore_attr 👍
There was a problem hiding this comment.
Also heads up that expect_vector_equal() calls a function named expect_vector() but this is not testthat::expect_vector(); it's just a simple function defined higher up in helper-expectation.R. That confused me initially.
There was a problem hiding this comment.
We can rename our expect_vector if that helps; I believe expect_vector is new in testthat (at least it's new to me).
There was a problem hiding this comment.
➕ for renaming our expect_vector to avoid any possible confusion.
In other places I've been bitten by these testing helper name overlaps, and each time it's happened it threw me for a loop trying to figure out what was going wrong until I realized it was a name collision in a helper.
There was a problem hiding this comment.
Have now updated the code here to implement ignore_attr. What makes sense as a new name for expect_vector? Maybe expect_as_vector or expect_vector_match?
There was a problem hiding this comment.
Gone with expect_as_vector for now!
jonkeane
left a comment
jonkeane
left a comment
There was a problem hiding this comment.
This is looking good. Most of my comments are super minor whitespace things. But two more substantive comments.
r/R/arrow-tabular.R
Outdated
| na.omit.ArrowTabular <- function(object, ...){ | ||
|
|
||
| na_expr <- paste0("!is.na(", names(object), ")", collapse = ",") | ||
| filter_expr <- paste0("dplyr::filter(object,", na_expr, ")") |
There was a problem hiding this comment.
Is it possible to implement this without dplyr? We only suggest dplyr, and it would be nice to not have to have it installed/loaded for na.omit() to work. This might not be the right approach, but my first thought: would it be possible to iterate objects and use the na.omit() method above? (or use the $Filter method you use above)?
There was a problem hiding this comment.
Ah, didn't realise it was only a suggest; I'll have a think!
| @@ -189,3 +187,109 @@ expect_vector_equal <- function(expr, # A vectorized R expression containing `in | |||
| skip(paste(skip_msg, collpase = "\n")) | |||
| } | |||
| } | |||
|
|
|||
| expect_vector_equivalent <- function(expr, # A vectorized R expression containing `input` as its input | |||
There was a problem hiding this comment.
Another argument for expect_vector_equal(ignore_attributes = TRUE) is that testthat 3e deprecates expect_equivalent() in favor of expect_equal(ignore_attr = TRUE). And if we can name the argument, I would vote for ignore_attr over ignore_attributes to match testthat (well, I guess technically it's waldo at that point).
thisisnic
force-pushed
the
ARROW-12184-na.fail
branch
from
8730001 to
2b5bc7a
Compare
nealrichardson
left a comment
nealrichardson
left a comment
There was a problem hiding this comment.
Overall this looks great. Two suggestions for how I think it can be even better, but not hugely important.
…pect_equivalent as failing due to attribute mismatch
…vector to use this arg
…reaky evaluation things
Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>
thisisnic
force-pushed
the
ARROW-12184-na.fail
branch
from
35f9b22 to
f113284
Compare
5 participants
No description provided.