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

[irenjj]

Which issue does this PR close?

• Closes First None in ListArray panics in cast_with_options #7043

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

[Jefffrey]

Some initial thoughts. I'm still going through the cast code to try understand what exactly is happening here and why the fix works the way it does.

[Jefffrey]

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

[Jefffrey]

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.

[irenjj]

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.

[Jefffrey]

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.

[irenjj]

Thanks @Jefffrey , I've made the requested changes. Could you help review it again?

[Jefffrey]

One minor nit, other than that looks good; thanks 👍

[alamb]

Thanks @irenjj and @Jefffrey