[branch-55] fix: apply struct field filters when the file schema needs adaptation (#24125) - #24530
Open
alamb wants to merge 2 commits into
Open
[branch-55] fix: apply struct field filters when the file schema needs adaptation (#24125)#24530alamb wants to merge 2 commits into
alamb wants to merge 2 commits into
Conversation
…apache#24125) ## Which issue does this PR close? - Closes apache#24109. ## Rationale for this change With `datafusion.execution.parquet.pushdown_filters = true`, a filter on a struct field returns **all rows** when the declared table schema differs from the physical file schema for that column: ```sql -- file stores s as Struct<x: Int32>, table declares Struct<x: BIGINT> SELECT id, s['x'] FROM t WHERE s['x'] = 200; -- returns 3 rows instead of 1 ``` The planning-time decision and the runtime construction disagree: 1. `ParquetSource::try_pushdown_filters` evaluates `can_expr_be_pushed_down_with_schemas` against the **table** schema. `get_field(s, 'x')` has a bare column under the `get_field`, so it reports the predicate as fully handled and `FilterExec` is removed from the plan. 2. At open time the expression adapter rewrites the predicate against the **file** schema. Because the struct types differ, `rewrite_column` wraps the whole column in a cast, giving `get_field(cast(s AS Struct<x: Int64>), 'x')`. 3. `PushdownChecker` only recognizes `get_field` whose first argument is a `Column`. It now sees a `CastExpr`, falls through to normal traversal, hits the struct `Column`, and rejects pushdown — so no row filter is built and the conjunct is silently dropped. Nothing applies the predicate, and the scan returns unfiltered rows. ## What changes are included in this PR? Narrow the cast to the field that is actually read, in `DefaultPhysicalExprAdapter`: ``` get_field(cast(s AS Struct<x: Int64>), 'x') -> cast(get_field(s, 'x') AS Int64) ``` Expressions are rewritten bottom-up, so the new `try_narrow_struct_cast` matches the `get_field` node after its struct argument has already been wrapped, and rebuilds the `get_field` over the uncast struct (recomputing its return field from the physical field type) with the cast moved outside. This keeps the column visible under the `get_field`, so the Parquet row filter builder makes good on what planning promised. Two details worth calling out: - A field that is missing from the file collapses to a typed null literal, matching what the struct cast would have produced (DataFusion's struct casts match by name and fill missing target fields with nulls). - `get_field` on a `Map` column is a runtime key lookup rather than a schema-level field access, so map values keep the whole-column cast. As a side effect this also avoids materializing an entire cast struct just to read one field, which is a small win for any struct-field access over an evolved schema — not only for filters. ### Not addressed here The issue also raises the broader concern that "a static determination made at planning time about what the scan can do, and the runtime construction that has to make good on it, are computed by different code against different schemas, and there is no mechanism forcing them to agree." This PR fixes the reported wrong-results bug; it does not add a mechanism (e.g. post-decode filtering in `ParquetOpener`) that would make any future divergence safe by construction. That seems worth doing separately. ## Are these changes tested? Yes. - `datafusion/physical-expr-adapter/src/schema_rewriter.rs`: unit tests for the narrowed cast (flat and nested field access), the missing-field null literal, and that Map columns keep their cast. - `datafusion/datasource-parquet/src/opener/mod.rs`: end-to-end opener tests reading a `Struct<x: Int32>` file through a `Struct<x: Int64>` table schema with pushdown enabled, plus a matching-schema control. - `datafusion/sqllogictest/test_files/parquet_filter_pushdown.slt`: a SQL-level regression test. Verified that it fails on `main` (returns all 3 rows) and passes with the fix. Full runs: `cargo clippy --all-targets --all-features -- -D warnings`, the complete sqllogictest suite (498 files), `datafusion-physical-expr-adapter`, `datafusion-datasource-parquet`, and the `datafusion` `core_integration` / `parquet_integration` suites all pass. ## Are there any user-facing changes? A wrong-results bug fix: struct-field predicates are now applied when the scan needs schema adaptation. No public API changes. --- _Generated by [Claude Code](https://claude.ai/code/session_01MebN5PsVnYvXUeVKju5K7P)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
18 tasks
branch-55 does not have apache#24130/apache#24315, which taught nested schema pruning to union the leaves needed by mixed whole-column + field-access reads. Without that, `select s, s['y'] from narrow` falls back to reading every physical leaf instead of clipping to the narrow schema, so bytes_scanned is 219 (matching the unclipped full_schema read) rather than 146.
Contributor
Author
|
I am not sure about 7809b9b -- it would be nice if @adriangb or @zhuqi-lucas could check that |
alamb
marked this pull request as ready for review
August 20, 2026 19:27
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## branch-55 #24530 +/- ##
=============================================
+ Coverage 81.14% 81.16% +0.01%
=============================================
Files 1110 1110
Lines 386305 386653 +348
Branches 386305 386653 +348
=============================================
+ Hits 313474 313808 +334
- Misses 54358 54367 +9
- Partials 18473 18478 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
adriangb
approved these changes
Aug 20, 2026
adriangb
left a comment
adriangb
left a comment
Contributor
There was a problem hiding this comment.
I do think it'd be nice to backport this. It is a somewhat niche fix, so I'm also open to not backporting it to keep the backports leaner until someone asks for it. Approving and you can decide if you want to merge or leave open for later decision.
Contributor
Author
I think that since it was a regression, we should fix it |
Contributor
Reminder it was a regression in 53->54, not 54->55. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
55.1.0(minor/patch) Release (Sep 2026) #24462branch-55(for 55.1.0, tracked in Release DataFusion55.1.0(minor/patch) Release (Sep 2026) #24462).get_fieldpredicate when the file needs schema adaptation (wrong results) #24109Rationale for this change
With
datafusion.execution.parquet.pushdown_filters = true, a predicate on a struct field was reported as fully handled by the scan whenever the file needed schema adaptation, soFilterExecwas removed from the plan and the predicate was silently dropped — returning every row instead of the filtered set. This is a correctness bug (wrong results), not specific to 55.0.0, so it fits the backport criteria.What changes are included in this PR?
Cherry-pick of #24125 (commit 40c208e). Git's recursive merge auto-resolved surrounding context differences in
datafusion/physical-expr-adapter/src/schema_rewriter.rsanddatafusion/sqllogictest/test_files/parquet_nested_schema_pruning.slt; no manual conflict resolution or adaptation of the fix itself was required.One follow-up commit adapts a test expectation:
branch-55doesn't have #24130/#24315, which taught nested schema pruning to union the leaves needed by mixed whole-column + field-access reads (e.g.select s, s['y'] from narrow). Without that optimization, the mixed-access case falls back to reading every physical leaf, sobytes_scannedis219here instead of the146the original PR's test expects onmain. This is a pre-existing difference in pruning capability, not a correctness regression from this fix.Are these changes tested?
Yes. Carries the original regression coverage, all tests pass.
Are there any user-facing changes?
WHERE s['field'] = ...predicates on struct columns now filter correctly when Parquet filter pushdown requires schema adaptation. No API changes.