Skip to content

Remove CastColumnExpr and custom_file_casts example; unify on field-aware CastExpr#21563

Open
kosiew wants to merge 8 commits intoapache:mainfrom
kosiew:cast-05-20164
Open

Remove CastColumnExpr and custom_file_casts example; unify on field-aware CastExpr#21563
kosiew wants to merge 8 commits intoapache:mainfrom
kosiew:cast-05-20164

Conversation

@kosiew
Copy link
Copy Markdown
Contributor

@kosiew kosiew commented Apr 12, 2026 •
edited
Loading

Which issue does this PR close?

Rationale for this change

This PR completes the cast unification effort by removing the remaining public and example surface of CastColumnExpr. Keeping both CastExpr and CastColumnExpr introduced duplication in casting logic, increased maintenance overhead, and made it easier for downstream code to reintroduce divergent behavior.

With the introduction of field-aware CastExpr (via new_with_target_field), all functionality previously covered by CastColumnExpr can now be expressed through a single, consistent abstraction. This simplifies the mental model for casting, ensures consistent behavior across execution paths, and aligns documentation and examples with the unified design.

Additionally, this change ensures that test coverage and examples reflect the final architecture, reducing confusion for contributors and users.

What changes are included in this PR?

  • Removal of CastColumnExpr

    • Deleted datafusion/physical-expr/src/expressions/cast_column.rs.
    • Removed module declaration and public re-export from expressions/mod.rs.

  • Migration to field-aware CastExpr

    • Updated code paths to rely exclusively on CastExpr.
    • Replaced usages of data_type(&schema) with target_field().data_type() where appropriate.

  • Example updates

    • Updated custom_file_casts example to only handle CastExpr.
    • Removed branching logic that handled both CastExpr and CastColumnExpr.
    • Updated CLI usage string to reflect current examples.

  • Test consolidation and improvements

    • Migrated struct, nested struct, and scalar struct cast tests into cast.rs.

    • Added comprehensive field-aware cast tests covering:

      • Target field metadata preservation
      • Nullability semantics
      • Struct casting with missing fields
      • Nested struct casting
      • Struct scalar casting

  • Refactoring and cleanup

    • Renamed helper functions for clarity (e.g., assert_cast_column → assert_cast_input_column).
    • Simplified adapter logic to operate on a single cast expression type.

  • Documentation updates

    • Added upgrade note documenting removal of CastColumnExpr and migration guidance.

Are these changes tested?

Yes.

  • Existing tests were updated to reflect the unified cast model.

  • Coverage from cast_column.rs was preserved and migrated into cast.rs.

  • New tests validate:

    • Field-aware CastExpr semantics
    • Struct and nested struct casting behavior
    • Scalar struct casting
    • Metadata and nullability propagation

  • Adapter and schema rewriting tests were updated and remain green.

Are there any user-facing changes?

Yes.

  • Breaking change: CastColumnExpr has been removed from the public API.

  • Users must migrate to CastExpr:

    • Use CastExpr::new_with_target_field(...) for field-aware casting.
    • Access output metadata via target_field().
    • Access input expressions via expr().

  • Documentation has been updated with migration guidance.

LLM-generated code disclosure

This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed and tested.

kosiew added 6 commits April 12, 2026 13:29
@kosiew
Refactor casting and update documentation
fac4934 
Remove active CastColumnExpr from mod.rs and delete
cast_column.rs. Migrate important CastColumnExpr regression
coverage to field-aware CastExpr tests in cast.rs. Eliminate
custom_file_casts wiring and documentation from main.rs and
README.md, along with custom_file_casts.rs. Add SQL struct
cast coverage in struct.slt and provide migration guidance
in 54.0.0.md. Clean up adapter test helper name in
schema_rewriter.rs.
@kosiew
Remove redundant cast tests for primitives and structs
d414820 
Eliminate field_aware_cast_primitive_array and
field_aware_cast_nested_struct_array as their behaviors are
adequately tested by existing generic cast tests and SQL-level
struct cast validations. This streamlines the test suite and
reduces redundancy.
@kosiew
Refactor duplicate checks in struct handling
1793eee 
Removed redundant checks for explicit scalar struct
field reordering, missing nullable target fields, and
nested explicit struct cast reordering. These were
previously addressed by the explicit struct cast
and field-reordering sections. Retained the compact
array explicit-cast case for valuable end-to-end
coverage of unifying partially overlapping struct
schemas.
@kosiew
Add cast nested struct array coverage and updates
0089b23 
Introduce field_aware_cast_nested_struct_array coverage
in cast.rs to handle recursive nested struct casting
and null-filled missing nested fields. Update the
upgrade guide in 54.0.0.md to recommend using the
public CastExpr::new_with_target_field(...) API.
Also, add adapter_serialization to the custom_data_source
usage string in main.rs for improved clarity.
@kosiew
Refactor cast.rs to consolidate field-aware tests
27fb120 
Combine three field-aware cast tests into a single table-style test
to enhance readability and maintainability. The new test,
field_aware_cast_preserves_target_field_semantics, covers all
necessary aspects including target field name, data type, metadata,
target nullability, and effective expression nullability for both
child-nullable and target-nullable cases. Retain certain tests
separately to ensure distinct CastColumnExpr migration coverage.
@kosiew
Refactor cast.rs and update migration wording
1d2da60 
Add private test helper for struct cast evaluation,
reuse make_struct_array, switch Boolean downcast to
as_boolean_array, and inline one-use field setup.
Tighten wording in the CastColumnExpr migration.
@github-actions github-actions bot added documentation Improvements or additions to documentation physical-expr Changes to the physical-expr crates labels Apr 12, 2026
@kosiew kosiew marked this pull request as ready for review April 12, 2026 05:56
@kosiew kosiew requested a review from adriangb April 12, 2026 05:56
adriangb
adriangb approved these changes Apr 12, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@adriangb adriangb left a comment

