arrow-row: Add ListView support #9176
Conversation
friendlymatthew
left a comment
friendlymatthew
left a comment
There was a problem hiding this comment.
Hi, do you mind rebasing? I think CI is failing because this needs to be rebased on the latest master- the base branch is missing the encoded_len fn that was added recently
Otherwise, the implementation makes sense to me
brancz
force-pushed
the
row-list-view
branch
2 times, most recently
from
cd2a465 to
29f13ef
Compare
|
Very confused because both this PR and #9175 are based off of latest main, and the tip of the other PR seems to work fine. |
|
Figured it out, I did actually need to change something after the rebase here, also refactored the use in both list-types. |
| list_size += 1; | ||
| } | ||
| } | ||
| O::from_usize(child_count).expect("overflow"); |
There was a problem hiding this comment.
Is this to force a panic?
There was a problem hiding this comment.
yes, same as the other assertion, this is consistent with what we do for regular lists as well
There was a problem hiding this comment.
Personally I feel it makes more sense to return an error here since the function already supports that
| .collect(); | ||
|
|
||
| let child = unsafe { converter.convert_raw(&mut child_rows, validate_utf8) }?; | ||
| assert_eq!(child.len(), 1); |
There was a problem hiding this comment.
Should we return an error here since the function returns a result anyway?
There was a problem hiding this comment.
we perform the exact same assertion in the regular lists, so I did this for consistency and it seems like a good idea since something went pretty spectacularly wrong if this isn't true
arrow-row/src/lib.rs
Outdated
|
|
||
| /// Computes the minimum offset and maximum end (offset + size) for a ListView array. | ||
| /// Returns (min_offset, max_end) which can be used to slice the values array. | ||
| fn compute_list_view_bounds<O: OffsetSizeTrait>(array: &GenericListViewArray<O>) -> (usize, usize) { |
There was a problem hiding this comment.
This function seems oddly placed; should be lower down instead of in the middle of the mod declarations?
There was a problem hiding this comment.
moved it further down in the file, but not sure I love that either, maybe move it to the list file? what do you think?
There was a problem hiding this comment.
We can always put it down near row_lengths where it won't intrude in the middle of Codec here
arrow-row/src/list.rs
Outdated
| _ => unreachable!(), | ||
| }; | ||
|
|
||
| let null_buffer = NullBuffer::new(BooleanBuffer::new(nulls.into(), 0, rows.len())); |
There was a problem hiding this comment.
Maybe use new_unchecked as you already have the null count and we want to avoid calculating it twice
| let null_buffer = NullBuffer::new(BooleanBuffer::new(nulls.into(), 0, rows.len())); | |
| let null_buffer = NullBuffer::new_unchecked(BooleanBuffer::new(nulls.into(), 0, rows.len()), null_count); |
|
|
||
| if size > 0 { | ||
| min_offset = min_offset.min(offset); | ||
| max_end = max_end.max(end); |
There was a problem hiding this comment.
Maybe you can break if you reached maximum bounds (0 and maximum value that can be)
There was a problem hiding this comment.
done (although I wonder how often this is better in practice while adding an additional branch for a case that might happen incredibly rarely)
| #[test] | ||
| fn test_list_view() { | ||
| test_single_list_view::<i32>(); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_large_list_view() { | ||
| test_single_list_view::<i64>(); | ||
| } |
There was a problem hiding this comment.
Please add nested tests like the regular list
| test_nested_list::<i64>(); | ||
| } | ||
|
|
||
| fn test_single_list_view<O: OffsetSizeTrait>() { |
There was a problem hiding this comment.
Please add more tests that take advantage of the fact that this is a view, namely
- both list point to the same value.
- unordered offsets (one item is from offset x and some item after that is from offset y and y is before x)
- list 1 items cover list 2 items and a little more (e.g. list 1 offset is 10 and size 5 and list 2 offset is 12 and size 2).
There was a problem hiding this comment.
done (added all cases in one test let me know if you prefer separate tests)
| ListArray::new(field, offsets, values, Some(nulls)) | ||
| } | ||
|
|
||
| fn generate_column(len: usize) -> ArrayRef { |
There was a problem hiding this comment.
Please add the list view and large list view to here as well similar to how list and large list are here.
don't forget to increase the random range so it will cover the new values
There was a problem hiding this comment.
this was a great idea, it actually caught something
|
@rluvaton thoughts on the assertion vs. returning an error? I don't feel too strongly one way or another, but I do think we should be consistent for both List and ListView. |
4 participants
Which issue does this PR close?
Closes #9174
What changes are included in this PR?
Implementation and tests. It's mostly copied from
List.Are these changes tested?
Yes, see unit tests.
Are there any user-facing changes?
No, purely additive.
@alamb @Jefffrey