alamb commented on code in PR #5331:
URL: https://github.com/apache/arrow-rs/pull/5331#discussion_r1466263261
##########
arrow-array/src/builder/generic_list_builder.rs:
##########
@@ -275,53 +289,37 @@ where
/// Builds the [`GenericListArray`] and reset this builder.
pub fn finish(&mut self) -> GenericListArray<OffsetSize> {
- let len = self.len();
- let values_arr = self.values_builder.finish();
- let values_data = values_arr.to_data();
+ let values = self.values_builder.finish();
+ let nulls = self.null_buffer_builder.finish();
- let offset_buffer = self.offsets_builder.finish();
- let null_bit_buffer = self.null_buffer_builder.finish();
+ let offsets = self.offsets_builder.finish();
+ // Safety: Safe by construction
+ let offsets = unsafe { OffsetBuffer::new_unchecked(offsets.into()) };
self.offsets_builder.append(OffsetSize::zero());
- let field = Arc::new(Field::new(
- "item",
- values_data.data_type().clone(),
- true, // TODO: find a consistent way of getting this
- ));
- let data_type =
GenericListArray::<OffsetSize>::DATA_TYPE_CONSTRUCTOR(field);
- let array_data_builder = ArrayData::builder(data_type)
- .len(len)
- .add_buffer(offset_buffer)
- .add_child_data(values_data)
- .nulls(null_bit_buffer);
- let array_data = unsafe { array_data_builder.build_unchecked() };
+ let field = match &self.field {
+ Some(f) => f.clone(),
+ None => Arc::new(Field::new("item", values.data_type().clone(),
true)),
Review Comment:
Yeah, I think there are two camps in terms of "do I know / want to know what
the name of the anonymous field are in the list"
Specifically,
1. if you are familiar with the convention, then it is better to have the
code explcit.
2. If you don't know the `"item"` convention having a function hide it is
easier to understand
##########
arrow-array/src/builder/generic_list_builder.rs:
##########
@@ -765,4 +763,39 @@ mod tests {
assert_eq!(0, i1.null_count());
assert_eq!(i1.values(), &[1, 2, 3, 4, 5, 6, 7, 8, 9, 10]);
}
+
+ #[test]
+ fn test_non_nullable_list() {
+ let field = Arc::new(Field::new("item", DataType::Int32, false));
Review Comment:
Would it make sense to also add a test for a field that is not named "item"?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
