Skip to content
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

fix: first none/empty list in ListArray panics in cast_with_options #7065

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix: first none/empty list in ListArray panics in cast_with_options #7065

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b61d054
fix: first none in `ListArray` panics in `cast_with_options`
irenjj Feb 2, 2025
7e79d9b
simplify
irenjj Feb 5, 2025
085c553
fix
irenjj Feb 6, 2025
7e0093e
Update arrow-cast/src/cast/list.rs
irenjj Feb 7, 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
22 changes: 13 additions & 9 deletions arrow-cast/src/cast/list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,11 +88,16 @@ where
let mut mutable = MutableArrayData::new(vec![&values], nullable, cap);
// The end position in values of the last incorrectly-sized list slice
let mut last_pos = 0;
let mut null_first = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe it's about nulls being first, rather about a potentially empty list being first. Consider this test case which also panics during cast:

    #[test]
    fn test456() {
        let array = Arc::new(ListArray::from_iter_primitive::<Int64Type, _, _>(vec![
            Some(vec![]),
            Some(vec![Some(1), Some(2)]),
        ])) as ArrayRef;
        let data_type =
            DataType::FixedSizeList(FieldRef::new(Field::new("item", DataType::Int64, true)), 2);
        let opt = CastOptions::default();
        let r = cast_with_options(&array, &data_type, &opt).unwrap();

        let fixed_array = Arc::new(FixedSizeListArray::from_iter_primitive::<Int64Type, _, _>(
            vec![None, Some(vec![Some(1), Some(2)])],
            2,
        )) as ArrayRef;
        assert_eq!(*fixed_array, *r);
    }

This distinction is important as nullability isn't determined by the gap between offsets, rather it's determined by the null buffer.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @Jefffrey for your review, in my local testing, null_first can identify both None and empty vec (the name null_first might be a bit misleading). My idea is to use null_first to prevent entering the let values = match last_pos { 0 => } situation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I understand what's happening now. It seems its an edge case of the algorithm used here, where a 0 sized list element precedes a list of the proper size, e.g.

Cast to fixed sized 2:
[
    [],
    [],
    [1, 2],
]

Where last_pos is never updated from 0 because end_pos for the first two elements is 0 and last_pos doesn't get updated when the list element already matches the fixed size to cast to.

(Sorry if this was already obvious, just needed to air my thoughts out)

Whilst this fix is correct, I'd like some more documentation, and especially rename null_first to something more accurate (as the problem is with empty size lists too), to explain this edge case so it's obvious to anyone else modifying the code here in the future.

We could also look into moving the:

if start_pos == 0 && end_pos == 0 {

Check outside of the for loop as we only need to check it once upfront.

for (idx, w) in array.offsets().windows(2).enumerate() {
let start_pos = w[0].as_usize();
let end_pos = w[1].as_usize();
let len = end_pos - start_pos;

if start_pos == 0 && end_pos == 0 {
null_first = true;
}

if len != size as usize {
if cast_options.safe || array.is_null(idx) {
if last_pos != start_pos {
Expand All @@ -112,16 +117,15 @@ where
}
}

let values = match last_pos {
0 => array.values().slice(0, cap), // All slices were the correct length
_ => {
if mutable.len() != cap {
// Remaining slices were all correct length
let remaining = cap - mutable.len();
mutable.extend(0, last_pos, last_pos + remaining)
}
make_array(mutable.freeze())
let values = if last_pos == 0 && !null_first {
array.values().slice(0, cap) // All slices were the correct length
} else {
if mutable.len() != cap {
// Remaining slices were all correct length
let remaining = cap - mutable.len();
mutable.extend(0, last_pos, last_pos + remaining)
}
make_array(mutable.freeze())
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let values = if last_pos == 0 && !null_first {
array.values().slice(0, cap) // All slices were the correct length
} else {
if mutable.len() != cap {
// Remaining slices were all correct length
let remaining = cap - mutable.len();
mutable.extend(0, last_pos, last_pos + remaining)
}
make_array(mutable.freeze())
};
let values = match last_pos {
0 if !null_first => array.values().slice(0, cap), // All slices were the correct length
_ => {
if mutable.len() != cap {
// Remaining slices were all correct length
let remaining = cap - mutable.len();
mutable.extend(0, last_pos, last_pos + remaining)
}
make_array(mutable.freeze())
}
};

nit: using guard in the match arm would reduce the diff happening here


// Cast the inner values if necessary
Expand Down
37 changes: 37 additions & 0 deletions arrow-cast/src/cast/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10033,4 +10033,41 @@ mod tests {
assert_eq!(result.unwrap_err().to_string(),
"Invalid argument error: 123456789 is too large to store in a Decimal256 of precision 6. Max is 999999");
}

#[test]
fn test_first_none() {
let array = Arc::new(ListArray::from_iter_primitive::<Int64Type, _, _>(vec![
None,
Some(vec![Some(1), Some(2)]),
])) as ArrayRef;
let data_type =
DataType::FixedSizeList(FieldRef::new(Field::new("item", DataType::Int64, true)), 2);
let opt = CastOptions::default();
let r = cast_with_options(&array, &data_type, &opt).unwrap();

let fixed_array = Arc::new(FixedSizeListArray::from_iter_primitive::<Int64Type, _, _>(
vec![None, Some(vec![Some(1), Some(2)])],
2,
)) as ArrayRef;
assert_eq!(*fixed_array, *r);
}

#[test]
fn test_first_last_none() {
let array = Arc::new(ListArray::from_iter_primitive::<Int64Type, _, _>(vec![
None,
Some(vec![Some(1), Some(2)]),
None,
])) as ArrayRef;
let data_type =
DataType::FixedSizeList(FieldRef::new(Field::new("item", DataType::Int64, true)), 2);
let opt = CastOptions::default();
let r = cast_with_options(&array, &data_type, &opt).unwrap();

let fixed_array = Arc::new(FixedSizeListArray::from_iter_primitive::<Int64Type, _, _>(
vec![None, Some(vec![Some(1), Some(2)]), None],
2,
)) as ArrayRef;
assert_eq!(*fixed_array, *r);
}
}
Loading