alamb commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1484833816
##########
datafusion/expr/src/signature.rs:
##########
@@ -132,7 +138,136 @@ pub enum ArrayFunctionSignature {
/// The first argument should be non-list or list, and the second argument
should be List/LargeList.
/// The first argument's list dimension should be one dimension less than
the second argument's list dimension.
ElementAndArray,
+ /// Specialized Signature for ArrayEqual and similar functions
ArrayAndIndex,
+ /// Specialized Signature for ArrayEmpty and similar functions
+ Array,
+}
+
+impl ArrayFunctionSignature {
+ /// Arguments to ArrayFunctionSignature
+ /// `current_types` - The data types of the arguments
+ /// `allow_null_coercion` - Whether null type coercion is allowed
+ /// Returns the valid types for the function signature
+ pub fn get_type_signature(
+ &self,
+ current_types: &[DataType],
+ allow_null_coercion: bool,
Review Comment:
since we are passing allow_null_coercion here, maybe it would make sense to
have allow_null_coercion to be part of each array enum? Perhaps something like
```rust
impl ArrayFunctionSignature {
ArrayAndElement(allow_null_coercion),
...
Array(allow_null_coercion)
}
```
But again I am not sure about the need for allow_null_coercion 🤔
##########
datafusion/expr/src/signature.rs:
##########
@@ -117,7 +122,8 @@ pub enum TypeSignature {
/// is `OneOf(vec![Any(0), VariadicAny])`.
OneOf(Vec<TypeSignature>),
/// Specifies Signatures for array functions
- ArraySignature(ArrayFunctionSignature),
+ /// Boolean value specifies whether null type coercion is allowed
+ ArraySignature(ArrayFunctionSignature, bool),
Review Comment:
Could you expand this comment to explain what null type coercion is and when
it would make sense *not*n to be allowed?
I don't understand what it is used for 🤔 it seems to me like null coercion
should always be allowed
##########
datafusion/expr/src/signature.rs:
##########
@@ -132,7 +138,136 @@ pub enum ArrayFunctionSignature {
/// The first argument should be non-list or list, and the second argument
should be List/LargeList.
/// The first argument's list dimension should be one dimension less than
the second argument's list dimension.
ElementAndArray,
+ /// Specialized Signature for ArrayEqual and similar functions
ArrayAndIndex,
+ /// Specialized Signature for ArrayEmpty and similar functions
Review Comment:
Can we explain what this is in more detail?
Does it mean something like
```suggestion
/// Specialized Signature for ArrayEmpty and similar functions
/// The function takes a single argument that must be a
List/LargeList/FixedSizeList
/// or something that can be coerced to one of those types
```
?
##########
datafusion/expr/src/signature.rs:
##########
@@ -132,7 +138,136 @@ pub enum ArrayFunctionSignature {
/// The first argument should be non-list or list, and the second argument
should be List/LargeList.
/// The first argument's list dimension should be one dimension less than
the second argument's list dimension.
ElementAndArray,
+ /// Specialized Signature for ArrayEqual and similar functions
ArrayAndIndex,
+ /// Specialized Signature for ArrayEmpty and similar functions
+ Array,
+}
+
+impl ArrayFunctionSignature {
+ /// Arguments to ArrayFunctionSignature
+ /// `current_types` - The data types of the arguments
+ /// `allow_null_coercion` - Whether null type coercion is allowed
+ /// Returns the valid types for the function signature
+ pub fn get_type_signature(
+ &self,
+ current_types: &[DataType],
+ allow_null_coercion: bool,
+ ) -> Result<Vec<Vec<DataType>>> {
+ fn array_append_or_prepend_valid_types(
Review Comment:
is there any reason to create these inner functions, rather than actual
functions (or maybe just inline the body of the functions into the `match`
statement below)?
I find the extra level of indenting somewhat confusing
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -80,73 +78,6 @@ fn get_valid_types(
signature: &TypeSignature,
current_types: &[DataType],
) -> Result<Vec<Vec<DataType>>> {
- fn array_append_or_prepend_valid_types(
Review Comment:
I see -- so one part of this PR is extracting this function into
ArrayFunctionSignature ?
--
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]
