Repository navigation
Deprecate values/keys/etc in variable length arrays like list/map/binary in favor of 2 functions actual_values and underlying_values or something #7992
Description
Slicing on a variable length arrays like list/map/binary, does not slice the underlying values but instead only slice the offsets due to performance.
However when wanting to get the underlying values for a list/map/binary, calling values (on list for example) give you the child values even the ones that are not referred to by the current slice
I've lost count on the number of times I saw or had bugs with getting the values instead of the the actual values that the list point to.
this is an example where it's counter intuitive
let list = ListArray::from_iter_primitive::<Int32Type, _, _>(vec![
Some(vec![Some(1), Some(2)]),
None,
Some(vec![Some(3), None, Some(5)]),
]);
let list = list.slice(1, 2);
// [null, [3, null, 5]]
println!("{:?}", list);
// [1, 2, 3, null, 5]
println!("{:?}", list.values());we added comments on list values that mention that The list array may not refer to all values in the values array ... but this is still not enough IMO.
also, creating 2 functions that explicitly state what is returned will force the developer to think there is a cost (for example, casting list to string, you should only cast the sliced underlying values and not the entire underlying values and then slice)
References to related changes/bugs:
- Fix
concatfor slicedListArrays#7037 - fix for a bug I had in concat of sliced list - IPC Writer Truncate Sliced List Values #4409 - IPC slice
RowConverteron list should only encode the sliced list values and not the entire data #7993 - If the function would be explicit I believe it could have done or at least be aware of in the first place
Activity
[-]Deprecate variable length arrays like list/map/binary `values`/`keys`/etc in favor of 2 functions `actual_values` and `underlying_values` or something[/-][+]Deprecate `values` in variable length arrays like list/map/binary `values`/`keys`/etc in favor of 2 functions `actual_values` and `underlying_values` or something[/+][-]Deprecate `values` in variable length arrays like list/map/binary `values`/`keys`/etc in favor of 2 functions `actual_values` and `underlying_values` or something[/-][+]Deprecate `values`/`keys`/etc in variable length arrays like list/map/binary in favor of 2 functions `actual_values` and `underlying_values` or something[/+]alamb commentedon Jul 24, 2025
I think making the API clearer about what is happening with different functions and examples sounds like a great idea
I will be quite happy to review such PRs and help with the documentation / naming
cc @tustvold and @westonpace as I think you may have opinions on this topic as well
rluvaton commentedon Oct 23, 2025
Just found another bug in my code due to that.
Any naming suggestion?
tustvold commentedon Oct 23, 2025
IMO adding more methods just makes it even more confusing, as if you return a sliced representation of values, the offsets no longer match up correctly. Additionally the proposed actual_values may still contain unreferenced values, e.g. in the presence of a non-empty null.
Whilst I agree it's easy to make mistakes here, it's somewhat inherent to the way the arrow format is formulated. You have the exact same "issue" with StringArray, BinaryArray, DictionaryArray, etc... At a certain point if you're messing around with the underlying arrays you need to be careful