LiaCastaneda commented on code in PR #27099:
URL: https://github.com/apache/datafusion/pull/27099#discussion_r4524447382
##########
datafusion/substrait/src/logical_plan/consumer/expr/scalar_function.rs:
##########
@@ -76,26 +79,75 @@ pub async fn from_scalar_function(
Ok(expr.resolve_lambda_variables(input_schema)?.data)
}
} else if let Ok(func) = udf_func {
- Ok(Expr::ScalarFunction(expr::ScalarFunction::new_udf(
- func.to_owned(),
- args,
- )))
+ build_naming_struct_literals_if_needed(args, input_schema, |args| {
+ Ok(Expr::ScalarFunction(expr::ScalarFunction::new_udf(
+ func.to_owned(),
+ args,
+ )))
+ })
} else if let Some(op) = name_to_op(fn_name) {
if args.len() < 2 {
return not_impl_err!(
"Expect at least two arguments for binary operator {op:?}, the
provided number of operators is {:?}",
f.arguments.len()
);
}
+ // an operator always combines its operands, so they need the same
field names
+ name_struct_literals_like_siblings(
+ &mut args.iter_mut().collect::<Vec<_>>(),
+ input_schema,
+ );
// In those cases we build a balanced tree of BinaryExprs
arg_list_to_binary_op_tree(op, args)
} else if let Some(builder) = BuiltinExprBuilder::try_from_name(fn_name) {
+ // these combine their arguments too, as in `s BETWEEN low AND high`
+ name_struct_literals_like_siblings(
+ &mut args.iter_mut().collect::<Vec<_>>(),
+ input_schema,
+ );
builder.build(consumer, f, args)
} else {
not_impl_err!("Unsupported function name: {fn_name:?}")
}
}
+/// Builds a function call with `build`, renaming its struct literals after a
same-shaped
+/// argument only if the call doesn't type-check without it.
+///
+/// `coalesce(s, [])` needs one type for `s: List<Struct{a, b}>` and the
literal's
+/// `List<Struct{c0, c1}>`, so the literal takes `s`'s names.
`named_struct('left', s,
+/// 'right', {c0: 42})` type-checks as is, and renaming would break a later
+/// `get_field(..., 'c0')`, so the literal is left alone. Function signatures
can't tell
+/// these apart, hence the check; operators always combine their operands and
always
+/// rename.
+fn build_naming_struct_literals_if_needed(
Review Comment:
This function checks whether the function accepts its input types as they
are (by calling `get_type`, which runs the function's own type coercion), and
based on that we decide whether to rename or not. I know this feels a bit
tricky, but the alternatives aren't cleaner or more reliable:
- Rely on the signature: not reliable, `named_struct` is `variadic_any`, but
coalesce and many UDFs are `user_defined`, which doesn't tell us whether the
arguments need a common type. And `cast_to_type(s, <struct literal>)` is
coercible, but renaming its literal would change the cast target.
- Hardcode the list of functions that don't need a common type: not
reliable (it doesn't cover custom UDFs) and not clean (it has to be kept up to
date).
There are other places in DF that also decide based on whether type checking
succeeds, for example the [SQL planner for
`NOT`](https://github.com/apache/datafusion/blob/9e0a08fd8627e5423f2b09b0ca13a5015b7cadee/datafusion/sql/src/expr/unary_op.rs#L35)
--
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]
