alamb commented on code in PR #14440:
URL: https://github.com/apache/datafusion/pull/14440#discussion_r1963481027
##########
datafusion/expr-common/src/signature.rs:
##########
@@ -466,6 +551,186 @@ fn get_data_types(native_type: &NativeType) ->
Vec<DataType> {
}
}
+/// Represents type coercion rules for function arguments, specifying both the
desired type
+/// and optional implicit coercion rules for source types.
+///
+/// # Examples
+///
+/// ```
+/// use datafusion_expr_common::signature::{Coercion, TypeSignatureClass};
+/// use datafusion_common::types::{NativeType, logical_binary, logical_string};
+///
+/// // Exact coercion that only accepts timestamp types
+/// let exact = Coercion::new_exact(TypeSignatureClass::Timestamp);
+///
+/// // Implicit coercion that accepts string types but can coerce from binary
types
+/// let implicit = Coercion::new_implicit(
+/// TypeSignatureClass::Native(logical_string()),
+/// vec![TypeSignatureClass::Native(logical_binary())],
+/// NativeType::String
+/// );
+/// ```
+///
+/// There are two variants:
+///
+/// * `Exact` - Only accepts arguments that exactly match the desired type
+/// * `Implicit` - Accepts the desired type and can coerce from specified
source types
+#[derive(Debug, Clone, Eq, PartialOrd)]
+pub enum Coercion {
+ /// Coercion that only accepts arguments exactly matching the desired type.
+ Exact {
+ /// The required type for the argument
+ desired_type: TypeSignatureClass,
+ },
+
+ /// Coercion that accepts the desired type and can implicitly coerce from
other types.
+ Implicit {
+ /// The primary desired type for the argument
+ desired_type: TypeSignatureClass,
+ /// Rules for implicit coercion from other types
+ implicit_coercion: ImplicitCoercion,
+ },
Review Comment:
> > It is not a step forward. We removed ability for a function f(s) to
express the most basic need: "f takes s being a type X, so the call succeeds
for any type X' coercible to X"
> > This is, i believe, what the `TypeSignature::Coercible(logical type)`
was doing. (At least this is what it should be doing by the name of it.)
>
> @findepi This was the behavior before DataFusion 43. Starting in
DataFusion 43, this was no longer the case. My old PR #14268 attempted to fix
this regression. I agree that the naming does not match the behavior, and there
is an extensive discussion in the PR I linked regarding that.
>
> @jayzhan211 @alamb @findepi The relevant comment from my old PR: [#14268
(comment)](https://github.com/apache/datafusion/pull/14268#issuecomment-2626079378).
>
> With that in mind, this PR is still a significant step forward from the
new behavior introduced in DataFusion 43. However, more work is needed to
define functions that are explicitly documented, have clear and consistent
naming, and sufficiently meet internal expectations while respecting system
contracts and remaining flexible for downstream use cases.
THanks @shehabgamin -- the writeup makes sense to me at a high level but I
can't fully understand the practical implications / usecase.
In the past when we have had such issues, one thing we have done is to write
up an example / usecase in
https://github.com/apache/datafusion/tree/main/datafusion-examples that tries
to show what is needed.
Perhaps in this case, someone could add an example to
https://github.com/apache/datafusion/blob/main/datafusion-examples/examples/advanced_udf.rs
that shows how to create a function with the desired coercion behavior (or if
that is not possible, shows why not)
I think that would make the principles of the [#14268
(comment)](https://github.com/apache/datafusion/pull/14268#issuecomment-2626079378)
clearer, at least to me, and we would have a specific example to evaluate
potential additional improvements against
--
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]
