arrow-ipc: Add ListView support #9006
Conversation
17b88f1 to
5c74cb8
Compare
vegarsti
left a comment
There was a problem hiding this comment.
Great! I did a cursory review.
arrow-ipc/src/reader.rs
Outdated
| .add_buffer(buffers[2].clone()) // sizes | ||
| .add_child_data(child_data) | ||
| .null_bit_buffer(null_buffer), | ||
| _ => unreachable!("Cannot create listview array from {:?}", data_type), |
There was a problem hiding this comment.
Since this function can return an error, should this be an error instead of unreachable?
There was a problem hiding this comment.
We could potentially return "internal error" or something -- however, since the code can only be called with a ListView / LargeListView it is probably fine.
Looking more at this code, it seems like this is more of an assert -- perhaps the intent would be clearer if it explicitly did something like
assert!(matches!(data_type, ListView(_)|LargeListView(_));
let mut builder = ArrayData::builder(data_type.clone())
.len(length)
.add_buffer(buffers[1].clone()) // offsets
.add_buffer(buffers[2].clone()) // sizes
..There was a problem hiding this comment.
I overindexed on making the function look like the other ones. I agree, I like the assert better as well.
There was a problem hiding this comment.
Nice, yeah, that's better!
arrow-ipc/src/reader.rs
Outdated
| .add_buffer(buffers[2].clone()) // sizes | ||
| .add_child_data(child_data) | ||
| .null_bit_buffer(null_buffer), | ||
| _ => unreachable!("Cannot create listview array from {:?}", data_type), |
There was a problem hiding this comment.
We could potentially return "internal error" or something -- however, since the code can only be called with a ListView / LargeListView it is probably fine.
Looking more at this code, it seems like this is more of an assert -- perhaps the intent would be clearer if it explicitly did something like
assert!(matches!(data_type, ListView(_)|LargeListView(_));
let mut builder = ArrayData::builder(data_type.clone())
.len(length)
.add_buffer(buffers[1].clone()) // offsets
.add_buffer(buffers[2].clone()) // sizes
..| } | ||
| } | ||
|
|
||
| fn generate_nested_list_view_data<O: OffsetSizeTrait>() -> GenericListViewArray<O> { |
Which issue does this PR close?
Rationale for this change
See the issue.
What changes are included in this PR?
Add tests and support for ListView/LargeListView support for arrow-ipc.
Are these changes tested?
Yes, various test cases added.
Are there any user-facing changes?
No breaking changes, strictly new support for types.
@alamb @vegarsti