jensholdgaard commented on PR #24770: URL: https://github.com/apache/datafusion/pull/24770#issuecomment-5648584086
> **Disclosure:** this reply was drafted with AI assistance (Claude Code) and reviewed by me before posting. @kosiew you are right, and thank you for catching it — this is a real hole and not a hypothetical one. I went and traced it rather than taking it on trust, and the mechanism is exactly as you describe. In `statistics_from_parquet_metadata`, when `StatisticsConverter::try_new` cannot find a physical Parquet column for a logical field, the `Err` branch sets `null_counts_array[idx] = Precision::Exact(num_rows)`. That is documented behaviour, not an accident — the function's own doc comment spells it out: > 2. The column is in arrow schema, but not in parquet schema due to schema revolution, min/max values are set to `Precision::Exact(null)` > - Null counts are set to `Precision::Exact(num_rows)` So an absent column and a physically present all-NULL column are represented identically by the time `constant_value_from_stats` sees them. It emits a NULL constant for the absent one, `stats_alone_unsatisfiable` collapses `a = 1 AND b = 2` to NULL, and my flag prunes — which is precisely the missing-column behaviour the commit was supposed to preserve. I reintroduced the bug I set out to avoid. **Both of my test defences were hollow, and I should have checked that before asking for another look.** `test_no_prune_when_missing_column_collapses_mixed_predicate` leaves the missing column's statistics as `Statistics::new_unknown`, so the constant never forms and the assertion passes for the wrong reason. And the `evolved_schema*` tests I pointed at as existing coverage go through the `RoundTrip` helper, which builds its file group with `meta.into_iter().map(Into::into)` — no statistics attached at all. My flag could never have fired in any of them. Citing them as evidence was not justified. **One thing that makes this worse than I first framed it.** I had been thinking of the regression as wasted work, on the assumption that a skipped missing-column file would have produced no rows anyway. That is not guaranteed. `schema_rewriter.rs` documents that a custom `PhysicalExprAdapter` may fill a missing column with a non-null default rather than NULL, so a row group that the default would have matched can be dropped. That puts this in correctness territory, which I think supports your blocking call rather than arguing with it. **On the two options you offered, I would like to take the deferral one.** Tagging provenance inside `Statistics` — something like distinguishing a synthesized all-NULL from a collected one — would change a struct that many consumers read, and I do not think this PR has earned the right to do that. Deferring is local: the flag is currently computed in `prepare_open_file`, but `physical_file_schema` is first known in `prepare_filters`, so moving the check there and dropping constants for columns absent from the physical schema keeps the decision inside the code that already owns schema evolution. If you would rather see the provenance approach because it fixes the class of problem instead of this instance, say so and I will go that way instead — you have much more context on where this code is heading than I do. For tests I plan to rewrite the mixed regression so the missing column actually carries `null_count = Exact(3)` — preferably via real collected statistics rather than hand-built ones, so the test cannot drift away from what the reader produces — plus a positive counterpart where the column is physically present and genuinely all-NULL and pruning *must* still happen. Without that second one, the fix could pass by simply disabling the feature. **Separately, and I think outside this PR's scope, something I noticed while digging.** The prepare-time statistics substitution already replaces an absent column with a NULL literal before the adapter runs, on every version since `collect_statistics` became a session default. If that reading is right, a custom adapter's non-null default never reaches the pushed-down filter today, independent of my change. I would rather not widen this PR to chase that, but I am happy to open a separate issue if it is not already known. Last thing, offered as context rather than excuse. It has been a long time since I worked on language and query-engine internals directly; my day-to-day instincts come from shipping breaking changes to our own logs DSL in a much smaller project, where the blast radius of getting a rewrite wrong is a handful of users rather than every DataFusion consumer. So I am likely to misjudge upstream conventions here, and I would rather be corrected early than have you find it in review twice. I appreciate you reading carefully enough to find this one. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
