alamb commented on code in PR #5541:
URL: https://github.com/apache/arrow-rs/pull/5541#discussion_r1535870036
##########
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:
I don't understand the rationale for this change. Is there some case where
invalid arrays are created?
Switching to use build() means the data is now checked twice (once when
added to the builder and once when the array is built)
If there are ways to construct an invalid FIxedListArray via the builder
APIs, I think those should be fixed rather than running the expensive
`build()` checks again.
For example, if you are trying to validate that a builder with a field that
was declared to be non-null actually has no nulls, it would be much faster to
leave the `build_unchecked()` call and do a separate check that
`array_data.null_count()` was zero
Likewise, you could verify the DataType of the `Field` once as part of
`with_field` call, rather than after construction
--
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]
