Skip to content

Saner handling of nulls inside arrays #15149

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Apr 29, 2025
Merged

Saner handling of nulls inside arrays #15149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1f1e725
Saner handling of nulls inside arrays
joroKr21 Mar 11, 2025
7a03235
Fix array_sort for empty record batch
joroKr21 Mar 13, 2025
c224126
Fix get_valid_types for FixedSizeLists
joroKr21 Mar 16, 2025
1f518c6
Optimize array_ndims
joroKr21 Mar 16, 2025
86d7167
Add a test for result type of Concatenating Mixed types
joroKr21 Mar 16, 2025
d3a8818
Fix array_element of empty array
joroKr21 Mar 16, 2025
3e8f005
Handle more FixedSizeLists
joroKr21 Mar 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 43 additions & 14 deletions datafusion/expr-common/src/signature.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -843,14 +843,16 @@ impl Signature {
volatility,
}
}

/// Any one of a list of [TypeSignature]s.
pub fn one_of(type_signatures: Vec<TypeSignature>, volatility: Volatility) -> Self {
Signature {
type_signature: TypeSignature::OneOf(type_signatures),
volatility,
}
}
/// Specialized Signature for ArrayAppend and similar functions

/// Specialized [Signature] for ArrayAppend and similar functions.
pub fn array_and_element(volatility: Volatility) -> Self {
Signature {
type_signature: TypeSignature::ArraySignature(
Expand All @@ -865,7 +867,41 @@ impl Signature {
volatility,
}
}
/// Specialized Signature for Array functions with an optional index

/// Specialized [Signature] for ArrayPrepend and similar functions.
pub fn element_and_array(volatility: Volatility) -> Self {
Signature {
type_signature: TypeSignature::ArraySignature(
ArrayFunctionSignature::Array {
arguments: vec![
ArrayFunctionArgument::Element,
ArrayFunctionArgument::Array,
],
array_coercion: Some(ListCoercion::FixedSizedListToList),
Copy link
Contributor

@thinkharderdev thinkharderdev Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbh I'm not entirely clear on how the coercion comes into play but an append to a fixed size list would still be a fixed size list right?

Copy link
Contributor Author

@joroKr21 joroKr21 Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a type signature PoV yes, it's possible to infer that the return type should also be a fixed-size list with size n + 1 but the implementation itself is not able to handle it:

pub(crate) fn array_append_inner(args: &[ArrayRef]) -> Result<ArrayRef> {
    let [array, _] = take_function_args("array_append", args)?;

    match array.data_type() {
        DataType::LargeList(_) => general_append_and_prepend::<i64>(args, true),
        _ => general_append_and_prepend::<i32>(args, true),
    }
}

It's unclear to me how an implementation for FixedSizeList would look like and I would say probably it's out of scope for this PR, it should be separate.

Copy link
Contributor

@jayzhan211 jayzhan211 Mar 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally if the list size is changed like (append operation) we will convert it to List, so append with fixed-size-list ends up with List

Copy link
Contributor

@jayzhan211 jayzhan211 Mar 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What we need is to provide customizable signature, so if they want List for append, they can add array_coercion: Some(ListCoercion::FixedSizedListToList), if they want to keep fixed-size-list they can add array_coercion: None. Both cases should be possible.

For datafusion implementation, we choose to convert to List by default

Copy link
Contributor Author

@joroKr21 joroKr21 Mar 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's why I'm setting array_coercion: Some(ListCoercion::FixedSizedListToList) by default 👍

},
),
volatility,
}
}

/// Specialized [Signature] for functions that take a fixed number of arrays.
pub fn arrays(
n: usize,
coercion: Option<ListCoercion>,
volatility: Volatility,
) -> Self {
Signature {
type_signature: TypeSignature::ArraySignature(
ArrayFunctionSignature::Array {
arguments: vec![ArrayFunctionArgument::Array; n],
array_coercion: coercion,
},
),
volatility,
}
}

/// Specialized [Signature] for Array functions with an optional index.
pub fn array_and_element_and_optional_index(volatility: Volatility) -> Self {
Signature {
type_signature: TypeSignature::OneOf(vec![
Expand All @@ -889,7 +925,7 @@ impl Signature {
}
}

/// Specialized Signature for ArrayElement and similar functions
/// Specialized [Signature] for ArrayElement and similar functions.
pub fn array_and_index(volatility: Volatility) -> Self {
Signature {
type_signature: TypeSignature::ArraySignature(
Expand All @@ -898,23 +934,16 @@ impl Signature {
ArrayFunctionArgument::Array,
ArrayFunctionArgument::Index,
],
array_coercion: None,
array_coercion: Some(ListCoercion::FixedSizedListToList),
},
),
volatility,
}
}
/// Specialized Signature for ArrayEmpty and similar functions

/// Specialized [Signature] for ArrayEmpty and similar functions.
pub fn array(volatility: Volatility) -> Self {
Signature {
type_signature: TypeSignature::ArraySignature(
ArrayFunctionSignature::Array {
arguments: vec![ArrayFunctionArgument::Array],
array_coercion: None,
},
),
volatility,
}
Signature::arrays(1, Some(ListCoercion::FixedSizedListToList), volatility)
}
}

Expand Down
Loading