Parquet: Fix null counts for nested fields of a null struct - #17560
Conversation
|
Issue - #17561 |
When an optional struct is null, OptionWriter writes a null directly to every leaf column it contains, so the writers for those columns never see the value and cannot count it. OptionWriter dropped its own null count in that case, with a comment saying nested null stats were not used. They are used: the counts it returns become DataFile.nullValueCounts. Float, double, geometry and geography are the types whose writers report metrics, and ParquetMetrics prefers writer metrics over footer statistics, so the correct footer count was never consulted. Such a field under a nullable struct was reported as having 0 nulls even when the struct was null for some rows. Required fields are affected too, since only optional fields are wrapped in an option writer. The incorrect counting of nulls could affect query engines that relay on this stats for optimization. For example, they could simply skip the file with null_count == 0 for predicate `WHERE c.f_id IS NULL` and produces wrong result. Add the nulls counted by an option writer to the metrics of the columns it wrote them to, at any depth. And add corresponding tests.
25a0d81 to
1c3b1c6
Compare
uros-b
left a comment
uros-b
left a comment
There was a problem hiding this comment.
Fix looks good, left just one comment regarding regression test coverage
| assertThat(metrics.nullValueCount()).isEqualTo(1); | ||
| assertThat(metrics.avgValueSizeInBytes()).isEqualTo(31); | ||
| } | ||
|
|
There was a problem hiding this comment.
All four new tests assert only at the writer.metrics() boundary via a mocked ColumnWriteStore; none pins the corrected count at the user-visible end of the claim, that it reaches DataFile.nullValueCounts(). The seam is load-bearing: ParquetMetrics.primitive() (lines 242–248) prefers writer metrics and only falls through to the footer when absent, and ParquetMetrics.metrics() line 146 filters on nullValueCount() >= 0 before the value lands in the manifest. A future refactor of either could silently drop this fix with all four writer-level tests still green. There is an existing harness for exactly this (TestMetrics/TestParquetMetrics with assertCounts(fieldId, valueCount, nullValueCount, metrics)), but the existing NESTED_SCHEMA uses required(2, "nestedStructCol", ...), so a new test case with an optional struct containing a double leaf (or float, or geo) is needed. One added case in that harness closes the gap.
2 participants
When an optional struct is null, OptionWriter writes a null directly to every leaf column it contains, so the writers for those columns never see the value and cannot count it. OptionWriter dropped its own null count in that case, with a comment saying nested null stats were not used. They are used: the counts it returns become DataFile.nullValueCounts.
Float, double, geometry and geography are the types whose writers report metrics, and ParquetMetrics prefers writer metrics over footer statistics, so the correct footer count was never used. A float or double under a nullable struct was reported as having 0 nulls even when the struct was null for some rows.
The incorrect counting of nulls could affect query engines that relay on this stats for optimization. For example, they could simply skip the file with null_count == 0 for predicate
WHERE c.f_id IS NULLand produces wrong result.Add the nulls counted by an option writer to the metrics of the columns it wrote them to, at any depth. And add corresponding tests.