Jefffrey commented on code in PR #20012:
URL: https://github.com/apache/datafusion/pull/20012#discussion_r2744258297
##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -598,13 +598,32 @@ impl ExprSchemable for Expr {
)
})?;
- let arguments = args
+ let coerced_values: Vec<Option<ScalarValue>> = args
.iter()
- .map(|e| match e {
- Expr::Literal(sv, _) => Some(sv),
- _ => None,
+ .zip(new_fields.iter())
+ .map(|(expr, field)| {
+ let mut current_expr = expr;
+
+ // Loop to remove all casting layers
+ loop {
+ match current_expr {
+ Expr::Cast(cast) => current_expr = &cast.expr,
+ Expr::TryCast(cast) => current_expr =
&cast.expr,
+ _ => break,
+ }
+ }
+
+ let literal = if let Expr::Literal(sv, _) =
current_expr {
+ Some(sv)
+ } else {
+ None
+ };
+
+ literal.map(|sv|
sv.cast_to(field.data_type())).transpose()
Review Comment:
I think this fix makes sense with where it is located; this casting ensures
we have consistent input to `return_field_from_args()` since we already get the
expected coerced types from `fields_with_udf()` above.
My only concern is this loop to remove the casting layers 🤔
Maybe we are better off not having this and just keeping the existing
behaviour of considering only the raw literals? And then in a followup we can
look at trying to remove these casting layers at an earlier stage if possible.
I think that would keep the behaviour consistent as is (since currently we only
look for `Literals` without recursing inside).
Another thing that would be nice to have is wrapping the cast error in a
DataFusion Plan error, to make it more explicit it failed during planning
rather than only showing an ArrowError
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
