`ScalarValue::convert_array_to_scalar_vec` doesn't respect null list elements

[Jefffrey]

Because it used to do array.as_list::<i32>().value(index), this never checked for nulls before.

So in the test case I added below:

// Funky (null slot has non-zero list offsets)
// Offsets + Values looks like this: [[1, 2], [3, 4], [5]]
// But with NullBuffer it's like this: [[1, 2], NULL, [5]]
let funky = ListArray::new(
    Field::new_list_field(DataType::Int64, true).into(),
    OffsetBuffer::new(vec![0, 2, 4, 5].into()),
    Arc::new(Int64Array::from(vec![1, 2, 3, 4, 5, 6])),
    Some(NullBuffer::from(vec![true, false, true])),
);
let converted = ScalarValue::convert_array_to_scalar_vec(&funky).unwrap();
assert_eq!(
    converted,
    vec![
        vec![ScalarValue::Int64(Some(1)), ScalarValue::Int64(Some(2))],
        vec![],
        vec![ScalarValue::Int64(Some(5))],
    ]
);

For the output, it incorrect would have vec![3, 4] instead of the empty Vec (it's accessing that element as if it were a valid list when in fact it was null in the parent list).

For now I made nulls return empty list to not change the signature of the method, though this seems undesirable 🤔

Originally posted by @Jefffrey in #17732 (comment)

[vegarsti]

From what I understand it seems more correct if it returns None rather than the empty list in that case, is that what you're implying as well? And making this change would mean changing the signature from returning -> Result<Vec<Vec<ScalarValue>>> to returning Result<Vec<Option<Vec<ScalarValue>>>>

[Jefffrey]

From what I understand it seems more correct if it returns None rather than the empty list in that case, is that what you're implying as well? And making this change would mean changing the signature from returning -> Result<Vec<Vec<ScalarValue>>> to returning Result<Vec<Option<Vec<ScalarValue>>>>

Pretty much, yeah

[vegarsti]

From what I understand it seems more correct if it returns None rather than the empty list in that case, is that what you're implying as well? And making this change would mean changing the signature from returning -> Result<Vec<Vec<ScalarValue>>> to returning Result<Vec<Option<Vec<ScalarValue>>>>

Pretty much, yeah

Nice, seems like an easy fix

[vegarsti]

take

[vegarsti]

@Jefffrey I made an attempt here #17891