fix: prune row groups when file statistics collapse the predicate to a constant - #24770
fix: prune row groups when file statistics collapse the predicate to a constant#24770jensholdgaard wants to merge 2 commits into
Conversation
…stant Closes apache#24769. `constant_columns_from_stats` substitutes columns that file statistics prove constant — including the all-NULL case, which folds to a NULL literal. When the substituted predicate then simplifies to a bare constant (`NULL`, or `false`), `build_pruning_predicates` returns `None` because there are no column references left, and the opener falls through with no pruning at all. That makes the substitution strictly counterproductive for these files. A predicate like `col = <literal>` over a column whose statistics say `null_count == row_count` is provably unsatisfiable, and before the substitution existed it *was* pruned via the `col_null_count != row_count` conjunct of the pruning predicate. The regression bisects to apache#22969, which enabled `collect_statistics` by default and so began feeding the substitution on paths that previously had no file statistics. Recognise the collapsed-to-constant case explicitly: if the predicate is a `false`/NULL literal and the original predicate referenced a column that `constant_columns_from_stats` proved constant, skip every remaining row group and credit the skip to statistics pruning. Results are unchanged either way — a filter drops NULL and false rows alike — but the scan work is not. The `stats_constants_in_predicate` guard deliberately narrows the skip to collapses that file statistics contributed to. A predicate can also collapse via the missing-column adapter (schema evolution) or partition-value folding; both keep their existing behaviour — partition-driven collapse is already `FilePruner`'s job, and widening the skip to missing columns changes the raw scan output that nine `evolved_schema` / `test_pushdown_with_missing_*` tests assert. The regression test asserts the `row_groups_pruned_statistics` metric rather than the row count, since a row-count assertion cannot tell "pruned" from "scanned, then row-filtered": both yield zero rows.
37f208e to
3f41e77
Compare
|
The The nine failing The update narrows the skip with a
If maintainers would rather generalise the skip to the missing-column collapse as well, I'm happy to do that in a follow-up with the corresponding test updates — it just seemed wrong to smuggle a second behaviour change into a targeted regression fix. |
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on this. The overall direction makes sense, and restoring pruning for predicates that collapse to false or NULL from file statistics addresses the regression nicely.
I found one case that I think needs to be addressed before merging. The current guard tracks whether the predicate references any stats-derived constant, but not whether that substitution is actually what made the predicate unsatisfiable. This can change behavior for mixed predicates involving schema-evolution missing columns.
I also left a small non-blocking test suggestion to cover the non-NULL false path.
| &logical_file_schema, | ||
| )); | ||
| ); | ||
| let stats_constants_in_predicate = !stats_constants.is_empty() |
There was a problem hiding this comment.
I think this guard is a little too broad. stats_constants_in_predicate tells us that the predicate references some column proven constant by file statistics, but it does not tell us that the stats substitution is what caused the final predicate to become false or NULL.
For example, consider a = 1 AND missing_b = 2 on a file where statistics prove a = 1. This guard passes because a is a stats-derived constant. Stats folding leaves that conjunct as true, but the missing-column adapter can then fold missing_b = 2 to NULL or false. We would then skip all row groups even though the stats substitution itself did not prove the predicate unsatisfiable.
Before this change, that case follows the intentionally preserved missing-column path and scans/filter-prunes instead. Could we track whether the stats substitution itself makes the predicate unsatisfiable, before missing-column or partition rewriting, or otherwise exclude this mixed case?
It would also be good to add a regression test with one stats-constant present column and one missing column.
There was a problem hiding this comment.
Written by Claude, an AI assistant, on behalf of the PR author.
Agreed, and addressed in 6f86ec7. The flag recorded only that the predicate referenced a column statistics had proven constant, then treated any later collapse to false/NULL as proof that statistics caused it. Those are two different claims, and the a = 1 AND missing_b = 2 example separates them.
The check now asks the narrower question at substitution time: substitute only the statistics-derived constants, simplify that, and take a collapse as proof. Anything the missing-column adapter or partition folding does afterwards is irrelevant, because the proof already stands on the file's own statistics. In the mixed case, missing_b = 2 is left standing as a real column reference, nothing collapses, and the file takes the existing missing-column path. That also drops the Literal downcast from prune_row_groups, which is now just the two flags; collect_columns stays as the cheap short-circuit so the simplifier only runs when a stats constant is actually referenced. The simplifier is given the full table schema, since the substitution deliberately leaves non-constant columns in place and it types each node as it walks.
test_no_prune_when_missing_column_collapses_mixed_predicate covers exactly that shape: a stats-constant present column plus a missing column, asserting the statistics branch does not claim the prune and the file is scanned (3 rows returned, 0 row groups pruned). The evolved_schema* and test_pushdown_with_missing_* tests are unchanged and pass.
| @@ -3023,6 +3075,100 @@ mod test { | |||
| assert_eq!(num_rows, 0); | |||
There was a problem hiding this comment.
Nice to have coverage for the all-NULL case. Could we also add a small test where exact non-NULL min/max statistics prove a column constant and a = <different literal> simplifies to false? That would explicitly cover both the NULL and false branches handled by the new pruning logic.
There was a problem hiding this comment.
Written by Claude, an AI assistant, on behalf of the PR author.
Added in 6f86ec7 as test_prune_exact_constant_column_false_predicate_from_file_statistics: exact min == max == 7 statistics prove a constant, and a = 8 folds to false, so the row group is pruned on that proof. Together with the all-NULL case, both literal values the branch accepts are now covered. The row_groups_pruned_statistics accessor is hoisted into a shared test helper, since all three tests assert on that metric rather than the row count, which cannot tell "pruned" from "scanned, then row-filtered".
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24770 +/- ##
==========================================
+ Coverage 81.52% 81.59% +0.06%
==========================================
Files 1123 1123
Lines 405970 411094 +5124
Branches 405970 411094 +5124
==========================================
+ Hits 330983 335437 +4454
- Misses 55626 55910 +284
- Partials 19361 19747 +386 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Review feedback: `stats_constants_in_predicate` recorded only that the predicate *referenced* a column file statistics proved constant, then treated any later collapse to `false`/NULL as proof that statistics caused it. Those are different claims. For `a = 1 AND missing_b = 2` on a file whose statistics prove `a = 1`, the flag is set by `a`, but the collapse to NULL comes from the missing-column adapter folding `missing_b`. The skip fired on a predicate the statistics had not disproven, taking the schema-evolution path's files out of the scan. Ask the narrower question instead: substitute *only* the statistics-derived constants and simplify that. If it collapses, the statistics alone have proven no row can match, whatever the adapter or partition folding do afterwards. The mixed predicate above leaves `missing_b = 2` standing as a real column reference, nothing collapses, and the missing-column path keeps its behaviour. Deciding it at substitution time also drops the literal downcast from `prune_row_groups`, which is now just the two flags. `collect_columns` stays as the cheap short-circuit so the simplifier only runs when a stats-derived constant is actually referenced. The simplifier is given the full table schema, since the substitution leaves non-constant columns in place and it types each node as it walks. Tests: the mixed stats-constant + missing-column case asserts the branch does *not* claim the prune and the file is scanned; exact non-NULL min/max statistics with `a = <other literal>` cover the `false` branch alongside the existing all-NULL NULL branch. The pruning-metric accessor is hoisted to a shared test helper. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y2NNRu7nTGmcK4BoWUy3s2
kosiew
left a comment
There was a problem hiding this comment.
Thanks for the follow-up here. The new stats_prove_unsatisfiable check is heading in the right direction, and I like that it is computed before the partition and schema adapters run. The added NULL and exact non-NULL cases also cover the two constant outcomes well.
I think there is still one blocking schema-evolution case, though. Collected file statistics can represent a column that is physically absent from the Parquet file as all NULL, which means that missing column can still enter stats_constants. In that case the new statistics-only check can classify the predicate as unsatisfiable and skip the row group, even though the intent is to preserve the existing missing-column behavior.
The mixed regression test does not currently exercise that path because the missing column's statistics are left unknown. I think the fix needs to distinguish an all-NULL constant that came from an actually present physical column from one synthesized for an absent column, or otherwise defer this attribution until the physical schema is known.
Once that case is covered, this should be in much better shape.
| // but since we use a HashMap, we'll just overwrite the partition values with the | ||
| // constant values from statistics (which should be the same). | ||
| literal_columns.extend(constant_columns_from_stats( | ||
| let stats_constants = constant_columns_from_stats( |
There was a problem hiding this comment.
I think there is still a schema-evolution hole here. DFParquetMetadata::statistics_from_parquet_metadata gives a logical column that is absent from the physical Parquet schema null_count = Exact(num_rows). constant_value_from_stats then turns that into a NULL entry in stats_constants.
So for a file that physically contains only a, collected stats can still make missing b look like a stats-derived NULL constant. With a = 1 AND b = 2, stats_alone_unsatisfiable can simplify the whole predicate to NULL, set stats_prove_unsatisfiable, and skip the row group through the statistics path.
That seems to bring back the missing-column row-group skip that this change is trying to avoid. The new mixed regression currently leaves b statistics unknown, so it does not reproduce what real collected schema-evolution statistics provide.
Could we preserve whether an all-NULL constant came from a physically absent column, or defer this attribution until the physical schema is known? I would also update the regression so b.null_count = Exact(3), or better, use real collected statistics for the missing-column case.
Note
This fix was investigated and written with AI assistance (Claude Code), posted with the account owner's review and consent.
Which issue does this PR close?
col = <literal>when statistics show the column is entirely NULL #24769.Rationale for this change
A query like
SELECT ... WHERE col = 'x'over a Parquet file whose statistics saycolis entirely NULL cannot match any row, and DataFusion 54 pruned such row groups from the footer without reading them. On 55 the same query reads them.The cause is an interaction rather than a bug in pruning itself.
constant_columns_from_statssubstitutes columns that file statistics prove constant, and its all-NULL branch folds such a column to a NULL literal. Once substituted, the predicate simplifies to a bare constant —NULLhere — at which pointbuild_pruning_predicatesreturnsNone(there are no column references left to build a pruning predicate over), andprune_row_groupsfalls through with no pruning at all.So for exactly the files where the statistics carry the most information, the substitution is strictly counterproductive: before it, the pruning predicate's own
col_null_count != row_countconjunct proved the row group empty and skipped it.This surfaced as a regression when bisecting a real workload (a Parquet log store where a
bodycolumn is NULL for the vast majority of rows) from 54 → 55. The bisect lands on #22969, which removedListingOptions::collect_statin favour of the session'sexecution.collect_statistics— defaulttrue. That change is correct in itself; it simply began feeding per-file statistics to the substitution on paths that previously had none, exposing the gap. Settingexecution.collect_statistics = falserestores pruning on 55, which is a useful confirmation but obviously not a fix.Results were never wrong — a filter drops NULL and false rows alike — but the scan work is real: row groups that used to be skipped from the footer are now decoded in full.
What changes are included in this PR?
prune_row_groupsnow recognises the collapsed-to-constant case: if the (post-substitution, post-simplification) predicate is afalseor NULL literal, every remaining row group is skipped and the skip is credited torow_groups_pruned_statistics. A smallRowGroupAccessPlanFilter::skip_allhelper is added alongside the existingprune_by_*methods.The check is deliberately narrow — a downcast to
Literalplus a NULL/falsevalue test — so it cannot affect predicates that still reference columns; those take the existing path unchanged.Are these changes tested?
Yes:
test_prune_all_null_column_equality_from_file_statisticsinopener/mod.rs, modelled on the neighbouringtest_prune_on_partition_values_and_file_statistics. It fails on currentmain(3 rows scanned, 0 pruned) and passes with this change.One note on how it asserts, in case it saves a reviewer time: it checks the
row_groups_pruned_statisticsmetric rather than the returned row count. A row-count assertion cannot distinguish "pruned" from "scanned, then row-filtered" — both give zero rows — and an earlier draft of this test passed against the unfixed code for exactly that reason.Verified
cargo test -p datafusion-datasource-parquet --lib opener::— 47 passed. The 16 pre-existingbloom_filter::testsfailures on this crate are unrelated and reproduce identically on an untouched checkout.Are there any user-facing changes?
No API or result changes. Queries that were already correct stay correct; affected scans read less data. Plans may show more row groups pruned, and
row_groups_pruned_statisticsincreases correspondingly.