-
Notifications
You must be signed in to change notification settings - Fork 853
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add validation logic for StructBuilder::finish #2413
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -216,13 +216,16 @@ impl StructBuilder { | |
|
||
/// Builds the `StructArray` and reset this builder. | ||
pub fn finish(&mut self) -> StructArray { | ||
self.validate_content(); | ||
|
||
let mut child_data = Vec::with_capacity(self.field_builders.len()); | ||
for f in &mut self.field_builders { | ||
let arr = f.finish(); | ||
child_data.push(arr.into_data()); | ||
} | ||
|
||
let null_bit_buffer = self.null_buffer_builder.finish(); | ||
|
||
let builder = ArrayData::builder(DataType::Struct(self.fields.clone())) | ||
.len(self.len) | ||
.child_data(child_data) | ||
|
@@ -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()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we have checked
before, we don't need to check
Based on the transitivity of integer equality, checking
is enough. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, wait! We don't need to check the null buffer length at all. As we can make sure that the length of null buffer builder is always equal to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aah yes, does beg the question why we even have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filed #2429 to track this. |
||
{ | ||
panic!( | ||
"StructBuilder null buffer length and field_builders length are unequal." | ||
); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should also verify the length of the null buffer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have added this logic in 29ff8a6 |
||
} | ||
} | ||
|
||
#[cfg(test)] | ||
|
@@ -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(); | ||
} | ||
This comment was marked as abuse.
Sorry, something went wrong. |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we check this when constructing the struct builder, such as moving this check to
StructBuilder::new()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I too was thinking the same. But I made my code according to issues expected behavior. If the team needs it, I shall move this check to new function.