paleolimbot commented on code in PR #13985:
URL: https://github.com/apache/arrow/pull/13985#discussion_r957286453
##########
r/R/expression.R:
##########
@@ -260,6 +255,52 @@ build_expr <- function(FUN,
expr
}
+wrap_scalars <- function(args, FUN) {
+ arrow_fun <- .array_function_map[[FUN]] %||% FUN
+ if (arrow_fun == "if_else") {
+ # For if_else, the first arg should be a bool Expression, and we don't
+ # want to consider that when casting the other args to the same type
+ args[-1] <- wrap_scalars(args[-1], FUN = "")
+ return(args)
+ }
+
+ is_expr <- map_lgl(args, ~ inherits(., "Expression"))
+ if (all(is_expr)) {
+ # No wrapping is required
+ return(args)
+ }
+
+ args[!is_expr] <- lapply(args[!is_expr], Scalar$create)
+
+ # Some special casing by function
Review Comment:
I'm not sure I like the idea of *another* place to remember to look to
figure out the behaviour of a binding. For bindings that require non-standard
logic I think it's better to use something like `register_binding("%/%",
function(x, y) { ... })`. With @dragosmg's user-defined binding PR it will be
easier to re-use bindings within bindings (which is sort of what we use
`build_expr()` for).
##########
r/R/expression.R:
##########
@@ -260,6 +255,52 @@ build_expr <- function(FUN,
expr
}
+wrap_scalars <- function(args, FUN) {
+ arrow_fun <- .array_function_map[[FUN]] %||% FUN
+ if (arrow_fun == "if_else") {
+ # For if_else, the first arg should be a bool Expression, and we don't
+ # want to consider that when casting the other args to the same type
+ args[-1] <- wrap_scalars(args[-1], FUN = "")
+ return(args)
+ }
+
+ is_expr <- map_lgl(args, ~ inherits(., "Expression"))
+ if (all(is_expr)) {
+ # No wrapping is required
+ return(args)
+ }
+
+ args[!is_expr] <- lapply(args[!is_expr], Scalar$create)
+
+ # Some special casing by function
+ # * %/%: we switch behavior based on int vs. dbl in R (see build_expr) so
skip
+ # * binary_repeat, list_element: 2nd arg must be integer, Acero will handle
it
+ if (any(is_expr) && !(arrow_fun %in% c("binary_repeat", "list_element")) &&
!(FUN %in% "%/%")) {
+ if (sum(is_expr) == 1) {
+ # Simple case: just one expr so take its type
+ try(
+ {
+ # If the Expression has no Schema embedded, we cannot resolve its
+ # type here, so this will error, hence the try() wrapping it
+ to_type <- args[[which(is_expr)]]$type()
+ # Try casting to this type, but if the cast fails,
+ # we'll just keep the original
+ args[!is_expr] <- lapply(args[!is_expr], function(x) x$cast(to_type))
+ },
+ silent = TRUE
+ )
+ } else {
+ # TODO: check if all expression types are the same, and if so, cast to
that
+ # Functions that exercise code that go through here (in our tests):
+ # * case_when
+ # * pmin/pmax
Review Comment:
I wonder if this would benefit from its own function (maybe
`expression_cast_common()` to mirror `vctrs::vec_cast_common()`).
##########
r/R/expression.R:
##########
@@ -260,6 +255,52 @@ build_expr <- function(FUN,
expr
}
+wrap_scalars <- function(args, FUN) {
+ arrow_fun <- .array_function_map[[FUN]] %||% FUN
+ if (arrow_fun == "if_else") {
+ # For if_else, the first arg should be a bool Expression, and we don't
+ # want to consider that when casting the other args to the same type
+ args[-1] <- wrap_scalars(args[-1], FUN = "")
+ return(args)
+ }
+
+ is_expr <- map_lgl(args, ~ inherits(., "Expression"))
+ if (all(is_expr)) {
+ # No wrapping is required
+ return(args)
+ }
+
+ args[!is_expr] <- lapply(args[!is_expr], Scalar$create)
+
+ # Some special casing by function
+ # * %/%: we switch behavior based on int vs. dbl in R (see build_expr) so
skip
+ # * binary_repeat, list_element: 2nd arg must be integer, Acero will handle
it
+ if (any(is_expr) && !(arrow_fun %in% c("binary_repeat", "list_element")) &&
!(FUN %in% "%/%")) {
+ if (sum(is_expr) == 1) {
+ # Simple case: just one expr so take its type
+ try(
+ {
+ # If the Expression has no Schema embedded, we cannot resolve its
+ # type here, so this will error, hence the try() wrapping it
+ to_type <- args[[which(is_expr)]]$type()
+ # Try casting to this type, but if the cast fails,
+ # we'll just keep the original
+ args[!is_expr] <- lapply(args[!is_expr], function(x) x$cast(to_type))
Review Comment:
Can/should this be built in to `Expression$cast()`?
--
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]
