jensholdgaard commented on PR #24770: URL: https://github.com/apache/datafusion/pull/24770#issuecomment-5648642028
> **Disclosure:** this comment and the commit it describes were produced with AI assistance (Claude Code) and reviewed by me before pushing. Pushed 3064b5f with the deferral, @kosiew. **The fix.** `prepare_open_file` now records the stats-derived constant columns the predicate actually references alongside the provisional flag, and `prepare_filters` clears the flag if any of them is absent from the physical file schema. Withdrawing the proof is the conservative direction: it restores the existing missing-column behaviour rather than inventing new pruning. I did not go the provenance route inside `Statistics` — it fixes the class rather than this instance, but it changes a struct every consumer reads, and I do not think this PR has earned that. **On the tests, which is where I was weakest.** The mixed regression now takes its statistics from `statistics_from_parquet_metadata` through a new `write_parquet_with_collected_statistics` helper, instead of hand-building them, and asserts that the absent column really does collect as all-NULL. I checked that it fails without the fix — it prunes the file to zero rows where it should scan its three. The previous version passed for the wrong reason, as you said. There is also a mirror test now: same predicate, same statistics source, but with the column physically present and genuinely all NULL, where the collapse is real proof and pruning must still happen. Without that one the fix could have quietly degraded into never trusting a NULL constant at all, which would have regressed #24769 again. Locally: `datafusion-datasource-parquet` 254 tests pass, the `datafusion` lib suite 444 pass, including the `test_pushdown_with_missing_*` set this touches. **One thing I would rather raise than silently fix.** While tracing this I think I found a pre-existing tension that is not mine to change here. The prepare-time statistics substitution replaces an absent column with the statistics NULL *before* the adapter runs, so a custom `PhysicalExprAdapter`'s non-null default for that column never reaches the pushed-down filter. If I am reading that right it holds on main independently of this PR, on every version since `collect_statistics` became a session default. Happy to open a separate issue if it is not already tracked — I would rather not widen this PR to chase it. -- 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]
