fix: apply struct field filters when the file schema needs adaptation by adriangb · Pull Request #24125 · apache/datafusion

adriangb · 2026-08-05T19:58:21Z

Which issue does this PR close?

Closes 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 filter on a struct field returns all rows when the declared table schema differs from the physical file schema for that column:

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

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.
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').
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

adriangb · 2026-08-05T19:58:46Z

+    /// Filters on struct fields (`s['x'] = 200`) must still be applied when the
+    /// table schema disagrees with the physical file schema, which forces the
+    /// expression adapter to insert a cast.
+    ///
+    /// See <https://github.com/apache/datafusion/issues/24109>.
+    mod struct_field_pushdown {


Prefer SLT tests if possible

Copilot

Pull request overview

Fixes a wrong-results bug in Parquet filter pushdown when struct-field predicates are used and the declared table schema differs from the physical file schema. The change ensures the runtime schema adaptation preserves a get_field(Column(..), ...) shape so Parquet’s row-filter builder can still recognize and apply the predicate that planning-time pushdown claimed would be handled.

Changes:

Add a physical-expr rewrite that narrows cast(struct) under get_field into cast(get_field(..)), preserving pushdown compatibility and avoiding casting whole structs unnecessarily.
Add unit tests covering flat, nested, and flattened multi-key get_field paths, missing-field-to-typed-null behavior, and Map behavior (no narrowing).
Add a SQL logic regression test reproducing issue #24109 and validating correct filtering for both schema-adapted and matching-schema reads.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
datafusion/physical-expr-adapter/src/schema_rewriter.rs	Introduces `try_narrow_struct_cast` + field-path resolution and adds focused unit tests for the new rewrite behavior.
datafusion/sqllogictest/test_files/parquet_filter_pushdown.slt	Adds an end-to-end SQL regression test to prevent the struct-field predicate from being silently dropped under schema adaptation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

codecov-commenter · 2026-08-05T20:46:10Z

Codecov Report

❌ Patch coverage is 95.85635% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.07%. Comparing base (e6b4221) to head (f72ff6b).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...usion/physical-expr-adapter/src/schema_rewriter.rs	96.54%	8 Missing and 4 partials ⚠️
...ion/datasource-parquet/src/projection_read_plan.rs	80.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #24125      +/-   ##
==========================================
+ Coverage   81.06%   81.07%   +0.01%     
==========================================
  Files        1105     1106       +1     
  Lines      380457   380916     +459     
  Branches   380457   380916     +459     
==========================================
+ Hits       308399   308816     +417     
- Misses      53839    53876      +37     
- Partials    18219    18224       +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.

When the declared table schema differs from the physical file schema for a struct column, the expression adapter wraps the whole struct column in a cast, so `s['x']` becomes `get_field(cast(s AS Struct<..>), 'x')`. That hides the column from consumers that pattern match on `get_field(column, 'f')`. The Parquet scan is one such consumer: it decides at planning time (against the table schema) that a struct-field predicate can be evaluated as a row filter and reports it as fully handled, so `FilterExec` is removed from the plan. At runtime the row filter builder no longer recognizes the adapted expression, silently drops the predicate, and the query returns unfiltered rows. Narrow the cast to the field that is actually read: `get_field(cast(s AS Struct<..>), 'x')` becomes `cast(get_field(s, 'x') AS <type of x>)`. This keeps the column visible under the `get_field`, and also avoids materializing a whole cast struct just to read one field. Fields that are missing from the file collapse to a typed null literal, matching what the struct cast would have produced. `get_field` on a Map column is a runtime key lookup rather than a schema-level field access, so those keep the whole-column cast. Closes apache#24109. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MebN5PsVnYvXUeVKju5K7P

`get_field` has a flattened multi-key form: the logical simplifier rewrites `s['a']['b']` into `get_field(s, 'a', 'b')`. The narrowing rule only matched the two-argument form, so nested field access kept the whole-struct cast and stayed exposed to the wrong-results bug it was meant to fix. Resolve the full key path through nested struct fields on both the logical (cast target) and physical sides, and rebuild `get_field` with every key preserved. A path whose leaf is missing from the file still collapses to a typed null literal; a path that runs through a non-struct field is left alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MebN5PsVnYvXUeVKju5K7P

Drop the opener-level Rust tests in favour of SLT, which covers the same ground end to end through the planner. Adds a matching-schema control and a missing-field case alongside the existing adapted-schema tests, so the deleted Rust coverage is preserved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MebN5PsVnYvXUeVKju5K7P

…rowing Coverage analysis of the narrowing showed two reachable branches with no test behind them: a struct column that needs no adaptation at all, and one where only a sibling field forced the column-level cast, so the accessed field needs no cast of its own. Both matter — the first is the common case the rewrite must not disturb, the second is where the cast disappears rather than moving. The remaining uncovered branches in the function are guards against shapes that cannot reach it: a `get_field` with fewer than two arguments, and any key path running through a non-struct field, which the column-level cast validation rejects first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MebN5PsVnYvXUeVKju5K7P

`build_read_plan_with_cast_clipping` documented that a root reached by both a narrowing cast and a `get_field` access is "not produced by `DefaultPhysicalExprAdapter`, which always routes a `get_field` over a narrowed column through the same cast", and fell back to reading every leaf of that root when it saw one. Narrowing the struct cast makes the adapter produce exactly that shape: `SELECT s, s['y']` over a narrowed struct now reads the whole column through the cast and the field through a `get_field` on the bare column. The fallback then widened the read from the narrow schema's leaves to every physical leaf. Clip such a root to the cast target when the target still names every field those accesses read -- the clip keeps exactly the leaves the target names, so it keeps theirs too, and the clipped type still has the fields to read out of. A `get_field` naming something the target lacks would be starved by the clip, so that keeps falling back to a full read. This restores `select s, s['y'] from narrow` to the narrow-leaf read, and `select s['x'] from narrow` now clips all the way down to `x` (146 -> 75 bytes) because the field access no longer hides behind a whole-struct cast. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

zhuqi-lucas · 2026-08-06T15:23:52Z

+/// nested struct fields.
+fn resolve_field_path<'a>(fields: &'a Fields, path: &[&str]) -> FieldPathResolution<'a> {
+    let Some((field_name, rest)) = path.split_first() else {
+        return FieldPathResolution::NotAStruct;


Minor: an empty path here returns NotAStruct, which reads a little oddly — an empty path is not really "not a struct". It is unreachable given the non-empty field_name_exprs guard in try_narrow_struct_cast, so a one-line comment noting it is a defensive default would save the next reader a double-take.

zhuqi-lucas · 2026-08-06T15:23:54Z

        Ok(Transformed::no(expr))
    }

