Align scalar UDF literal args with coerced return fields - #23232
Align scalar UDF literal args with coerced return fields#23232yinli-systems wants to merge 6 commits into
Conversation
Signed-off-by: Kevin-Li-2025 <2242139@qq.com>
github-actions
Bot
added
logical-expr
|
This seems to follow a similar approach to a previous PR that attempted this issue: There was some good discussion on there, perhaps can double check that since we found some difficulties with such an approach |
|
Thanks for pointing me back to #20012. I went through that discussion and tightened this PR so it does not take the broad approach that caused the earlier test churn. The latest commit narrows scalar literal recovery to the implicit-cast shape inserted by type coercion, Local checks run:
New CI is now queued on head |
github-actions
Bot
added
optimizer
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
looks like some tests are affected 🤔
| // A failed value cast remains an expression cast so execution produces the | ||
| // same error as before. Since it is no longer a literal at the coerced type, | ||
| // it is reported as `None` in `ReturnFieldArgs::scalar_arguments`. | ||
| Expr::Literal(value, metadata).cast_to(data_type, schema) |
There was a problem hiding this comment.
is this to maintain backwards compatibility?
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_fn_arrow_cast_requires_literal_type_arg() -> Result<()> { |
There was a problem hiding this comment.
we can probably just put this as an SLT which is less verbose
| if let Some(scalar) = args.scalar_arguments[0] { | ||
| assert_eq!(args.arg_fields[0].data_type(), &scalar.data_type()); | ||
| } |
There was a problem hiding this comment.
| if let Some(scalar) = args.scalar_arguments[0] { | |
| assert_eq!(args.arg_fields[0].data_type(), &scalar.data_type()); | |
| } | |
| assert_eq!(args.arg_fields[0].data_type(), &args.scalar_arguments[0].unwrap().data_type()); |
nit: we already assert it is_some() above
|
|
||
| fn scalar_argument_for_field(expr: &Expr, arg_field: &FieldRef) -> Option<ScalarValue> { | ||
| match expr { | ||
| Expr::Literal(sv, _) => sv.cast_to(arg_field.data_type()).ok(), |
There was a problem hiding this comment.
i suppose this is an easier way to ensure proper type without a wider refactor 🤔
| // Note: `scalar_arguments` contains the original literal values (pre-coercion), so | ||
| // integer literals may appear as Int64 even though the signature coerces them to Int32. | ||
| // `scalar_arguments` uses the coerced argument type, so an integer literal | ||
| // is Int32 here when the signature coerces it to Int32. |
There was a problem hiding this comment.
we can just remove the note here if we fix this behaviour, no need to explain it again (since scalar_arguments docstring should explain it)
| /// | ||
| /// When present, the scalar value has the same data type as the corresponding | ||
| /// entry in [`Self::arg_fields`], including after implicit type coercion. | ||
| /// User-written `Cast` and `TryCast` expressions are not scalar arguments. |
There was a problem hiding this comment.
| /// User-written `Cast` and `TryCast` expressions are not scalar arguments. |
this just seems confusing, can remove it
Signed-off-by: Kevin-Li-2025 <kxl474@student.bham.ac.uk>
2 participants
Which issue does this PR close?
ReturnFieldArgs.scalar_argumentstype doesn't match witharg_fields#19982.Rationale for this change
ReturnFieldArgsexposes both coercedarg_fieldsand optional scalar literal arguments. A coercible UDF could receive anInt16field but an originalInt64literal, or lose the literal behind an implicit cast. That makes return-field decisions inconsistent and forced UDF implementations to compensate for planner internals.What changes are included in this PR?
arg_fieldsalways have the same type.CastandTryCastexpressions as expressions rather than reporting them as literals.roundto use the strengthened contract and document it onReturnFieldArgs.arrow_cast's required literal type argument.This follows the approach discussed in #20012: materialize implicit literal coercion in the type-coercion pass without changing explicit-cast semantics.
Are these changes tested?
cargo fmt --all --checkcargo test -p datafusion --test user_defined_integration test_return_field_args_scalar_argument_types_match_arg_fields(1 passed)cargo test -p datafusion --test core_integration test_fn_arrow_cast_requires_literal_type_arg(1 passed)cargo test -p datafusion-expr scalar_arguments_(2 passed)cargo clippy --all-targets --all-features -- -D warningsgit diff --checkThe validation process caught and fixed two semantic regressions before push: explicit casts being folded into scalar literals, and
arrow_castreporting its existing error during planning rather than collection.AI assistance
AI tooling assisted with implementation and test iteration. I reviewed the final diff, the related #20012 design discussion, and the scalar-function coercion/return-field flow end to end, and ran the validations listed above.