Further refactoring of type coercion function code #19603
Conversation
github-actions
bot
added
logical-expr
| } | ||
| } | ||
|
|
||
| /// Performs type coercion for scalar function arguments. |
There was a problem hiding this comment.
Reorganizing these so the only public function of this file is near the top (fields_with_udf())
| /// (losslessly converted) into a value of `type_to` | ||
| /// | ||
| /// See the module level documentation for more detail on coercion. | ||
| #[deprecated(since = "52.0.0", note = "Unused internal function")] |
There was a problem hiding this comment.
This was only ever used in a test; trying to minimize our API surface area for simplicity so deprecating this
| Expr::ScalarFunction(_) | ||
| | Expr::WindowFunction(_) | ||
| | Expr::AggregateFunction(_) => { | ||
| Ok(self.to_field(schema)?.1.data_type().clone()) |
There was a problem hiding this comment.
Reusing existing logic from to_field() implementation
|
|
||
| /// Verify that function is invoked with correct number and type of arguments as | ||
| /// defined in `TypeSignature`. | ||
| fn verify_function_arguments<F: UDFCoercionExt>( |
There was a problem hiding this comment.
With the new UDFCoercionExt trait we can now more easily share common code between scalar, aggregate and window UDFs
| /// | ||
| /// Otherwise, returns an error if there's a type mismatch between | ||
| /// the window function's signature and the provided arguments. | ||
| fn data_type_and_nullable_with_window_function( |
There was a problem hiding this comment.
This has been inlined to to_field()
| #[deprecated(since = "52.0.0", note = "Internal function")] | ||
| pub fn generate_signature_error_msg( |
There was a problem hiding this comment.
Similarly removing this from our public API since I see no need for it to be public
| pub fn new() -> Self { | ||
| Self { | ||
| signature: Signature::one_of( | ||
| vec![TypeSignature::Nullary, TypeSignature::UserDefined], |
There was a problem hiding this comment.
I don't really like the idea of having UserDefined nested within a OneOf signature, so amending some functions which are doing this. Ideally a function declaring UserDefined should handle all of its coercion logic itself, instead of mixing this custom logic with the other signature API.
| expr::WindowFunctionDefinition::WindowUDF(udf) => { | ||
| coerce_arguments_for_signature(args, self.schema, udf.as_ref())? |
There was a problem hiding this comment.
This arm for coercing non-aggregate window UDFs actually was missing; adding this back in lead to discovering some UDF window tests were incorrect (see next comment)
| fn new(test_state: Arc<TestState>) -> Self { | ||
| let signature = | ||
| Signature::exact(vec![DataType::Float64], Volatility::Immutable); | ||
| Signature::exact(vec![DataType::Int64], Volatility::Immutable); |
There was a problem hiding this comment.
This sample UDF declared it accepted f64's but internally it acted as if input was i64; this was never caught because apparently type coercion wasn't being run on non-aggregate window functions, and the input data was already i64. Nice little fix.
| TypeSignature::Variadic(valid_types) => valid_types | ||
| .iter() | ||
| .map(|valid_type| current_types.iter().map(|_| valid_type.clone()).collect()) | ||
| .map(|valid_type| vec![valid_type.clone(); current_types.len()]) |
2 participants
Which issue does this PR close?
type_coercion/functions.rs#19518Rationale for this change
Found lots of code duplicated here, so unifying them.
What changes are included in this PR?
UDFCoercionExttrait which unifies some of their behaviours (introduced by Refactor duplicate code intype_coercion/functions.rs#19518)can_coerce_from()andgenerate_signature_error_msg()to work towards minimizing our public API surfaceUserDefinedwithinOneOfAre these changes tested?
Existing tests.
Are there any user-facing changes?
Deprecated some functions.