Repository navigation
Fix fully matched row groups with null counts - #21907
Conversation
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @xudong963 -- this seems better than what is on main, but I am not quite sure about the decision to pick
missing_null_counts_as_zero: false,
vs
missing_null_counts_as_zero: true,
| parquet_schema, | ||
| row_group_metadatas, | ||
| arrow_schema, | ||
| missing_null_counts_as_zero: true, |
There was a problem hiding this comment.
shouldn't this value also be passed down rather than hard coded?
There was a problem hiding this comment.
It also threads with_missing_null_counts_as_zero through RowGroupPruningStatistics so normal row-group pruning keeps the existing default behavior, while fully matched proofs treat missing null counts as unknown. This reuses the existing statistics conversion path instead of adding a separate null-count conversion pass.
I think this is a setting on the reader that controls how missing statistics are interpreted (as older versions of arrow-rs didn't write null counts when there were 0 nulls)
I am not sure why this code path is changing its value
There was a problem hiding this comment.
I checked the current DataFusion parquet reader options and I don’t think this is currently exposed as a reader-level DataFusion option (We can open another PR for it!). The existing behavior was implicitly the StatisticsConverter default, which treats missing null counts as zero.
So the regular pruning path keeps that existing behavior.
The fully matched path is intentionally stricter because it is proving every row passes the predicate. For that proof, missing null counts must remain unknown; otherwise nullable columns with missing null_count can be incorrectly marked fully matched.
I’ll add comments/named constructors to make this distinction clearer.
| .map(|&i| &groups[i]) | ||
| .collect::<Vec<_>>(), | ||
| arrow_schema, | ||
| missing_null_counts_as_zero: false, |
There was a problem hiding this comment.
why is this always false? Maybe it is worth some comments explaining what is going on here
|
Thanks @xudong963 |
## Which issue does this PR close? Closes apache#25239. Follows up on apache#21907 and [Arrow apache#6485](apache/arrow-rs#6485). ## Rationale for this change An absent Parquet `null_count` is unknown. Treating it as zero can discard NULL rows during `NULLS FIRST` TopK and `IS NULL` pruning, and let metadata-only `COUNT(column)` return an incorrect result. ## What changes are included in this PR? Preserve missing null counts in the shared row-group statistics adapter and file statistics. Remove the per-caller compatibility flag so static pruning, runtime pruning, and fully matched proofs use the same interpretation. Add an upgrade guide covering affected writers, plan changes, and migration. ## What is the testing strategy for this PR? Regressions cover missing and explicit-zero counts, static pruning, file-statistics precision, `COUNT`, and TopK with ASC/DESC, NULLS FIRST/LAST, and dynamic pruning enabled/disabled. The affected cases fail without the fix. Extended workspace tests, strict all-target/all-feature Clippy, and the full lint suite pass. ## Are there any user-facing changes? Correct results for Parquet files with missing null counts. Affected files can lose `IS NULL` pruning, metadata-only `COUNT(column)`, and `NULLS FIRST` TopK pruning. Sort pushdown can also retain a full `SortExec` for `ORDER BY` on nullable columns, even when files are sorted and non-overlapping. This includes older files written with parquet-rs before 53.1.0, used by DataFusion releases before 42.1.0. The DataFusion 56 upgrade guide explains how to identify and rewrite affected files. Explicit counts retain their existing behavior. No public API changes. --------- Signed-off-by: peterxcli <[email protected]>
## Which issue does this PR close? Closes apache#25239. Follows up on apache#21907 and [Arrow apache#6485](apache/arrow-rs#6485). ## Rationale for this change An absent Parquet `null_count` is unknown. Treating it as zero can discard NULL rows during `NULLS FIRST` TopK and `IS NULL` pruning, and let metadata-only `COUNT(column)` return an incorrect result. ## What changes are included in this PR? Preserve missing null counts in the shared row-group statistics adapter and file statistics. Remove the per-caller compatibility flag so static pruning, runtime pruning, and fully matched proofs use the same interpretation. Add an upgrade guide covering affected writers, plan changes, and migration. ## What is the testing strategy for this PR? Regressions cover missing and explicit-zero counts, static pruning, file-statistics precision, `COUNT`, and TopK with ASC/DESC, NULLS FIRST/LAST, and dynamic pruning enabled/disabled. The affected cases fail without the fix. Extended workspace tests, strict all-target/all-feature Clippy, and the full lint suite pass. ## Are there any user-facing changes? Correct results for Parquet files with missing null counts. Affected files can lose `IS NULL` pruning, metadata-only `COUNT(column)`, and `NULLS FIRST` TopK pruning. Sort pushdown can also retain a full `SortExec` for `ORDER BY` on nullable columns, even when files are sorted and non-overlapping. This includes older files written with parquet-rs before 53.1.0, used by DataFusion releases before 42.1.0. The DataFusion 56 upgrade guide explains how to identify and rewrite affected files. Explicit counts retain their existing behavior. No public API changes. --------- Signed-off-by: peterxcli <[email protected]>
Which issue does this PR close?
Rationale for this change
This is split out from review feedback on #21637. Row groups can only be marked fully matched when all rows are guaranteed to pass the filter. For nullable predicate columns, proving
NOT(predicate)is not enough because rows where the predicate evaluates to NULL do not pass the filter.What changes are included in this PR?
This PR makes the fully matched row-group proof conservative for nulls by adding
IS NULLchecks for nullable columns referenced by the predicate before evaluating the inverted pruning predicate.It also threads
with_missing_null_counts_as_zerothroughRowGroupPruningStatisticsso normal row-group pruning keeps the existing default behavior, while fully matched proofs treat missing null counts as unknown. This reuses the existing statistics conversion path instead of adding a separate null-count conversion pass.Are these changes tested?
Added a regression test covering row groups with known nulls, known zero nulls, and missing null counts.
Are there any user-facing changes?
No API changes. This only prevents false positives in the row-group fully matched optimization.