Skip to content

[branch-55] Align metadata propagation through Physical and Logical casts (#23169) - #24875

Merged
timsaucer merged 1 commit into
apache:branch-55from
timsaucer:fix/backport-23169
Sep 2, 2026
Merged

[branch-55] Align metadata propagation through Physical and Logical casts (#23169)#24875
timsaucer merged 1 commit into
apache:branch-55from
timsaucer:fix/backport-23169

Conversation

@timsaucer

@timsaucertimsaucer commented Sep 2, 2026

Copy link
Copy Markdown
Member

This PR is a backport of #23169 onto branch-55.

I made one update the use statement in planner.rs.

@github-actionsgithub-actionsBot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) proto Related to proto crate functions Changes to functions implementation labels Sep 2, 2026
@codecov-commenter

codecov-commenter commented Sep 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.56627% with 49 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.20%. Comparing base (4d524a1) to head (a75e52f).

Files with missing linesPatch %Lines
...tafusion/physical-expr/src/expressions/try_cast.rs87.87%13 Missing and 11 partials ⚠️
datafusion/functions/src/core/arrow_try_cast.rs0.00%11 Missing ⚠️
datafusion/physical-expr/src/expressions/cast.rs95.21%1 Missing and 8 partials ⚠️
datafusion/physical-expr/src/planner.rs95.12%0 Missing and 4 partials ⚠️
datafusion/functions/src/core/arrow_cast.rs90.90%0 Missing and 1 partial ⚠️
Additional details and impacted files
@@ Coverage Diff @@## branch-55 #24875 +/- ##
=============================================
+ Coverage 81.19% 81.20% +0.01% 
=============================================
Files 1110 1110 Lines 387773 388267 +494 Branches 387773 388267 +494 =============================================
+ Hits 314852 315304 +452 - Misses 54404 54426 +22 - Partials 18517 18537 +20 

☔ 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.

@timsaucer
timsaucer marked this pull request as ready for review September 2, 2026 13:41

@adriangbadriangb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor updates in cargo lock file due to a yanked dependency and security vulernability.

Should this / can this be it's own commit? Not sure if #23169 introduced the yanked dep, or if it's coming from main or the 55 branch?

@timsaucer

Copy link
Copy Markdown
MemberAuthor

Minor updates in cargo lock file due to a yanked dependency and security vulernability.

Should this / can this be it's own commit? Not sure if #23169 introduced the yanked dep, or if it's coming from main or the 55 branch?

I definitely can make it it's own commit. It's a very minor change, but if you want I'll remove it and add another PR. That one would need to land first to avoid breaking CI on this one.

@adriangb

Copy link
Copy Markdown
Contributor

If you don't mind that'd be great to land as it's own prequel commit. I'll aprove that PR, hopefuly it's not much more work.

…#23169)
- Closesapache#22079
- Closesapache#24724
The logical `Expr::Cast` and `Expr::TryCast` have a `FieldRef` target
that was added in apache#18136 so
that logical casts can express a cast to an extension type. In
combination with a SQL type planner (
apache#20676 ) and an optimizer rule,
this enabled casting to/from extension types with custom semantics to
actually occur. The ability to do this was reverted by
apache#20836 (which removed the
original test) and I am not sure that ability ever made it into a
release. When investigating this issue, it became clear the logical and
physical cast behaviour had diverged with respect to the target field.
This PR strips specific metadata keys (extension name and extension
metadata) when propagating metadata from the source of a cast to the
target (because doing so may result in an invalid destination field that
consumers could reject), and propagates all metadata from the (logical)
cast target field (e.g., so that a cast to an extension type represented
by the cast target field will have a `to_field()` that communicates the
extension type).
For the physical cast, this behaviour is replicated exactly (I hope).
Note that actually casting to an extension type can be implemented with
an optimizer rule, planner, or by the mechanism I have in the works in
apache#21071 .
Yes
It was in practice not common to create a `Expr::Cast` with field
metadata internally and thus I don't think users will see metadata
changes from the inclusion of metadata from the target field. I would be
surprised if stripping the extension name/metadata from the source was
disruptive (it was more likely to have caused errors).
Superceeds an earlier but similar attempt (
apache#22162 ).
---------
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Co-authored-by: Tim Saucer <timsaucer@gmail.com>
(cherry picked from commit 124291e)
@timsaucer

Copy link
Copy Markdown
MemberAuthor

Rebased to only have the single commit from main. I'll merge after CI is green.

@timsaucer
timsaucer merged commit cf1ccae into apache:branch-55Sep 2, 2026
39 checks passed
@timsaucer
timsaucer deleted the fix/backport-23169 branch September 2, 2026 18:41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

functionsChanges to functions implementationlogical-exprLogical plan and expressionsphysical-exprChanges to the physical-expr cratesprotoRelated to proto cratesqllogictestSQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@timsaucer@codecov-commenter@adriangb@paleolimbot