nealrichardson commented on code in PR #13985:
URL: https://github.com/apache/arrow/pull/13985#discussion_r965298014
##########
r/R/expression.R:
##########
@@ -210,21 +213,20 @@ build_expr <- function(FUN,
}
if (FUN == "%in%") {
# Special-case %in%, which is different from the Array function name
+ value_set <- Array$create(args[[2]])
+ try(
+ value_set <- cast_or_parse(value_set, args[[1]]$type()),
+ silent = TRUE
+ )
+
expr <- Expression$create("is_in", args[[1]],
options = list(
- # If args[[2]] is already an Arrow object (like a scalar),
- # this wouldn't work
- value_set = Array$create(args[[2]]),
+ value_set = value_set,
skip_nulls = TRUE
)
)
} else {
- args <- lapply(args, function(x) {
- if (!inherits(x, "Expression")) {
- x <- Expression$scalar(x)
- }
- x
- })
+ args <- wrap_scalars(args, FUN)
Review Comment:
I went through the function list on
https://arrow.apache.org/docs/cpp/compute.html and evaluated whether you should
try to cast scalar inputs to the type of the corresponding column (and
remember, if you can't cast the scalar without loss of precision, it doesn't
add the cast, so for `int + 4.2`, 4.2 won't get cast to int so that will go to
`cast(int, float64) + 4.2` in Acero). For the non-unary scalar functions, all
but 4 make sense to try to convert scalars like this. The 4 functions that
don't are `binary_repeat`, `list_element`, `binary_join` (kind of an odd case,
which we don't use, we use binary_join_element_wise instead), and
`make_struct`. It's around 40-50 functions on the allow side, so it seems that
the "don't cast" functions are the exception.
Does that persuade you in favor of blocklist instead of allowlist?
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