Choose a reason for hiding this comment

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

I think we should keep the example, it seems like it would be a 1 line change to make it work with CastExpr.

datafusion-examples/examples/custom_data_source/custom_file_casts.rs
Comment on lines -45 to -46
// Example showing how to implement custom casting rules to adapt file schemas.
// This example enforces that casts must be strictly widening: if the file type is Int64 and the table type is Int32, it will error
Copy link
Copy Markdown
Contributor

@adriangb adriangb Apr 12, 2026

Choose a reason for hiding this comment

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

Is this example not valuable anymore? Can't we just change the expression used?

Copy link
Copy Markdown
Contributor Author

@kosiew kosiew Apr 12, 2026

Choose a reason for hiding this comment

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

Yes, that is a fair point. The intent of deleting it was to avoid preserving an example that taught downstream users to branch on both CastExpr and the now-removed CastColumnExpr. But the example itself is still useful because it demonstrates a custom PhysicalExprAdapter policy for file-schema casts.

I restored the example and updated it to use the unified cast representation.

kosiew added 2 commits April 12, 2026 21:46
@kosiew
feat: add custom casting rules example for file schemas
9587752 
- Restored example `custom_file_casts` that demonstrates how to implement custom casting rules in DataFusion.
- Updated the README to include the  example.
- Modified `main.rs` to include the `custom_file_casts` module in the execution options.
@kosiew
fix: update function call for checking cast compatibility in CustomCa…
76c3cf7 
…stsPhysicalExprAdapter

This commit modifies the method used to check for valid casts within the `CustomCastsPhysicalExprAdapter` implementation. It replaces `CastExpr::check_bigger_cast` with `cast.is_bigger_cast` for improved clarity and maintainability.
@adriangb adriangb added this pull request to the merge queue Apr 12, 2026
@adriangb adriangb removed this pull request from the merge queue due to a manual request Apr 12, 2026
@adriangb
Copy link
Copy Markdown
Contributor

adriangb commented Apr 12, 2026

@kosiew one thought that occurs to me - is there some sense in leaving the symbol around for another release marked as deprecated?

Thinking through it there's basically two uses of this:

  1. Constructing it (adding a cast, etc.). This use we need to get rid of immediately: we no longer handle this expression in various code paths. So arguably we should immediately remove constructors, etc otherwise users will end up with confusing cast errors in weird places.
  2. Matching on it (like we used to do in various code paths). We could in theory leave the struct around and it would be, in theory, less churn for users. But given that the solution is either "delete the block of code" or "replace with CastExpr" it's really not much more work to just fix the issue than it is to add an allow for the deprecation lint.

So with that said I think it makes sense to just remove this completely as this PR does.
It's helped by the fact that it has only been around for 1 or 2 releases.

But I do want to maybe get an okay from @comphead before we merge knowing that they've dealt with a lot of issues w.r.t. PhysicalExprAdapter and struct casting in particular w/ 52.0.0.

@comphead
Copy link
Copy Markdown
Contributor

comphead commented Apr 12, 2026

Thanks @adriangb for pinging me, in Comet we would need to adapt to the changes, if I understood the changes we would need to use CastExpr instead of CastColumnExpr ?

@adriangb
Copy link
Copy Markdown
Contributor

adriangb commented Apr 12, 2026 •
edited
Loading

Thanks @adriangb for pinging me, in Comet we would need to adapt to the changes, if I understood the changes we would need to use CastExpr instead of CastColumnExpr ?

Yes I just checked and it seems like you’re actually using neither. Searching the codebase for CastColumnExpr all I found was your custom implementation: https://github.com/apache/datafusion-comet/blob/88c1ffcd083eb05bcbdcc85090e53b3398850946/native/core/src/parquet/cast_column.rs#L181. Now that CastExpr is more capable maybe you can just use that?

You do use CastExpr but that shouldn’t change: https://github.com/apache/datafusion-comet/blob/88c1ffcd083eb05bcbdcc85090e53b3398850946/native/core/src/execution/planner.rs#L1991

So in summary I think this change shouldn’t be a problem for you.

@kosiew kosiew added the api change Changes the API exposed to users of the crate label Apr 13, 2026
@kosiew
Copy link
Copy Markdown
Contributor Author

kosiew commented Apr 13, 2026

Added the api change label for higher visibility of the change impact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adriangb adriangb adriangb approved these changes

Assignees

No one assigned

Labels

api change Changes the API exposed to users of the crate documentation Improvements or additions to documentation physical-expr Changes to the physical-expr crates

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Extend CastExpr and Converge on Single Cast Expression

3 participants

@kosiew @adriangb @comphead