New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement if_else #241
Implement if_else #241
Conversation
d1d4427
to
2e6dabd
Compare
tests/testthat/test-pkg-arrow.R
Outdated
skip_if_not(has_arrow_with_substrait()) | ||
|
||
expect_equal( | ||
example_data[1:5, "dbl"] %>% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When writing new tests, it would be nice to move away from example_data
(so that when I'm reviewing these tests I don't have to remember what example_data
to know if the expected answer is correct).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I quite like having the same dataset to reuse across multiple tests. I guess it could make sense to leave it exclusively for the compare_dplyr_bindings()
functions, where you don't need to know what the dataset looks like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a good rule of thumb...those tests should pass no matter what example_data
is (provided that it has a reasonable type coverage) so it doesn't help much when reading the tests to recreate a dataset every time.
R/pkg-arrow.R
Outdated
@@ -259,6 +259,20 @@ arrow_funs[[">="]] <- function(lhs, rhs) { | |||
) | |||
} | |||
|
|||
arrow_funs[["if_else"]] <- function(condition, true, false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After your previous PR, you can put this in substrait_funs
since it's the same for arrow and duckdb, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed
447c2e0
to
0662fae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Fixes #156
There is an issue with return type in DuckDB with logical values, but it won't affect the TPC-H query calculations, so I was going to leave that as something to come back to.