fix: preserve explicit Struct casts and evolved field semantics - #24680
Open
sunchao wants to merge 4 commits into
Open
fix: preserve explicit Struct casts and evolved field semantics#24680sunchao wants to merge 4 commits into
sunchao wants to merge 4 commits into
Conversation
github-actions
Bot
added
core
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #24680 +/- ##
==========================================
+ Coverage 81.44% 81.46% +0.01%
==========================================
Files 1118 1119 +1
Lines 399602 400981 +1379
Branches 399602 400981 +1379
==========================================
+ Hits 325460 326659 +1199
- Misses 55146 55200 +54
- Partials 18996 19122 +126 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
sunchao
marked this pull request as ready for review
August 26, 2026 06:10
Keep evolved decimal conversions inside their Struct ancestors while restricting the cast target to the requested field path. This preserves all-null shortcuts without evaluating unselected sibling conversions. Index generated casts by pointer while retaining their Arc allocations, avoiding repeated linear identity scans for large expressions. Cover flat and nested decimal access and selective Parquet filters with pushdown enabled and disabled.
viirya
approved these changes
Aug 26, 2026
viirya
left a comment
viirya
left a comment
Member
There was a problem hiding this comment.
The problem statement here is unusually clear — three distinct failure modes, each with a concrete reproducer, plus an explicit list of what's out of scope. Having the tests labelled by whether they reproduce the bug or merely act as a control on the base is genuinely useful; that distinction usually has to be reverse-engineered by the reviewer.
I walked all three fixes and they hold up:
- Gating narrowing on "did the adapter generate this cast" is the right axis. A schema-adaptation cast and a user-written cast are structurally identical, so provenance is the only thing that can separate them, and the obligation genuinely differs.
- Comparing the full
return_fieldrather than the leaf fields, and usinglogical_return_fieldas the cast target, fixes inherited nullability and picks up metadata at the same time. - Keeping the conversion inside its Struct ancestors for decimal changes, while
retain_field_pathtrims the target to just the selected path, is the neatest part of this. It preserves Arrow's all-null shortcut without re-implementing Arrow's conversion-validation rules in the rewriter — which would have been a maintenance trap.
I also checked the allow_struct_casts asymmetry rather than assuming it: planning passes false so an explicit cast leaves a residual filter, while the post-adaptation runtime path passes true so an already-delegated predicate stays evaluable. The test asserting both directions is the right way to pin that down.
Approving. My only real feedback is about how much of the pointer-identity scheme's safety argument is written down — details inline, neither blocking.
| physical_file_schema: SchemaRef, | ||
| // Retain generated casts so their pointer identity remains reliable even | ||
| // after a wider cast has been removed from the expression tree. | ||
| generated_struct_casts: HashMap<*const (), Arc<dyn PhysicalExpr>>, |
Member
There was a problem hiding this comment.
This map's soundness rests on three things, and only the second is currently stated:
rewrite()usesexpr.transform(...), which is bottom-up, so aColumnbecomes a generated cast before the parentget_fieldis visited — that is what makes the childArcthe parent sees the same allocation that was recorded.- The map holds an owned
Arcclone, so the allocation cannot be freed and its address recycled (the part the current comment covers). - The rewriter is constructed per
rewrite()call, so keys never leak across expressions.
Point 1 is the fragile one: switching to transform_down, or hoisting the rewriter to be reused across expressions, would silently stop narrowing. The failure is fail-closed — it would under-optimise rather than mis-narrow — so this isn't a correctness worry, but it is a silent performance regression that no test would catch. Could the comment name the traversal-order dependency explicitly?
Separately, and only if it isn't invasive: was a structural marker considered instead of pointer identity — threading "this cast was generated" out through the rewrite result, or a thin wrapper type around generated casts? That would remove this whole class of concern rather than documenting around it. If you tried it and it spread too far through the rewriter, saying so in the comment would save the next person from re-litigating it.
Member
Author
There was a problem hiding this comment.
Thanks, clarified in 801bb0e. The comment now documents bottom-up transform traversal, a fresh tracker per rewrite(), and retained Arc ownership. It also explains why provenance stays local instead of adding markers to expression types or threading it through rewrite results. I kept the existing implementation; this follow-up only changes comments.
| } | ||
|
|
||
| /// Retain a field path without changing its ancestors' metadata or nullability. | ||
| fn retain_field_path(field: &FieldRef, path: &[&str]) -> Option<FieldRef> { |
Member
There was a problem hiding this comment.
retain_field_path and the existing resolve_field_path sit next to each other and read as near-synonyms, but they do different jobs: one resolves a key path to a leaf for inspection, the other rebuilds a trimmed cast target that keeps the ancestors intact. The doc comment explains the what ("retain a field path without changing its ancestors' metadata or nullability"); a clause on why it exists — to exclude unselected siblings from a conversion while keeping the Struct ancestors for Arrow's all-null shortcut — would connect it to the decimal case that motivates it. A name closer to that purpose (trim_cast_target_to_path, say) would also help, though renaming is entirely your call.
Member
Author
There was a problem hiding this comment.
Updated in 801bb0e. The doc comment now explains that the helper trims the cast target to exclude unselected sibling conversions while retaining the Struct ancestors, their metadata and nullability, and the all-null shortcut. I kept the helper name unchanged to keep this follow-up limited to documentation.
Contributor
|
Thank you @sunchao . I plan to review this later today. |
Document the traversal order, per-rewrite lifetime, and retained Arc ownership required by generated-cast tracking, including why provenance stays local. Explain how retaining only the selected cast-target path avoids sibling conversions while preserving the all-null Struct shortcut.
Contributor
|
I'll check it this afternoon |
Contributor
|
run benchmarks tpch tpcds |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing dev/chao/codex/oss-struct-cast-semantics (801bb0e) to 6e66a85 (merge-base) diff Run configurationrun benchmark tpchResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing dev/chao/codex/oss-struct-cast-semantics (801bb0e) to 6e66a85 (merge-base) diff Run configurationrun benchmark tpcdsResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing dev/chao/codex/oss-struct-cast-semantics (801bb0e) to 6e66a85 (merge-base) diff Run configurationrun benchmark tpchCPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing dev/chao/codex/oss-struct-cast-semantics (801bb0e) to 6e66a85 (merge-base) diff Run configurationrun benchmark tpcdsCPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
comphead
approved these changes
Aug 26, 2026
comphead
left a comment
comphead
left a comment
Contributor
There was a problem hiding this comment.
Thanks @sunchao/ One real finding (P2): the decimal all-null fix uses is_decimal() on the top-level leaf type, so it misses decimals nested in List/FixedSizeList/ListView/Map/Dictionary.
Suggested fix direction
Replace the top-level is_decimal() check with a recursive "does this leaf type contain a decimal anywhere" test, so container-wrapped decimals also take the struct-cast (retain_field_path) shape and keep the
all-null shortcut, while non-decimal leaves keep the pushdown-friendly scalar-cast shape.
comphead
reviewed
Aug 26, 2026
| cast_accesses: Vec<CastColumnAccess>, | ||
| /// Whether to collect [`Self::cast_accesses`]. | ||
| collect_cast_accesses: bool, | ||
| /// Allow field access through a retained Struct cast after schema adaptation. |
Contributor
There was a problem hiding this comment.
perhaps its not very obvious the param allow scope? is it cast to struct, from struct, both?
Member
Author
There was a problem hiding this comment.
Clarified in e79e91236: both the source and target must be Struct types. This flag allows get_field(CAST(struct_column AS Struct(...)), 'field', ...) after schema adaptation; the reader preserves the cast and reads the full source Struct. Planning keeps the flag disabled so explicit casts retain a residual filter.
comphead
reviewed
Aug 26, 2026
| let return_type = func.return_type(); | ||
| if DataType::is_nested(return_type) && !self.is_nested_type_supported(return_type) | ||
| { | ||
| return None; |
Contributor
There was a problem hiding this comment.
kind of optional, but when we have multiple early returns maybe its good to debug the reason of None?
Member
Author
There was a problem hiding this comment.
Thanks, reason logging could help debugging. I'm deferring it to keep this correctness fix focused. Here, None means the retained-Struct-cast special case did not apply; the caller continues with its normal checks and traversal. It does not itself reject pushdown or indicate an execution error.
comphead
reviewed
Aug 26, 2026
Comment on lines
+324
to
+329
| let Some((name, rest)) = path.split_first() else { | ||
| return Some(Arc::clone(field)); | ||
| }; | ||
| let DataType::Struct(fields) = field.data_type() else { | ||
| return None; | ||
| }; |
Contributor
There was a problem hiding this comment.
Suggested change
| let Some((name, rest)) = path.split_first() else { | |
| return Some(Arc::clone(field)); | |
| }; | |
| let DataType::Struct(fields) = field.data_type() else { | |
| return None; | |
| }; | |
| let DataType::Struct(fields) = field.data_type() else { | |
| return None; | |
| }; | |
| let Some((name, rest)) = path.split_first() else { | |
| return Some(Arc::clone(field)); | |
| }; |
maybe we can swap early returns?
Member
Author
There was a problem hiding this comment.
I kept the empty-path check first because it is the recursion's success case, including scalar leaves. Swapping the checks returns None at a decimal leaf, so narrowing is abandoned and the whole Struct cast, including unselected siblings, remains.
I tried the suggested swap: test_narrow_decimal_struct_cast_ignores_siblings failed while converting the unused y = "bad" to Int32. The current order preserves the selected leaf and lets the caller trim the unselected siblings.
Member
Author
|
Thanks @comphead, confirmed and fixed in e79e91236. The guard now looks through the selected field's container value types, covering the reported List families, Map, and Dictionary cases while retaining only the selected field path. The existing regression now covers these containers and deeper nesting. I also added protection for matching-type narrowing, chained field access, and existing container-to-Struct conversions, which must keep their current casting path. The Parquet regression covers Validation passed: 42 adapter tests, 249 Parquet datasource tests, 227 Parquet integration tests, 8 doctests (5 existing ignores), and 21 SQL logic files. Formatting, Clippy across all targets/features with warnings denied, and the full repository lint suite also passed. This addresses the container-decimal finding. I updated the PR description with the container example and the validation results for this revision. |
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
6 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?
Closes #24679. Follow-up to #24125 and #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 justxinstead of every field ins.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:The explicit cast asks to convert both fields. It must fail because
'bad'cannot become an integer, even though the caller only usesx. Narrowing it toCAST(get_field(s, 'x') AS INT)instead returns1: the conversion ofy, 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:
DataFusion's whole-Struct conversion returns nulls without converting its children. If the rewrite extracts
xfirst, 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.xfromList<Utf8>toList<Decimal128(10, -1)>can fail even when every parent Struct is null. Checking only whetherxitself 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 NULLshould select every row in the batch, not fail while preparing a decimal conversion.There is a related schema-contract problem. A required child
xinside a nullable logical parentsstill gives a nullable result fors.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.
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 that cast and reads the full source Struct. Planning remains conservative about explicit casts, keeping a residual filter for them.
Are these changes tested?
Regression tests cover explicit cast errors, nullability inherited from parents, decimal conversions on entirely null Structs, unselected sibling conversion errors, and execution through Parquet filters. The adapter and retained-cast read-plan regressions fail without the relevant production fixes. The explicit-cast SQL integration test is a control that also passes on the base; it is not presented as another reproduction of the adapter bug.
The container follow-up extends the all-null regression across the List families, Maps, Dictionaries, and nested containers. It also checks matching-type optimization, chained field access, and successful container-to-Struct conversions. The Parquet regression now exercises both scalar and List decimal changes with filter pushdown enabled and disabled.
For the container follow-up in e79e91236, the targeted Rust and SQL tests and all-feature Clippy used the unchanged tracked
Cargo.lockwith--locked:struct cast parquetpassed.Earlier in this PR, the extended workspace suite (10,794 Rust tests, 8 ignored), all 505 SQL logic files, and 107 CLI tests also passed. Those broader runs predate the container follow-up; the results above are the checks repeated for this update.
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. Some evolved-decimal filters may read the full Struct to preserve correctness; this PR does not claim a general performance improvement.
There are no public API or dependency changes. General changes to container-to-Struct conversion semantics, generic
get_fieldbehavior under null parents, and masking of encoded arrays remain outside this PR's scope.AI assistance: Codex generated the implementation, regression tests, and PR text, and performed the stated local checks and source reviews. This does not claim a separate human review.