-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add Full UnionArray validation #1444
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
Changes from 1 commit
30cb59c
ec58d86
dc40d4e
20b445c
7fcdc9e
c0c21f1
2bc1491
d56cf46
6271ba8
2769573
72ef638
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 |
|---|---|---|
|
|
@@ -916,8 +916,6 @@ impl ArrayData { | |
| /// 3. All String data is valid UTF-8 | ||
| /// 3. All dictionary offsets are valid | ||
| /// | ||
| /// Does not (yet) check | ||
| /// 1. Union type_ids are valid see [#85](https://github.com/apache/arrow-rs/issues/85) | ||
| /// Note calls `validate()` internally | ||
| pub fn validate_full(&self) -> Result<()> { | ||
| // Check all buffer sizes prior to looking at them more deeply in this function | ||
|
|
@@ -957,12 +955,16 @@ impl ArrayData { | |
| let child = &self.child_data[0]; | ||
| self.validate_offsets_full::<i64>(child.len + child.offset)?; | ||
| } | ||
| DataType::Union(_, _) => { | ||
| // Validate Union Array as part of implementing new Union semantics | ||
| // See comments in `ArrayData::validate()` | ||
| // https://github.com/apache/arrow-rs/issues/85 | ||
| // | ||
| // TODO file follow on ticket for full union validation | ||
| DataType::Union(_fields, mode) => { | ||
| match mode { | ||
| UnionMode::Sparse => { | ||
| // typeids should all be valid | ||
| self.validate_offsets_full::<i8>(self.child_data.len())?; | ||
|
alamb marked this conversation as resolved.
Outdated
|
||
| } | ||
| UnionMode::Dense => { | ||
| self.validate_dense_union_full()?; | ||
|
Contributor
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 was going to suggest that this should validate that the null bitmasks are disjoint, but this may not even be a requirement - the specification says "All “unselected” values are ignored and could be any semantically correct array value." |
||
| } | ||
| } | ||
| } | ||
| DataType::Dictionary(key_type, _value_type) => { | ||
| let dictionary_length: i64 = self.child_data[0].len.try_into().unwrap(); | ||
|
|
@@ -1098,7 +1100,7 @@ impl ArrayData { | |
| ) | ||
| } | ||
|
|
||
| /// Ensures that all offsets in `buffers[0]` into `buffers[1]` are | ||
| /// Ensures that all values in `buffers[0]` are | ||
| /// between `0` and `offset_limit` | ||
| fn validate_offsets_full<T>(&self, offset_limit: usize) -> Result<()> | ||
| where | ||
|
|
@@ -1117,6 +1119,44 @@ impl ArrayData { | |
| ) | ||
| } | ||
|
|
||
| /// Ensures that for each union element, the offset is correct for | ||
| /// the corresponding child array | ||
| fn validate_dense_union_full(&self) -> Result<()> { | ||
|
Contributor
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 think should also check that offsets are monotonic for a given array type, but that could definitely be left as a todo |
||
| // safety justification is that the size of the buffers was validated in self.validate() | ||
|
Contributor
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. We could potentially make
Contributor
Author
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 think actually that all child arrays need to have the same length for a sparse representation (which is checked in cheap Which is covered by |
||
| let type_ids = self.typed_offsets::<i8>(&self.buffers[0])?; | ||
| let offsets = self.typed_offsets::<i32>(&self.buffers[1])?; | ||
|
|
||
| type_ids.iter().enumerate().try_for_each(|(i, &type_id)| { | ||
| // this will panic if out of bounds. Could make a nicer error message | ||
| let type_id: usize = type_id | ||
| .try_into() | ||
| .map_err(|_| { | ||
| ArrowError::InvalidArgumentError(format!( | ||
| "Offset invariant failure: Could not convert type id {} to usize in slot {}", | ||
| type_id, i)) | ||
| })?; | ||
|
|
||
| let num_children = self.child_data[type_id].len(); | ||
| let child_offset: usize = offsets[i] | ||
| .try_into() | ||
| .map_err(|_| { | ||
| ArrowError::InvalidArgumentError(format!( | ||
| "Offset invariant failure: Could not convert offset {} at position {} to usize", | ||
| offsets[i], i)) | ||
| })?; | ||
|
|
||
|
|
||
| if child_offset >= num_children { | ||
| Err(ArrowError::InvalidArgumentError(format!( | ||
| "Value at position {} out of bounds: {} (child array {} length is {})", | ||
|
alamb marked this conversation as resolved.
Outdated
|
||
| i, child_offset, type_id, num_children | ||
| ))) | ||
| } else { | ||
| Ok(()) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| /// Validates that each value in self.buffers (typed as T) | ||
| /// is within the range [0, max_value], inclusive | ||
| fn check_bounds<T>(&self, max_value: i64) -> Result<()> | ||
|
|
||
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.
Unless I'm missing something, we should probably also add buffer length checks into
ArrayData::validateas I don't think these are currently present anywhereThere 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.
what do you mean "buffer length checks"? Which buffers?
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.
Sorry this got lost in my various rewording of this, I mean child arrays 🤦♂️
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.
The length of the
ArrayData::buffersare checked as part of the 'layout' inArrayData::validatehttps://github.com/apache/arrow-rs/blob/master/arrow/src/array/data.rs#L1210
The child arrays
ArrayData::child_dataare validated recursively inArrayData::validate_child_datahttps://github.com/apache/arrow-rs/blob/master/arrow/src/array/data.rs#L788