[branch-55] fix: apply struct field filters when the file schema needs adaptation (#24125) by alamb · Pull Request #24530 · apache/datafusion

alamb · 2026-08-20T18:14:29Z

Which issue does this PR close?

Part of Release DataFusion 55.1.0 (minor/patch) Release (Sep 2026) #24462
Backport of fix: apply struct field filters when the file schema needs adaptation #24125 to branch-55 (for 55.1.0, tracked in Release DataFusion 55.1.0 (minor/patch) Release (Sep 2026) #24462).
Fixes Parquet filter pushdown silently drops a get_field predicate when the file needs schema adaptation (wrong results) #24109

Rationale 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, so FilterExec was 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.rs and datafusion/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-55 doesn'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, so bytes_scanned is 219 here instead of the 146 the original PR's test expects on main. 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.

…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>

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.

alamb · 2026-08-20T19:27:48Z

I am not sure about 7809b9b -- it would be nice if @adriangb or @zhuqi-lucas could check that

codecov-commenter · 2026-08-20T19:32:58Z

Codecov Report

❌ Patch coverage is 96.83908% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.16%. Comparing base (520f389) to head (7809b9b).
⚠️ Report is 1 commits behind head on branch-55.

Files with missing lines	Patch %	Lines
...usion/physical-expr-adapter/src/schema_rewriter.rs	96.83%	7 Missing and 4 partials ⚠️

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

adriangb

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.

alamb · 2026-08-20T19:53:53Z

Approving and you can decide if you want to merge or leave open for later decision.

I think that since it was a regression, we should fix it

adriangb · 2026-08-20T20:19:25Z

Approving and you can decide if you want to merge or leave open for later decision.

I think that since it was a regression, we should fix it

Reminder it was a regression in 53->54, not 54->55.

zhuqi-lucas

Nice to backport, thanks!

…he#24680) ## Which issue does this PR close? Closes apache#24679. Follow-up to apache#24125 and apache#24530. ## Rationale for this change When Parquet files have different schemas, DataFusion may need to convert a file's Struct column to the table's logical Struct type. If a query only reads `s.x`, struct-cast narrowing can save work by converting just `x` instead of every field in `s`. That optimization must preserve the meaning of the original expression. The earlier narrowing rule could hide an explicit cast's error, change the result's nullability, or make an entirely null input fail during decimal conversion. ### Selecting one field must not hide an explicit cast error For `s = {x: 1, y: 'bad'}`, consider this physical expression: ```sql get_field(CAST(s AS STRUCT<x INT, y INT>), 'x') ``` The explicit cast asks to convert both fields. It must fail because `'bad'` cannot become an integer, even though the caller only uses `x`. Narrowing it to `CAST(get_field(s, 'x') AS INT)` instead returns `1`: the conversion of `y`, and its error, have disappeared. This failure is reproduced directly at the physical-expression adapter boundary. The distinction is whether a cast was inserted to reconcile file and table schemas or was already part of the query; the two can have the same shape but different obligations. ### An entirely null Struct should not acquire a decimal-conversion error Consider a Parquet batch with this schema evolution: ```text File schema: s: Struct<x: Utf8> Table schema: s: Struct<x: Decimal128(10, -1)> Batch values: s is NULL for every row ``` DataFusion's whole-Struct conversion returns nulls without converting its children. If the rewrite extracts `x` first, however, it invokes string-to-decimal conversion on an array of null strings. Arrow rejects the negative scale while setting up that conversion, before looking at any values. A query that should return nulls now fails despite there being no non-null value to convert. The same problem occurs inside a selected container. For example, evolving `s.x` from `List<Utf8>` to `List<Decimal128(10, -1)>` can fail even when every parent Struct is null. Checking only whether `x` itself is a decimal misses the conversion inside the List. This case also reproduces through a Parquet scan with filter pushdown: `WHERE get_field(s, 'x') IS NULL` should select every row in the batch, not fail while preparing a decimal conversion. There is a related schema-contract problem. A required child `x` inside a nullable logical parent `s` still gives a nullable result for `s.x`. Rebuilding an expression from the child's Field alone can lose that inherited nullability, including through nested parents. ## What changes are included in this PR? The adapter now distinguishes casts it introduces for schema adaptation from casts already present in the query. It can continue narrowing generated conversions where that is safe while preserving explicit Struct casts and their errors. Narrowing also preserves the original logical result Field, including metadata and nullability; matching scalar types alone do not establish an equivalent result. Cast tracking also avoids quadratic lookup work for large expressions. For the covered decimal changes, the conversion stays inside its Struct ancestors so DataFusion's existing casting code retains control over the all-null shortcut. The decision looks through container value types too, so selecting a List, Map, or Dictionary does not hide a decimal conversion from the check. The cast target keeps only the selected field path, excluding conversions for unselected siblings. Matching types still narrow normally, and existing container-to-Struct conversion paths are preserved. This extends the existing fix without duplicating Arrow's conversion-validation rules or changing the underlying casting implementation. Unwrapping a Dictionary whose values are Structs retains its existing all-null decimal limitation; the built-in Parquet reader does not produce that source shape. The Parquet reader must then be able to evaluate the retained expression after schema adaptation. This matters when the query plan has already delegated a predicate to the reader: retaining a cast must not prevent that filter from executing. The runtime allowance is specifically for field access through a Struct-to-Struct cast of a column. The reader preserves the cast and reuses projection's existing read clipping to decode only the leaves named by the cast target, with a full-root fallback when clipping is unsafe. The target, rather than the final field selection, determines which conversions must run: an explicit cast that names `y` still reads and converts `y` even if the query selects only `x`. Filter cost estimates use the same selected leaves. Planning remains conservative about explicit casts, keeping a residual filter for them. ## Are these changes tested? The Rust regressions cover explicit cast errors, nullability inherited from parents, decimal conversions on entirely null Structs, unselected sibling conversion errors, and execution through Parquet filters. Container cases include the List families, Maps, Dictionaries, and deeper nesting, with checks that matching types and existing container-to-Struct conversions keep their previous behavior. The existing `schema_evolution_nested.slt` now adds SQL coverage using generated Parquet files: an explicit Struct cast whose unselected sibling must still fail, all-null scalar and List decimal projections, and null predicates with filter pushdown both enabled and disabled. Statistics and pruning shortcuts are disabled for the filter cases. These SQL tests supplement the Rust tests, which also check adapter provenance, logical Field metadata/nullability, expression shape, and Arrow types that are difficult to express in SQL. The explicit-cast SQL case is an integration control; the direct adapter test exercises the cast-provenance regression. The retained-cast read-plan regression now uses three physical siblings. It verifies that an unused sibling is pruned, a sibling named by the cast still raises its conversion error, the projected physical schema is correct, and the estimated compressed bytes count only the selected leaves. A target containing just the selected field also decodes and evaluates successfully. Local validation for [ec4b3f2](apache@ec4b3f2) used the tracked `Cargo.lock` unchanged, with `--locked` for the Cargo test and all-feature Clippy commands: - The required extended workspace suite passed **10,795 Rust tests** (eight existing ignores) and **all 505 SQL logic files**. - All **107 CLI tests** passed. - The adapter and Parquet datasource crates also passed separately: **291 unit tests and eight doctests**, with five existing doctest ignores. - `cargo fmt --all`, full-workspace Clippy across all targets and features with warnings denied, and the complete `./dev/rust_lint.sh` suite passed. The strengthened read-plan regression was also checked with the old full-root collection behavior restored: it failed because it selected leaves `[1, 2, 3]` instead of `[1, 2]`. Restoring the fix made it pass again. <details> <summary>Existing benchmark comparison</summary> The existing benchmarks were compared against `main` at `ee59f628b44eb80e8a4f126288632ef51fda5dc2`, using `release-nonlto`, freshly built workspace artifacts for each revision, and the checked-in Criterion settings. Variants ran sequentially after builds and tests had stopped. | Existing benchmark case | Result | | --- | --- | | `parquet_struct_filter_pushdown`: `select_id/with_pushdown` | No change detected (p = 0.83; 100 samples per variant). | | `parquet_nested_schema_pruning`: `top_level_struct/select_struct_narrow_schema` | PR/main mean-time changes were +4.15%, -2.96%, and -2.84% across three repeats, including reversed execution order (10 samples per variant). The direction changed, so no stable timing conclusion is claimed. | Scan-byte counts were identical in every projection run. These cases cover ordinary field-filter pushdown and the existing projection-clipping path; neither directly measures retained-cast row-filter speedup. The `main` snapshot also contains unrelated commits, so this comparison does not isolate the follow-up patch. </details> ## Are there any user-facing changes? The intended changes preserve explicit-cast errors, logical field metadata and nullability, and the covered all-null decimal behavior. Ordinary field pruning remains enabled. Retained-cast Parquet filters can also avoid reading sibling leaves outside the cast target; required conversions are preserved, with a full-root fallback where necessary. There are no public API or dependency changes. General changes to container-to-Struct conversion semantics, generic `get_field` behavior under null parents, and masking of encoded arrays remain outside this PR's scope. AI assistance: Codex assisted with the implementation, regression tests, PR text, and the stated local checks and source reviews. The row-filter clipping follow-up incorporates Adrian Garcia Badaracco's [recommended change](pydantic@9746f3f), with additional regression coverage. --------- Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>

alamb mentioned this pull request Aug 20, 2026

Release DataFusion 55.1.0 (minor/patch) Release (Sep 2026) #24462

Open

23 tasks

github-actions Bot added the sqllogictest SQL Logic Tests (.slt) label Aug 20, 2026

alamb marked this pull request as ready for review August 20, 2026 19:27

adriangb approved these changes Aug 20, 2026

View reviewed changes

zhuqi-lucas approved these changes Aug 21, 2026

View reviewed changes

Dandandan approved these changes Aug 21, 2026

View reviewed changes

Dandandan merged commit 045b590 into apache:branch-55 Aug 21, 2026
38 checks passed

This was referenced Aug 25, 2026

Struct-cast narrowing can suppress cast errors and change field semantics #24679

Closed

fix: preserve explicit Struct casts and evolved field semantics #24680

Merged

Navigation Menu

Uh oh!

[branch-55] fix: apply struct field filters when the file schema needs adaptation (#24125) - #24530

[branch-55] fix: apply struct field filters when the file schema needs adaptation (#24125)#24530
Dandandan merged 2 commits into
apache:branch-55from
alamb:alamb/backport_24125

alamb commented Aug 20, 2026 •

edited

Loading

Uh oh!

alamb commented Aug 20, 2026

Uh oh!

codecov-commenter commented Aug 20, 2026

Uh oh!

adriangb left a comment

Uh oh!

alamb commented Aug 20, 2026

Uh oh!

adriangb commented Aug 20, 2026

Uh oh!

zhuqi-lucas left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

alamb commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

alamb commented Aug 20, 2026

Uh oh!

codecov-commenter commented Aug 20, 2026

Codecov Report

Uh oh!

adriangb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented Aug 20, 2026

Uh oh!

adriangb commented Aug 20, 2026

Uh oh!

zhuqi-lucas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alamb commented Aug 20, 2026 •

edited

Loading