feat: add GenericListViewBuilder #6552
Conversation
| impl<OffsetSize: OffsetSizeTrait, T: ArrayBuilder> ArrayBuilder | ||
| for GenericListViewBuilder<OffsetSize, T> | ||
| where | ||
| T: 'static, |
There was a problem hiding this comment.
Is static lifespan constraint avoidable?
|
These two failed unit tests seem unrelated to this pr? |
| let offsets = Buffer::from_slice_ref(self.offsets_builder.as_slice()); | ||
| // Safety: safe by construction | ||
| let offsets = ScalarBuffer::from(offsets); |
There was a problem hiding this comment.
Does offsets_builder have finish_clonsed function?
There was a problem hiding this comment.
Like other builders, it is completed with a finish function.
| offsets_builder, | ||
| null_buffer_builder: NullBufferBuilder::new(capacity), | ||
| values_builder, |
There was a problem hiding this comment.
nit: for consistency inline all, or define all above struct constructor (uless there is a reason to treat null buffer builder differently)
There was a problem hiding this comment.
The reason for writing it this way is to maintain consistency with the GenericListBuilder API. I think aligning with it makes the API easier to use.
| /// Returns the child array builder as an immutable reference | ||
| pub fn values_ref(&self) -> &T { | ||
| &self.values_builder | ||
| } |
There was a problem hiding this comment.
This is also to maintain API consistency with GenericListBuilder API , so it hasn’t been removed.
| #[should_panic(expected = "ListViewArray expected data type Int64 got Int32")] | ||
| fn test_checks_data_type() { | ||
| let field = Arc::new(Field::new("item", DataType::Int64, false)); | ||
| let mut builder = ListViewBuilder::new(Int32Builder::new()).with_field(field.clone()); | ||
| builder.append_value([Some(1)]); | ||
| builder.finish(); |
There was a problem hiding this comment.
Does this panic on ListViewBuilder::new, append_value or finish?
Generally annotaiton-driven "should fail" tests are best if they are ~one-liners.
For longer code it might actually be important to asset which line, which operation fails, because that's the guarantee we want to provide to API users.
There was a problem hiding this comment.
Sounds good, I added some comments on the should fail test.
|
@tustvold please take a look |
|
I think he's phasing out of this part of the work; perhaps @alamb help review or assign it to someone. |
|
@alamb I think we can proceed with this PR now. 🤠 |
|
Thanks @Kikkon -- looks like a cool PR. I have put it on my list for review, but given all the other things that are ahead of it I do not know when I will be able to get to this. Perhaps you have some time to help review some of the other PRs in this repository which could free up some time from the other maintainers? |
Sounds good, I will choose some PRs that are within my capability to review. |
| use crate::{Array, Int32Array}; | ||
| use arrow_schema::DataType; | ||
|
|
||
| fn _test_generic_list_view_array_builder<O: OffsetSizeTrait>() { |
There was a problem hiding this comment.
That's mostly a style nitpick: I would prefer if we don't start with an underscore because the rust naming usually suggests that this is unused (at least that's how variable/member naming works). So a good name would be test_generic_list_view_array_builder_impl.
There was a problem hiding this comment.
@crepererum Great advice! I’ve already made the modifications and committed them. Ready to merge.
| _test_generic_list_view_array_builder::<i64>() | ||
| } | ||
|
|
||
| fn _test_generic_list_view_array_builder_nulls<O: OffsetSizeTrait>() { |
Co-authored-by: Marco Neumann <marco@crepererum.net>
Co-authored-by: Marco Neumann <marco@crepererum.net>
Which issue does this PR close?
A part of #5501
Rationale for this change
What changes are included in this PR?
Add GenericListViewBuilder implement
Are there any user-facing changes?