alamb commented on code in PR #5541:
URL: https://github.com/apache/arrow-rs/pull/5541#discussion_r1555016618
##########
arrow-array/src/builder/fixed_size_list_builder.rs:
##########
@@ -248,9 +325,37 @@ mod tests {
}
#[test]
- fn test_fixed_size_list_array_builder_finish_cloned() {
+ fn test_fixed_size_list_array_builder_with_field() {
+ let builder = make_list_builder(false, false);
Review Comment:
> If the field is set to non-nullable, this element will fail the null
check. I wanted to check with you if this is logically right or not?
I think I would expect it to be consistent with whatever
`FixedSlizeListArray::try_new` did
##########
arrow-array/src/builder/fixed_size_list_builder.rs:
##########
@@ -194,13 +227,31 @@ 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 field = match &self.field {
+ Some(f) => {
+ assert_eq!(
+ f.data_type(),
+ values_data.data_type(),
+ "DataType of field ({}) should be the same as the
values_builder DataType ({})",
+ f.data_type(),
+ values_data.data_type()
+ );
+ if !f.is_nullable() {
+ assert!(
+ values_data.null_count() == 0,
Review Comment:
I think it is valid to have a zero length array (which would also have no
nulls, but the null count would be zero)
```suggestion
values_data.null_count() == 0 || values_data.len()
== 0,
```
(same for the check below)
##########
arrow-array/src/builder/fixed_size_list_builder.rs:
##########
@@ -248,9 +325,37 @@ mod tests {
}
#[test]
- fn test_fixed_size_list_array_builder_finish_cloned() {
+ fn test_fixed_size_list_array_builder_with_field() {
+ let builder = make_list_builder(false, false);
+ let mut builder = builder.with_field(Field::new("list_element",
DataType::Int32, false));
+ let list_array = builder.finish();
+
+ assert_eq!(DataType::Int32, list_array.value_type());
+ assert_eq!(4, list_array.len());
+ assert_eq!(0, list_array.null_count());
+ assert_eq!(6, list_array.value_offset(2));
+ assert_eq!(3, list_array.value_length());
+ }
+
+ #[test]
+ #[should_panic(
+ expected = "field is nullable = false, but the values_builder contains
null values"
Review Comment:
this doesn't seem right -- the builder doesn't have null values, instead it
has no values at all.
--
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]
