fix: Fix a bug in how definition levels are calculated for nested structs in a list #1185
Conversation
Using the definition level and the nullability of the column only produces the correct indices if max_definition - 1 is the list level. For deeper nesting (struct in a list) this produces incorrect indices, silently causing incorrect data to be written. This fix uses the array offsets to compute the indices instead.
Codecov Report
@@ Coverage Diff @@
## master #1185 +/- ##
=======================================
Coverage 82.66% 82.67%
=======================================
Files 173 173
Lines 50902 50916 +14
=======================================
+ Hits 42077 42093 +16
+ Misses 8825 8823 -2
Continue to review full report at Codecov.
|
| @@ -759,13 +759,6 @@ impl LevelInfo { | |||
| /// Given a level's information, calculate the offsets required to index an array correctly. | |||
| pub(crate) fn filter_array_indices(&self) -> Vec<usize> { | |||
| // happy path if not dealing with lists | |||
| let is_nullable = match self.level_type { | |||
| LevelType::Primitive(is_nullable) => is_nullable, | |||
| _ => panic!( | |||
There was a problem hiding this comment.
I think this assertion is still important - this method will return nonsense if called on something that isn't a leaf
There was a problem hiding this comment.
Makes sense. Added the assertion back.
| if *def >= self.max_definition - is_nullable as i16 { | ||
| index += 1; | ||
|
|
||
| for len in self.array_offsets.windows(2).map(|s| s[1] - s[0]) { |
There was a problem hiding this comment.
I'm not hugely familiar with this code, but the below is my attempt to describe what is going on here.
The parquet writer iterates through all the leaf arrays in a potentially nested arrow array, producing a list of LevelInfo that it then writes. Each LevelInfo identifies for that leaf, what repetition and definition levels to write. This method is then used to compute which non-null indices from the source primitive array to write that correspond to this information.
It therefore iterates through the definition levels and where def == self.max_definition it records that index as one that needs to be written to parquet.
The problem with the previous logic was it assumed that only a null or value at the leaf level, i.e. def >= max_def - is_nullable would increase the index position in the source primitive array.
Whilst this is the case for a parent nullable ListArray, it is not for a parent nullable StructArray. Critically, a null in a ListArray may not take up space in its child array, but a null in a StructArray does.
By using the array_offsets instead of the nesting level, we can continue to handle list array nulls that don't take up space, whilst handling StructArray nulls that do. 👍
As an added bonus I think this fixes a related bug, where a ListArray with a non-zero length null slot would break for similar reasons
parquet/src/arrow/levels.rs
Outdated
|
|
||
| for len in self.array_offsets.windows(2).map(|s| s[1] - s[0]) { | ||
| if len == 0 { | ||
| // Skip this definition level (it should be less than max_definition) |
There was a problem hiding this comment.
Perhaps this code should verify that it is indeed a null? 🤔
There was a problem hiding this comment.
Added an assert :)
alamb
changed the title
Which issue does this PR close?
Closes #1184.
What changes are included in this PR?
Using the definition level and the nullability of the column only produces the
correct indices if
max_definition - 1is the list level. For deeper nesting(struct in a list) this produces incorrect indices, silently causing incorrect
data to be written.
This PR fixes the bug by using the array offsets to compute the indices
instead, also adds a regression test.
Are there any user-facing changes?
No.