Encapsulates logic from TypeSignatureClass, rename methods #4
Conversation
| @@ -405,7 +405,7 @@ fn get_udf_args_and_return_types( | |||
| udf: &Arc<ScalarUDF>, | |||
| ) -> Result<Vec<(Vec<String>, Option<String>)>> { | |||
| let signature = udf.signature(); | |||
| let arg_types = signature.type_signature.get_possible_types(); | |||
| let arg_types = signature.type_signature.get_example_types(); | |||
There was a problem hiding this comment.
renamed and deprecated get_possible_types to reflect what the function actually does
| /// | ||
| /// This is used for `information_schema` and can be used to generate | ||
| /// documentation or error messages. | ||
| fn get_example_types(&self) -> Vec<DataType> { |
There was a problem hiding this comment.
Moved logic that was related to TypeSignatureClass into methods
| @@ -429,32 +516,6 @@ impl TypeSignature { | |||
| } | |||
| } | |||
|
|
|||
| fn get_possible_types_from_signature_classes( | |||
| @@ -599,76 +596,19 @@ fn get_valid_types( | |||
|
|
|||
| let mut new_types = Vec::with_capacity(current_types.len()); | |||
| for (current_type, param) in current_types.iter().zip(param_types.iter()) { | |||
| let current_logical_type: NativeType = current_type.into(); | |||
| /// | ||
| /// This is used for `information_schema` and can be used to generate | ||
| /// documentation or error messages. | ||
| pub fn get_example_types(&self) -> Vec<Vec<DataType>> { |
There was a problem hiding this comment.
Can we make this private, it is not useful to make this public. I'm also thinking about other types than DataType
| } | ||
|
|
||
| /// Does the specified `NativeType` match this type signature class? | ||
| pub fn matches_native_type( |
There was a problem hiding this comment.
It would be nice to make this private
There was a problem hiding this comment.
Actually we can't make this private, that is why I didn't move it as a method, to keep us easy to change the logic
There was a problem hiding this comment.
It seems to me like the exact details of what types are coerced to what other types is actually part of the DataFusion public API because those rules effect what queries will run
Whenever we make changes to the coercsion rules, it seems to result in a bunch of issues reported by users (because some queries that used to work no longer do).
Thus I propose we move towards a design that is both more explicit (and extensible)
I realize we could probably achieve this goal without making more pub methods 🤔
| } | ||
|
|
||
| /// What type would `origin_type` be casted to when casting to the specified native type? | ||
| pub fn default_casted_type( |
There was a problem hiding this comment.
It would be nice to make this private
There was a problem hiding this comment.
As above, I don't fully understand the desire to keep it non pub
Proposed PR to apache#14440 from @jayzhan211
Will comment inline