feat: Replace `DataType` with `FieldRef` in Cast by Weijun-H · Pull Request #18120 · apache/datafusion

Weijun-H · 2025-10-17T09:45:33Z

Which issue does this PR close?

Closes LogicalPlan Casts can't express a cast to an extension type #18060

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

paleolimbot · 2025-10-17T17:43:18Z

Apologies...I missed this PR! Feel free to lift anything useful from #18136 (I'm just as happy to review this instead).

paleolimbot · 2025-10-27T15:46:00Z

(I did clean up #18136 because I had some time on Friday but also feel free to keep going here and I'm happy to review!)

Weijun-H · 2025-10-27T16:45:24Z

(I did clean up #18136 because I had some time on Friday but also feel free to keep going here and I'm happy to review!)

Hi @paleolimbot , this pr is ready for review

paleolimbot

Nice! I have a few high-level concerns but am happy to iterate. Another option is to pursue #18136 and implement the nullability handling here (which I didn't implement) as a follow-on!

The DataTypeExt::into_nullable_field_ref() I think will help eliminate the need for the Arc::new(Field::new("", ...)) changes.
I think that the cast() and Cast::new() signatures should probably remain unchanged to minimize disruption in this PR and downstream
Considering the nullability of the cast field in the logical expression is problematic (highighted by the CI failures here) because all existing casts simply propagate the input expression nullability. I think this is what most people mean by a cast and so we should keep that behaviour. If we were to express a cast with explicit nullability we would have to do something like Option<bool> (where None means propagate the input, true means explicitly cast to nullable, and false means explicitly cast to non-nullable). All of those would require integration with the physical expression implementing the cast to ensure output schema consistency (which is the reason for the CI failure I believe)

paleolimbot · 2025-10-27T18:03:33Z

datafusion/expr/src/expr.rs

-            Expr::Cast(Cast { expr, data_type }) => {
-                write!(f, "CAST({expr} AS {data_type})")
+            Expr::Cast(Cast { expr, field }) => {
+                write!(f, "CAST({expr} AS {})", field.data_type())


There's now a format_type_and_metadata() helper in datafusion_common that will ensure that casts with metadata are displayed. That display is a bit verbose at the moment when metadata exists but it makes it quite a bit easier to track down bugs where the metadata was dropped.

paleolimbot · 2025-10-27T18:08:28Z

datafusion/expr/src/expr.rs

        let expr = Expr::Cast(Cast {
            expr: Box::new(Expr::Literal(ScalarValue::Float32(Some(1.23)), None)),
-            data_type: DataType::Utf8,
+            field: Arc::new(Field::new("cast", DataType::Utf8, false)),


Suggested change

field: Arc::new(Field::new("cast", DataType::Utf8, false)),

field: DataType::Utf8.into_nullable_field_ref(),

@alamb kindly added a helper for this (there are many places in this PR that would benefit!)

An aside is that I think we should ensure that these fields are always unnamed for consistency (and that the names of such a field are never used).

paleolimbot · 2025-10-27T18:15:34Z

datafusion/expr/src/expr_fn.rs

+/// Types that can be used to describe the result of a cast expression.
+pub trait IntoCastField {
+    fn into_cast_field(self, expr: &Expr) -> FieldRef;
+}


These are somewhat similar to the FieldExt traits that Andrew added to datafusion_common/src/datatypes.rs. I think DataFusion would benefit if it had a way to say "get me a FieldRef that is actually a data type", but I am also not sure this PR is the right place to do that.

paleolimbot · 2025-10-27T18:19:48Z

datafusion/expr/src/expr_fn.rs

-pub fn cast(expr: Expr, data_type: DataType) -> Expr {
-    Expr::Cast(Cast::new(Box::new(expr), data_type))
+pub fn cast<F>(expr: Expr, field: F) -> Expr
+where
+    F: IntoCastField,
+{
+    let field = field.into_cast_field(&expr);
+    Expr::Cast(Cast::new(Box::new(expr), field))


I see that a number of references to cast() were updated as cast(Arc::new(Field::new(...)))...I wonder if those can be reverted (in theory IntoCastField should handle the conversion?)

paleolimbot · 2025-10-27T18:23:48Z

datafusion/expr/src/expr_schema.rs

+                let (_, input_field) = expr.to_field(schema)?;
+                let mut combined_metadata = FieldMetadata::from(input_field.metadata());
+                combined_metadata.extend(FieldMetadata::from(field.metadata()));
+                let field = combined_metadata.add_to_field(field.as_ref().clone());
+                Ok(Arc::new(field))


I would prefer not to combine metadata here...this would mean that a Cast for an extension type would basically be a "reinterpret cast" not a "logical cast", which is what I believe the Cast is trying to represent in a logical plan. The Alias can currently add metadata in this way which I think is a bit fishy but is at least not attempting to represent a logical operation.

paleolimbot · 2025-10-27T18:29:03Z

datafusion/proto/proto/datafusion.proto

I think many of these changes should be reverted (I don't think this file is generated?)

paleolimbot · 2025-10-27T18:29:46Z

datafusion/proto/proto/datafusion.proto

-  datafusion_common.ArrowType arrow_type = 2;
+  datafusion_common.Field field = 2;


It is also possible to add metadata + nullability here to keep the protobuf backward compatible (not a requirement but is relatively easy to do)

paleolimbot · 2025-10-27T18:31:58Z

datafusion/expr/src/expr.rs

-    pub fn new(expr: Box<Expr>, data_type: DataType) -> Self {
-        Self { expr, data_type }
+    pub fn new(expr: Box<Expr>, field: FieldRef) -> Self {
+        Self { expr, field }


This change (and the corresponding change for TryCast::new() seem to have had caused significant disruption...it is probably worth keeping the original signature and adding a new function specifically for casts with metadata and/or nullability to minimize the disruption in DataFusion and downstream.

github-actions · 2025-12-30T02:13:40Z

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.

github-actions bot added sql SQL Planner logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates optimizer Optimizer rules substrait Changes to the substrait crate proto Related to proto crate functions Changes to functions implementation labels Oct 17, 2025

Weijun-H force-pushed the 18060-fieldref-in-cast branch from 1598f94 to 2a06d44 Compare October 17, 2025 11:59

github-actions bot added the core Core DataFusion crate label Oct 17, 2025

Weijun-H changed the title ~~WIP feat: Replace DataType with FieldRef in Cast~~ WIP feat: Replace DataType with FieldRef in Cast Oct 17, 2025

Weijun-H added the api change Changes the API exposed to users of the crate label Oct 17, 2025

Weijun-H force-pushed the 18060-fieldref-in-cast branch from 40f0012 to bd29587 Compare October 17, 2025 12:22

github-actions bot added the datasource Changes to the datasource crate label Oct 17, 2025

paleolimbot mentioned this pull request Oct 17, 2025

Add Field to Expr::Cast -- allow logical expressions to express a cast to an extension type #18136

Open

Weijun-H added 4 commits October 27, 2025 11:51

feat: Replace DataType with FieldRef in Cast

faf53c1

chore: Update tests

94f3803

chore: Fix clippy

2f5a531

feat: Update Cast and TryCast to use FieldRef for result metadata

7b32f01

Weijun-H force-pushed the 18060-fieldref-in-cast branch from 6563998 to 7b32f01 Compare October 27, 2025 09:57

Weijun-H added 3 commits October 27, 2025 12:46

chore: Fixed tests

fb3e50b

chore

8bee783

chore

7953f30

Weijun-H marked this pull request as ready for review October 27, 2025 16:44

Weijun-H changed the title ~~WIP feat: Replace DataType with FieldRef in Cast~~ feat: Replace DataType with FieldRef in Cast Oct 27, 2025

paleolimbot reviewed Oct 27, 2025

View reviewed changes

github-actions bot added the Stale PR has not had any activity for some time label Dec 30, 2025

github-actions bot closed this Jan 7, 2026

	field: Arc::new(Field::new("cast", DataType::Utf8, false)),
	field: DataType::Utf8.into_nullable_field_ref(),

		datafusion_common.ArrowType arrow_type = 2;
		datafusion_common.Field field = 2;

Conversation

Weijun-H commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

paleolimbot commented Oct 17, 2025

Uh oh!

paleolimbot commented Oct 27, 2025

Uh oh!

Weijun-H commented Oct 27, 2025

Uh oh!

paleolimbot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Weijun-H commented Oct 17, 2025 •

edited

Loading