fix parquet max_definition for non-null structs #246
Conversation
Codecov Report
@@ Coverage Diff @@
## master #246 +/- ##
==========================================
+ Coverage 82.51% 82.53% +0.01%
==========================================
Files 162 162
Lines 43655 43740 +85
==========================================
+ Hits 36022 36099 +77
- Misses 7633 7641 +8
Continue to review full report at Codecov.
|
jorgecarleitao
left a comment
jorgecarleitao
left a comment
There was a problem hiding this comment.
Thanks a lot for taking this on, @nevi-me !
I started reviewing this. I can see where the problem may lie, but I am still trying to understand the logic.
| // second exception, always increment after a list | ||
| self.max_definition + 1 | ||
| } else if is_parent_struct && !self.is_nullable { | ||
| // if the parent is a non-null struct, don't increment |
There was a problem hiding this comment.
I am trying to wrap my head around this comment: self.is_nullable is the nullability of the parent in this case?
I would expect something like is_parent_struct_null computed from the nullability of the parent?
There was a problem hiding this comment.
Yes, self is the parent, so if the parent is a null struct ...
Interestingly, I don't need the 'is_parent_steuct' variable, because '!self.is_list' means that self is a struct at this point in the code. I'll tweak the logic later tonight
There was a problem hiding this comment.
I checked this, trying to rely on the !self.is_list doesn't always work, so I'm leaving the logic as is for now.
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
I can't say I understand the intricacies of this logic, but I did read the PR and it seems to me that the PR is a step in the right direction and can be merged as is:
- Some tests pass now that did not pass previously
- The additional cases which may require more work are tracked by an issue.
| let b_data = ArrayDataBuilder::new(field_b.data_type().clone()) | ||
| .len(6) | ||
| .null_bit_buffer(Buffer::from(vec![0b00100111])) | ||
| // .null_bit_buffer(Buffer::from(vec![0b00100111])) |
There was a problem hiding this comment.
I think this is ok (given the test didn't pass previously anyways) so this isn't a regression
| } | ||
| DataType::List(_) | DataType::LargeList(_) | DataType::Struct(_) => { | ||
| list_level.calculate_array_levels(&child_array, list_field) | ||
| list_level.calculate_array_levels(&child_array, list_field, false) |
There was a problem hiding this comment.
if the data type is DataType::Struct(_), wouldn't is_parent_struct be true? I probably misunderstand
There was a problem hiding this comment.
In this case, this is an immediate struct of a parent list, so it's false.
Co-authored-by: Andrew Lamb <[email protected]>
jorgecarleitao
left a comment
jorgecarleitao
left a comment
There was a problem hiding this comment.
LGTM. Thanks @nevi-me !
4 participants
Which issue does this PR close?
Closes #244 .
Rationale for this change
This fixes a bug
What changes are included in this PR?
Are there any user-facing changes?
No, changes made to crate-level structs, that are not publicly exposed