nealrichardson commented on a change in pull request #11133:
URL: https://github.com/apache/arrow/pull/11133#discussion_r706430015
##########
File path: r/R/dplyr-functions.R
##########
@@ -694,6 +694,10 @@ nse_funcs$wday <- function(x, label = FALSE, abbr = TRUE,
week_start = getOption
}
nse_funcs$log <- nse_funcs$logb <- function(x, base = exp(1)) {
+ if (inherits(base, "Expression")) {
+ return(Expression$create("logb_checked", x, base))
+ }
+
Review comment:
Should we assert anything about `base` here? Length 1, numeric? (If
length(base) > 1, the we'll get more warnings about `if()` getting more than
one condition.)
##########
File path: r/tests/testthat/test-dplyr.R
##########
@@ -1035,10 +1035,20 @@ test_that("log functions", {
df
)
- expect_error(
- nse_funcs$log(Expression$scalar(x), base = 5),
- "`base` values other than exp(1), 2 and 10 not supported by Arrow",
- fixed = TRUE
+ expect_dplyr_equal(
+ input %>%
+ mutate(y = log(x, base = 5)) %>%
+ collect(),
+ df
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ # suppress 'NaNs produced' warning on the first row of df
+ # that evaluates to NaN (R raises warning but Arrow does not)
+ suppressWarnings(mutate(., y = log(x, base = x))) %>%
Review comment:
Looks like the old Windows build failed--perhaps the magrittr
improvement that supports this is too new for 3.6 binary packages. So maybe the
best option is to move the suppressWarnings and the accompanying comment to be
around/outside of expect_dplyr_equal().
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
