Skip to content

fix(proto): preserve physical cast expression state - #25056

Open
buraksenn wants to merge 2 commits into
apache:mainfrom
buraksenn:24615-cast-proto-destructure
Open

fix(proto): preserve physical cast expression state#25056
buraksenn wants to merge 2 commits into
apache:mainfrom
buraksenn:24615-cast-proto-destructure

Conversation

@buraksenn

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Physical cast protobuf hooks did not serialize CastExpr options or explicit target-field metadata. A round trip could therefore change cast behavior, nullability, or metadata. The hooks also accessed fields individually, so future fields could be omitted silently.

What changes are included in this PR?

  • Exhaustively destructure CastExpr, TryCastExpr, and their protobuf messages.
  • Preserve explicit target fields for CAST and TRY_CAST while keeping legacy type-only payload behavior.
  • Preserve all serializable Arrow cast/format options; reject custom formatter factories instead of silently dropping them.
  • Store decoded format strings in owned cast options.
  • Regenerate protobuf models and document the CastExpr::cast_options API adjustment.

The protobuf changes are additive and backward compatible.

What is the testing strategy for this PR?

Added unit coverage for:

  • target-field and cast-option round trips
  • legacy payloads without the new fields
  • explicit default-shaped targets
  • malformed target-field types
  • unsupported formatter factories

Validated with:

  • cargo test -p datafusion-physical-expr --features proto expressions::cast::
  • cargo test -p datafusion-physical-expr --features proto expressions::try_cast::
  • cargo clippy -p datafusion-physical-expr -p datafusion-physical-expr-adapter --all-features --tests -- -D warnings
  • cargo check -p datafusion-proto --all-features
  • ./ci/scripts/doc_prettier_check.sh --write --allow-dirty

Are there any user-facing changes?

Cast expressions now retain their options and explicit output-field semantics across protobuf round trips. CastExpr::cast_options() returns a borrowing CastOptions<'_> value rather than &CastOptions<'static>; the upgrade guide documents migration for code rebuilding casts.

@github-actions github-actions Bot added documentation Improvements or additions to documentation logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates common Related to common crate labels Sep 8, 2026
@buraksenn
buraksenn marked this pull request as draft September 8, 2026 09:10
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

Thank you for opening this pull request!

Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).

Details
     Cloning apache/main
    Building datafusion-common v55.0.0 (current)
       Built [  29.299s] (current)
     Parsing datafusion-common v55.0.0 (current)
      Parsed [   0.051s] (current)
    Building datafusion-common v55.0.0 (baseline)
       Built [  27.323s] (baseline)
     Parsing datafusion-common v55.0.0 (baseline)
      Parsed [   0.052s] (baseline)
    Checking datafusion-common v55.0.0 -> v55.0.0 (no change; assume patch)
     Checked [   0.776s] 223 checks: 223 pass, 31 skip
     Summary no semver update required
    Finished [  59.644s] datafusion-common
    Building datafusion-expr-common v55.0.0 (current)
       Built [  15.800s] (current)
     Parsing datafusion-expr-common v55.0.0 (current)
      Parsed [   0.016s] (current)
    Building datafusion-expr-common v55.0.0 (baseline)
       Built [  15.712s] (baseline)
     Parsing datafusion-expr-common v55.0.0 (baseline)
      Parsed [   0.017s] (baseline)
    Checking datafusion-expr-common v55.0.0 -> v55.0.0 (no change; assume patch)
     Checked [   0.240s] 223 checks: 223 pass, 31 skip
     Summary no semver update required
    Finished [  32.660s] datafusion-expr-common
    Building datafusion-physical-expr v55.0.0 (current)
       Built [  23.514s] (current)
     Parsing datafusion-physical-expr v55.0.0 (current)
      Parsed [   0.041s] (current)
    Building datafusion-physical-expr v55.0.0 (baseline)
       Built [  23.444s] (baseline)
     Parsing datafusion-physical-expr v55.0.0 (baseline)
      Parsed [   0.041s] (baseline)
    Checking datafusion-physical-expr v55.0.0 -> v55.0.0 (no change; assume patch)
     Checked [   0.398s] 223 checks: 223 pass, 31 skip
     Summary no semver update required
    Finished [  48.356s] datafusion-physical-expr
    Building datafusion-physical-expr-adapter v55.0.0 (current)
       Built [  25.721s] (current)
     Parsing datafusion-physical-expr-adapter v55.0.0 (current)
      Parsed [   0.008s] (current)
    Building datafusion-physical-expr-adapter v55.0.0 (baseline)
       Built [  25.940s] (baseline)
     Parsing datafusion-physical-expr-adapter v55.0.0 (baseline)
      Parsed [   0.008s] (baseline)
    Checking datafusion-physical-expr-adapter v55.0.0 -> v55.0.0 (no change; assume patch)
     Checked [   0.087s] 223 checks: 223 pass, 31 skip
     Summary no semver update required
    Finished [  52.640s] datafusion-physical-expr-adapter
    Building datafusion-proto-models v55.0.0 (current)
       Built [  19.889s] (current)
     Parsing datafusion-proto-models v55.0.0 (current)
      Parsed [   0.106s] (current)
    Building datafusion-proto-models v55.0.0 (baseline)
       Built [  19.832s] (baseline)
     Parsing datafusion-proto-models v55.0.0 (baseline)
      Parsed [   0.109s] (baseline)
    Checking datafusion-proto-models v55.0.0 -> v55.0.0 (no change; assume patch)
     Checked [   1.902s] 223 checks: 222 pass, 1 fail, 0 warn, 31 skip

--- failure constructible_struct_adds_field: struct exhaustively constructible through public API adds field ---

Description:
A pub struct that could be exhaustively constructed with a literal using only public API has a new pub field, breaking existing exhaustive literals.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/constructible_struct_adds_field.ron

Failed in:
  field PhysicalCastNode.target_field in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1863
  field PhysicalCastNode.cast_options in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1866
  field PhysicalCastNode.target_field in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1863
  field PhysicalCastNode.cast_options in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1866
  field PhysicalTryCastNode.target_field in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1852
  field PhysicalTryCastNode.target_field in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1852

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  42.850s] datafusion-proto-models

@github-actions github-actions Bot added the auto detected api change Auto detected API change label Sep 8, 2026
@codecov-commenter

codecov-commenter commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 54.91184% with 358 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.69%. Comparing base (92746a9) to head (7f66f53).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/proto-models/src/generated/pbjson.rs 0.00% 315 Missing ⚠️
datafusion/physical-expr/src/expressions/cast.rs 92.15% 20 Missing and 7 partials ⚠️
datafusion/proto-models/src/generated/prost.rs 0.00% 11 Missing ⚠️
...tafusion/physical-expr/src/expressions/try_cast.rs 95.76% 2 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25056      +/-   ##
==========================================
- Coverage   81.74%   81.69%   -0.06%     
==========================================
  Files        1128     1128              
  Lines      416645   417391     +746     
  Branches   416645   417391     +746     
==========================================
+ Hits       340580   340974     +394     
- Misses      55998    56349     +351     
- Partials    20067    20068       +1     

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

@buraksenn
buraksenn marked this pull request as ready for review September 8, 2026 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto detected api change Auto detected API change common Related to common crate documentation Improvements or additions to documentation logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Destructure proto hooks for cast physical expressions

2 participants