istvan-fodor commented on code in PR #5541:
URL: https://github.com/apache/arrow-rs/pull/5541#discussion_r1535950940
##########
arrow-array/src/builder/fixed_size_list_builder.rs:
##########
@@ -194,17 +212,23 @@ where
);
let nulls = self.null_buffer_builder.finish_cloned();
- let array_data = ArrayData::builder(DataType::FixedSizeList(
- Arc::new(Field::new("item", values_data.data_type().clone(),
true)),
- self.list_len,
- ))
- .len(len)
- .add_child_data(values_data)
- .nulls(nulls);
- let array_data = unsafe { array_data.build_unchecked() };
Review Comment:
The rationale is that with giving the ability to the user to change the
field we use, the values_builder and the field can be out of sync.
If we use build_unchecked, the build will not fail as expected, so the unit
tests
`builder::fixed_size_list_builder::tests::test_fixed_size_list_array_builder_with_field_null_panic`
and
`builder::fixed_size_list_builder::tests::test_fixed_size_list_array_builder_with_field_type_panic`will
fail.
I think the best place would be to do this check in `with_field`, asserting
that the nullable and data_type are the same as the values-builder, but I
didn't find a way to actually get the values from values_builder.
If we stick with `build_unchecked`, we could have this check once the
`values_builder` is built (data_type and nullable are readily available on the
built `ArrayData`).
I am updating the PR with this version and let me know if this is better.
--
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]
