jkosh44 commented on code in PR #14532:
URL: https://github.com/apache/datafusion/pull/14532#discussion_r1950225082
##########
datafusion/expr-common/src/signature.rs:
##########
@@ -227,25 +226,13 @@ impl Display for TypeSignatureClass {
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Hash)]
pub enum ArrayFunctionSignature {
- /// Specialized Signature for ArrayAppend and similar functions
- /// The first argument should be List/LargeList/FixedSizedList, and the
second argument should be non-list or list.
- /// The second argument's list dimension should be one dimension less than
the first argument's list dimension.
- /// List dimension of the List/LargeList is equivalent to the number of
List.
- /// List dimension of the non-list is 0.
- ArrayAndElement,
- /// Specialized Signature for ArrayPrepend and similar functions
- /// 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 Array functions of the form (List/LargeList,
Index+)
- /// The first argument should be List/LargeList/FixedSizedList, and the
next n arguments should be Int64.
- ArrayAndIndexes(NonZeroUsize),
- /// Specialized Signature for Array functions of the form (List/LargeList,
Element, Optional Index)
- ArrayAndElementAndOptionalIndex,
- /// 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.
- Array,
+ /// A function takes at least one List/LargeList/FixedSizeList argument.
+ Array {
+ /// A full list of the arguments accepted by this function.
+ arguments: Vec<ArrayFunctionArgument>,
+ /// Whether any of the input arrays are modified.
+ mutability: ArrayFunctionMutability,
Review Comment:
Ok, I think I'm starting to understand this better. We coerce the argument
types in `get_valid_types` in case we might use that argument type in the
return type. It is a bit confusing/unintuitive because we don't actually know
anything about the return type at this point, and the return type may have
nothing to do with the argument types For example `array_has` accepts an array
but just returns a bool.
One thing I'm still confused about is, what if we wanted to define a
function that accepted `FixedSizeList` and returned `List`, or vice versa? Is
that possible? Does it even make sense?
I agree with your other comment though, requiring function implementers to
coerce types in `return_type` is probably a footgun and will likely be
forgotten.
I think the best way forward is to revert
3a7c0e698e65141ca1fba913d0009db008a31902 and add back an enum that controls the
coercing of arguments. I'll have a think about the best way of doing that. It
feels a little bad that we'll require function implementers to implement
`return_type()` and provide an enum that describes how we should coerce types
for the return type, since they both are doing similar things. It seems like it
may be a source of confusion.
Also I noticed through some manual testing that this PR broke some function
calls with nested `FixedSizeList` so I'll add some test cases with that. For
example, `SELECT array_prepend(arrow_cast([1], 'FixedSizeList(1, Int64)'),
[arrow_cast([2], 'FixedSizeList(1, Int64)'), arrow_cast([3], 'FixedSizeList(1,
Int64)')]);` is broken in this PR.
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
