Skip to content

Keep cast metadata changes non-breaking - #2

Merged
paleolimbot merged 1 commit into
paleolimbot:cast-metadata-cleanupfrom
timsaucer:non-breaking-cast-metadata
Aug 31, 2026
Merged

Keep cast metadata changes non-breaking#2
paleolimbot merged 1 commit into
paleolimbot:cast-metadata-cleanupfrom
timsaucer:non-breaking-cast-metadata

Conversation

@timsaucer

Copy link
Copy Markdown

Which issue does this PR close?

Rationale for this change

We would like the logical/physical cast metadata alignment from apache#23169 in the
55.1.0 patch release, because it fixes the cast half of apache#24721. On branch-55
today, a CAST to an extension type produced by a TypePlanner gives the
logical plan a field with no metadata while the physical CastExpr produces one
with ARROW:extension:name, and ProjectionPushdown then fails with:

PhysicalOptimizer rule 'ProjectionPushdown' failed. Schema mismatch.
Internal error: Schema field unallowed change:
  old field: Field { name: "u", data_type: FixedSizeBinary(16), nullable: true },
  new field: Field { name: "u", data_type: FixedSizeBinary(16), nullable: true,
                     metadata: {"ARROW:extension:name": "arrow.uuid"} }.

The blocker for backporting is that apache#23169 changes two public signatures on
CastExpr. This PR removes that blocker without changing any of the semantics
you implemented.

What changes are included in this PR?

One commit on top of your branch, restoring the two signatures to what they are
on main:

  • CastExpr::new_with_target_field takes FieldRef by value again
  • CastExpr::target_field() returns &FieldRef again

Both fall out of storing the target FieldRef as before and adding a private
explicit_target: bool recording whether the field came from the caller or was
synthesized from a DataType. That flag carries exactly the information the
Option<HashMap<..>> / Option<bool> pair carried, so target_metadata(),
target_nullable(), has_explicit_metadata() and has_explicit_nullability()
keep their meaning and are derived from it. Hash/PartialEq still compare the
same components, so the field name still does not participate.

TryCastExpr is given the same shape for symmetry. Its metadata-aware API is
new on this branch, so nothing there is a compatibility constraint.

cast_with_target_field keeps its &FieldRef parameter — mod cast is private
and it is only re-exported as pub(crate), so it is not public API.

Cast semantics are untouched: explicit target fields still supply their metadata
and nullability verbatim, type-only casts still pass source metadata through with
the extension type keys stripped, and the output field name still comes from the
source expression. One test assertion comes back, since target_field() again
returns the field it was constructed with:

assert_eq!(target_field.name(), "file_row_index");

Comparing public signatures against the merge base with main, the only
remaining deltas are additions:

+ CastExpr::target_metadata / target_nullable
+ CastExpr::has_explicit_metadata / has_explicit_nullability
+ TryCastExpr::new_with_target_field / target_metadata / target_field
+ expressions::try_cast_with_target_field

Are these changes tested?

Yes — existing tests, plus one new unit test
(target_field_accessor_returns_the_constructed_field) covering the restored
accessor and the derived explicit/type-only reporting.

  • cargo clippy --all-targets --workspace --features avro,integration-tests,extended_tests -- -D warnings — clean
  • sqllogictest: 504/504 files pass
  • unit tests pass for datafusion-physical-expr (1634), datafusion-physical-expr-adapter (42), datafusion-physical-plan (1844), datafusion-pruning (95), datafusion-expr (256), datafusion-functions (336) and datafusion core lib (444)

Are there any user-facing changes?

No API changes relative to main beyond the additions listed above. The
behavioral changes described in apache#23169 are unaffected by this commit — worth
noting for a patch release that they still include the cast output field name
coming from the source rather than the target, extension keys being stripped on
type-only casts, arrow_cast/arrow_try_cast no longer eliding same-type casts
over extension-typed sources, and TRY_CAST to an extension type no longer
erroring.

Restore the two public signatures on `CastExpr` that this branch changed,
so the metadata fixes can be backported to a patch release:

* `CastExpr::new_with_target_field` takes `FieldRef` by value again
* `CastExpr::target_field()` returns `&FieldRef` again

Both are achieved by storing the target `FieldRef` as before and adding a
private `explicit_target` flag that records whether the field was supplied
by the caller or synthesized from a `DataType`. That flag carries exactly
the information the `Option<HashMap<..>>` / `Option<bool>` pair carried, so
`target_metadata()`, `target_nullable()`, `has_explicit_metadata()` and
`has_explicit_nullability()` keep their meaning and are derived from it.
`Hash`/`PartialEq` compare the same components as before, so the field name
still does not participate.

`TryCastExpr` is given the same shape for symmetry. Its metadata-aware API
is new on this branch, so nothing there is a compatibility constraint.

Cast semantics are unchanged: explicit target fields still supply their
metadata and nullability verbatim, type-only casts still pass source
metadata through with the extension type keys stripped, and the output
field name still comes from the source expression.

Comparing the public signatures against the merge base with main, the only
remaining deltas are additions:

    + CastExpr::target_metadata / target_nullable
    + CastExpr::has_explicit_metadata / has_explicit_nullability
    + TryCastExpr::new_with_target_field / target_metadata / target_field
    + expressions::try_cast_with_target_field

`cast_with_target_field` keeps its `&FieldRef` parameter: `mod cast` is
private and it is only re-exported as `pub(crate)`, so it is not part of
the public API.

Verified: `cargo clippy --workspace --all-targets -- -D warnings` clean,
504/504 sqllogictest files pass, and the unit tests for physical-expr,
physical-expr-adapter, physical-plan, pruning, expr, functions and the
datafusion core lib all pass.
@timsaucer

Copy link
Copy Markdown
Author

To see the diff against main: apache#24828

@paleolimbot
paleolimbot merged commit 007b8ef into paleolimbot:cast-metadata-cleanup Aug 31, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants