HaoYang670 commented on code in PR #2413:
URL: https://github.com/apache/arrow-rs/pull/2413#discussion_r944436453
##########
arrow/src/array/builder/struct_builder.rs:
##########
@@ -233,6 +236,28 @@ impl StructBuilder {
let array_data = unsafe { builder.build_unchecked() };
StructArray::from(array_data)
}
+
+ /// Constructs and validates contents in the builder to ensure that
+ /// - fields and field_builders are of equal length
+ /// - the number of items in individual field_builders are equal to
self.len
+ /// - the number of items in individual field_builders are equal to
self.null_buffer_builder.len()
+ fn validate_content(&self) {
+ if self.fields.len() != self.field_builders.len() {
+ panic!("Number of fields is not equal to the number of
field_builders.");
+ }
+ if !self.field_builders.iter().all(|x| x.len() == self.len) {
+ panic!("StructBuilder and field_builders are of unequal lengths.");
+ }
+ if !self
+ .field_builders
+ .iter()
+ .all(|x| x.len() == self.null_buffer_builder.len())
Review Comment:
As we have checked
```
self.field_builders.iter().all(|x| x.len() == self.len)
```
before, we don't need to check
```
self .field_builders.iter().all(|x| x.len() ==
self.null_buffer_builder.len())
```
Based on the transitivity of integer equality, checking
```
self.len == self.null_buffer_builder.len()
```
is enough.
##########
arrow/src/array/builder/struct_builder.rs:
##########
@@ -411,4 +436,44 @@ mod tests {
let mut builder = StructBuilder::new(fields, field_builders);
assert!(builder.field_builder::<BinaryBuilder>(0).is_none());
}
+
+ #[test]
+ #[should_panic(expected = "StructBuilder and field_builders are of unequal
lengths.")]
+ fn test_struct_array_builder_unequal_field_builders_lengths() {
+ let mut int_builder = Int32Builder::new(10);
+ let mut bool_builder = BooleanBuilder::new(10);
+
+ int_builder.append_value(1);
+ int_builder.append_value(2);
+ bool_builder.append_value(true);
+
+ let mut fields = Vec::new();
+ let mut field_builders = Vec::new();
+ fields.push(Field::new("f1", DataType::Int32, false));
+ field_builders.push(Box::new(int_builder) as Box<dyn ArrayBuilder>);
+ fields.push(Field::new("f2", DataType::Boolean, false));
+ field_builders.push(Box::new(bool_builder) as Box<dyn ArrayBuilder>);
+
+ let mut builder = StructBuilder::new(fields, field_builders);
+ builder.append(true);
+ builder.append(true);
+ builder.finish();
+ }
+
+ #[test]
+ #[should_panic(
+ expected = "Number of fields is not equal to the number of
field_builders."
+ )]
+ fn test_struct_array_builder_unequal_field_field_builders() {
+ let int_builder = Int32Builder::new(10);
+
+ let mut fields = Vec::new();
+ let mut field_builders = Vec::new();
+ fields.push(Field::new("f1", DataType::Int32, false));
+ field_builders.push(Box::new(int_builder) as Box<dyn ArrayBuilder>);
+ fields.push(Field::new("f2", DataType::Boolean, false));
+
+ let mut builder = StructBuilder::new(fields, field_builders);
+ builder.finish();
+ }
Review Comment:
We also need tests to check the length of null buffer builder.
--
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]