+    /// Rewrite `get_field(cast(s AS Struct<..>), 'f')` into


Nice fix. One design question for the record: did you consider teaching the pushdown side (PushdownChecker / row-filter builder) to see through the cast — i.e. recognize get_field(cast(col), 'f') — instead of narrowing it here?

I assume narrowing was chosen because (a) it avoids materializing the whole cast struct just to read one field, and (b) it fixes it at the source, so every consumer that pattern-matches get_field(column, 'f') benefits — not just the row filter — rather than loosening the pushdown contract to see through arbitrary casts. Worth capturing that rationale.

Relatedly, the PR notes the broader planning-vs-runtime schema divergence is intentionally out of scope — a tracking issue for the "safe by construction" mechanism (e.g. post-decode filtering in ParquetOpener) would be good so it is not lost. Happy to file it.

did you consider teaching the pushdown side (PushdownChecker / row-filter builder) to see through the cast — i.e. recognize get_field(cast(col), 'f') — instead of narrowing it here?

I agree that this fix seems very specific -- it almost seems like it is focusing on the symptom rather than the underlying problem (the physical / logical mismatch that @zhuqi-lucas is pointing out)

It seems like if this is a rewrite that should be done, shouldn't we be doing at a higher level 🤔

alamb · 2026-08-06T20:55:08Z


 # Mixed access -- the whole (narrowed) column and a subfield of it -- still
-# reads only the narrow schema's leaves.
+# reads only the narrow schema's leaves. The whole-column read goes through


I am not sure what this comment is trying to say -- it seems like it is trying to explain implementation details. I think we can remove it from here unless it is adding crititcal context past this PR

alamb · 2026-08-06T20:56:38Z

-# more precise, single-leaf pushdown path.
+# `get_field` on a schema-narrowed struct is rewritten to
+# `CAST(get_field(s, 'x'))` rather than `get_field(CAST(s), 'x')`, so it takes
+# `get_field`'s own single-leaf pushdown path: the read clips all the way down


