GH-51097: Fix Parquet null counts for fixed-width leaves - #51357
AnuragRaut08 wants to merge 1 commit into
Conversation
|
|
283c550 to
d6f477d
Compare
5f9696f to
ac0b0a9
Compare
Signed-off-by: AnuragRaut08 <anuragtraut2003@gmail.com>
ac0b0a9 to
49995ad
Compare
|
Hi @wgtmac , |
wgtmac
left a comment
There was a problem hiding this comment.
Thanks @AnuragRaut08 for the quick update! I think the code change looks good now. I have some comments for better test coverage.
|
|
||
| ASSERT_EQ(row_group->num_columns(), 2); | ||
|
|
||
| auto int32_stats = row_group->ColumnChunk(1)->statistics(); |
There was a problem hiding this comment.
Can we run this test for both data page V1 and V2 and then check both leaf columns? The old change fixed i32 but broke the string leaf, so the current checks would not catch that error.
| // had so we need to recompute it from def levels. | ||
| MaybeCalculateValidityBits(AddIfNotNull(def_levels, offset), batch_size, | ||
| &batch_num_values, &batch_num_spaced_values, &null_count); | ||
| const int64_t total_null_count = batch_size - batch_num_values; |
There was a problem hiding this comment.
Please add the same nested test case with a dictionary leaf. Run it for data page V1 and V2 by checking the statistics and reading the values back.
| MaybeCalculateValidityBits(AddIfNotNull(def_levels, offset), batch_size, | ||
| &batch_num_values, &batch_num_spaced_values, | ||
| &null_count); | ||
| const int64_t total_null_count = batch_size - batch_num_values; |
There was a problem hiding this comment.
Can we rename total_null_count to parquet_null_count because it means number of nulls in the parquet leaf column? null_count is for the Arrow leaf array but renaming it would result in a larger change.
Rationale for this change
Parquet null-count statistics can be incorrect for fixed-width leaf columns nested below a repeated ancestor, such as
list<struct<...>>.When a list is null or empty, its descendant leaf does not produce a value in the leaf values buffer.
DefLevelsToBitmapintentionally excludes these repeated-ancestor nulls from itsnull_count.MaybeCalculateValidityBitswas using that value directly for the Parquet column statistics, causing the null count to be undercounted.For example, a
list<struct<string, int32>>column can report an incorrectnull_countfor theint32leaf even though the encoded definition levels correctly represent the null and empty list entries.What changes are included in this PR?
MaybeCalculateValidityBitsto calculate the total null count frombatch_size - out_values_to_write.int32leaf underlist<struct<...>>.Are these changes tested?
Yes.
Added a regression test that verifies the
int32leaf reports:null_count = 3num_values = 2The focused regression test passes:
StatisticsTest.FixedWidthLeafUnderListStructNullCountThe existing
parquet-writer-testalso passes.Are there any user-facing changes?
Yes. This fixes incorrect Parquet column statistics for affected nested fixed-width columns. The change does not alter the encoded data or public APIs.
This PR contains a "Critical Fix".
This fixes a bug that produces incorrect Parquet statistics. The underlying data remains correct, but the reported
null_countfor affected fixed-width leaf columns can be incorrect.Was AI used for this PR?
PR code and description written by:
Reviewed before submission by:
Human
AI
Not reviewed
GitHub Issue: [Python][C++][Parquet] null_count statistics undercount for a fixed-width leaf under list<struct<...>> when lists are null or empty #51097