I am not sure what 'get_fields own single-leaf pushdown path is' (It seems like a bunch of implementation detail -- can we just clarify that this query should read fewer bytes because it is selecting a field of s (not all the fields) ?

alamb · 2026-08-06T20:57:24Z

+##########
+# Regression test for https://github.com/apache/datafusion/issues/24109
+#
+# When the declared table schema differs from the physical file schema, the


I think the old behavior is not very relevant after this PR -- maybe this could just focus on what this case covers-- namely that the declared table schema differs from the physical schema

alamb · 2026-08-06T20:58:31Z

+LOCATION 'test_files/scratch/parquet_filter_pushdown/struct_schema_cast.parquet';
+
+query II
+SELECT id, s['x'] FROM t_struct_no_schema_cast WHERE s['x'] = 200;


can we also run the query too:

SELECT id, s['x'] FROM t_struct_schema_cast WHERE s['x'] > 100 AND id > 2;

alamb · 2026-08-06T20:59:12Z

+LOCATION 'test_files/scratch/parquet_filter_pushdown/struct_schema_cast.parquet';
+
+query II
+SELECT id, s['x'] FROM t_struct_missing_field WHERE s['missing'] = 200;


how about also running the two queries above and verifying that they still get the right answer even when there are new fields inserted?

alamb · 2026-08-06T21:00:31Z

+        (logical, physical)
+    }
+
+    /// `s['x']` where the file stores `x` as `Int32` and the table declares


do these unit tests add any additional coverage compared with the .slt coverage? I think the slt coverage is adequate and we could remove these tests and make the PR much smaller

alamb · 2026-08-06T21:01:28Z

+            vec![Field::new("x", DataType::Int64, true)],
+        );
+
+        let adapter = DefaultPhysicalExprAdapterFactory


there is a lot of boiler plate here (factor creation rewrite, cast, etc) -- maybe it could be factored into a helper so it is clearer what is being tested and what is setup

alamb · 2026-08-06T21:03:03Z

    }

+    /// Rewrite `get_field(cast(s AS Struct<..>), 'f')` into
+    /// `cast(get_field(s, 'f') AS <type of f>)`.


Can we also describe here the rationale for why we would want to do this rewrite? It is not obvious I think from these commens

alamb · 2026-08-06T21:04:00Z

+    /// by [`Self::rewrite_column`] whenever the logical and physical struct
+    /// types differ. Casting the whole struct just to read one field is
+    /// wasteful, and — more importantly — it hides the underlying column from
+    /// consumers that pattern match on `get_field(column, 'f')`. The Parquet


the silently dropping the predicate part I think is an implementation detail that may not be relevant in the future.

alamb · 2026-08-06T21:05:33Z

        Ok(Transformed::no(expr))
    }

+    /// Rewrite `get_field(cast(s AS Struct<..>), 'f')` into


did you consider teaching the pushdown side (PushdownChecker / row-filter builder) to see through the cast — i.e. recognize get_field(cast(col), 'f') — instead of narrowing it here?

I agree that this fix seems very specific -- it almost seems like it is focusing on the symptom rather than the underlying problem (the physical / logical mismatch that @zhuqi-lucas is pointing out)

It seems like if this is a rewrite that should be done, shouldn't we be doing at a higher level 🤔

Addresses review feedback on apache#24125: - Say *why* the cast is narrowed (reading one field should not cost a whole struct; keeping the column visible keeps every `get_field(column, 'f')` consumer working) and why it is fixed in the adapter rather than by teaching one consumer to see through casts. - Drop the description of the pre-fix row-filter behaviour, which is an implementation detail that will date. - Note that the empty-path arm of `resolve_field_path` is a defensive default, not a claim about empty paths. - Run the compound filter against the matching-schema table too, and check that declaring a field the file lacks leaves the fields it does have answering correctly. - Trim the sqllogictest comments to what the queries demonstrate rather than how the read plan gets there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MebN5PsVnYvXUeVKju5K7P

github-actions Bot added sqllogictest SQL Logic Tests (.slt) datasource Changes to the datasource crate labels Aug 5, 2026

adriangb changed the title ~~Claude/datafusion 24109 xkxvqy~~ fix: apply struct field filters when the file schema needs adaptation Aug 5, 2026

adriangb commented Aug 5, 2026

View reviewed changes

github-actions Bot removed the datasource Changes to the datasource crate label Aug 5, 2026

adriangb requested a lite review from Copilot August 5, 2026 20:28

Copilot started reviewing on behalf of adriangb August 5, 2026 20:28 View session

Copilot AI reviewed Aug 5, 2026

View reviewed changes

claude and others added 5 commits August 6, 2026 08:52

adriangb force-pushed the claude/datafusion-24109-xkxvqy branch from 7ea589d to f72ff6b Compare August 6, 2026 13:56

github-actions Bot added the datasource Changes to the datasource crate label Aug 6, 2026

zhuqi-lucas reviewed Aug 6, 2026

View reviewed changes

alamb reviewed Aug 6, 2026

View reviewed changes

Uh oh!

Conversation

adriangb commented Aug 5, 2026

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Not addressed here

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

codecov-commenter commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-commenter commented Aug 5, 2026 •

edited

Loading