From c74dd7792e62fc9d668079ed0f13e6fbded0480e Mon Sep 17 00:00:00 2001 From: Dewey Dunnington Date: Wed, 24 Jun 2026 14:27:11 -0500 Subject: [PATCH 01/19] try the generated answer --- Cargo.lock | 1 + datafusion/expr/src/expr_schema.rs | 166 ++++++++++++++++-- datafusion/physical-expr/Cargo.toml | 1 + .../physical-expr/src/expressions/cast.rs | 114 +++++++++++- 4 files changed, 260 insertions(+), 22 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a8371ac9db4ef..6874eac833cb8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2365,6 +2365,7 @@ name = "datafusion-physical-expr" version = "54.0.0" dependencies = [ "arrow", + "arrow-schema", "criterion", "datafusion-common", "datafusion-expr", diff --git a/datafusion/expr/src/expr_schema.rs b/datafusion/expr/src/expr_schema.rs index 039bbad65a660..9e1ccdf04bca6 100644 --- a/datafusion/expr/src/expr_schema.rs +++ b/datafusion/expr/src/expr_schema.rs @@ -69,18 +69,43 @@ pub trait ExprSchemable { -> Result<(DataType, bool)>; } -/// Derives the output field for a cast expression from the source field. +/// Derives the output field for a cast expression from the source and target fields. +/// +/// Metadata handling: +/// - Source field metadata is propagated by default +/// - Extension type metadata keys (`ARROW:extension:name` and `ARROW:extension:metadata`) +/// are taken from the target field, overwriting any extension metadata from the source. +/// This ensures casting does not incorrectly propagate extension type identity. +/// /// For `TryCast`, `force_nullable` is `true` since a failed cast returns NULL. fn cast_output_field( source_field: &FieldRef, - target_type: &DataType, + target_field: &FieldRef, force_nullable: bool, ) -> Arc { + use arrow_schema::extension::{EXTENSION_TYPE_METADATA_KEY, EXTENSION_TYPE_NAME_KEY}; + + // Start with source metadata + let mut metadata = source_field.metadata().clone(); + + // Remove any extension type metadata from source - these should not propagate through casts + metadata.remove(EXTENSION_TYPE_NAME_KEY); + metadata.remove(EXTENSION_TYPE_METADATA_KEY); + + // Add extension type metadata from the target field if present + let target_metadata = target_field.metadata(); + if let Some(name) = target_metadata.get(EXTENSION_TYPE_NAME_KEY) { + metadata.insert(EXTENSION_TYPE_NAME_KEY.to_string(), name.clone()); + } + if let Some(ext_meta) = target_metadata.get(EXTENSION_TYPE_METADATA_KEY) { + metadata.insert(EXTENSION_TYPE_METADATA_KEY.to_string(), ext_meta.clone()); + } + let mut f = source_field .as_ref() .clone() - .with_data_type(target_type.clone()) - .with_metadata(source_field.metadata().clone()); + .with_data_type(target_field.data_type().clone()) + .with_metadata(metadata); if force_nullable { f = f.with_nullable(true); } @@ -595,20 +620,16 @@ impl ExprSchemable for Expr { func.return_field_from_args(args) } // _ => Ok((self.get_type(schema)?, self.nullable(schema)?)), - Expr::Cast(Cast { expr, field }) => { - expr.to_field(schema).map(|(_table_ref, src)| { - cast_output_field(&src, field.data_type(), false) - }) - } + Expr::Cast(Cast { expr, field }) => expr + .to_field(schema) + .map(|(_table_ref, src)| cast_output_field(&src, field, false)), Expr::Placeholder(Placeholder { id: _, field: Some(field), }) => Ok(Arc::clone(field).renamed(&schema_name)), - Expr::TryCast(TryCast { expr, field }) => { - expr.to_field(schema).map(|(_table_ref, src)| { - cast_output_field(&src, field.data_type(), true) - }) - } + Expr::TryCast(TryCast { expr, field }) => expr + .to_field(schema) + .map(|(_table_ref, src)| cast_output_field(&src, field, true)), Expr::LambdaVariable(LambdaVariable { field: Some(field), .. }) => Ok(Arc::clone(field).renamed(&schema_name)), @@ -1208,4 +1229,121 @@ mod tests { assert_eq!(meta, expr.metadata(&schema).unwrap()); } + + #[test] + fn test_cast_extension_type_metadata() { + use crate::expr::Cast; + use arrow_schema::extension::{ + EXTENSION_TYPE_METADATA_KEY, EXTENSION_TYPE_NAME_KEY, + }; + + // Create a schema with a field that has extension type metadata + let mut source_meta = HashMap::new(); + source_meta.insert( + EXTENSION_TYPE_NAME_KEY.to_string(), + "arrow.uuid".to_string(), + ); + source_meta.insert("custom_key".to_string(), "custom_value".to_string()); + + let source_field = Field::new("foo", DataType::FixedSizeBinary(16), false) + .with_metadata(source_meta); + + let schema = MockExprSchema::new() + .with_data_type(DataType::FixedSizeBinary(16)) + .with_metadata(FieldMetadata::from(source_field.metadata().clone())); + + // Test 1: Cast to a type without extension metadata strips extension metadata + // but preserves non-extension metadata + let cast_expr = Expr::Cast(Cast { + expr: Box::new(col("foo")), + field: Arc::new(Field::new("", DataType::Utf8, true)), + }); + + let (_, result_field) = cast_expr.to_field(&schema).unwrap(); + assert!( + result_field + .metadata() + .get(EXTENSION_TYPE_NAME_KEY) + .is_none(), + "Extension type name should be stripped when target has no extension metadata" + ); + assert_eq!( + result_field.metadata().get("custom_key"), + Some(&"custom_value".to_string()), + "Non-extension metadata should be preserved" + ); + + // Test 2: Cast to a field with different extension type replaces extension metadata + let mut target_meta = HashMap::new(); + target_meta.insert( + EXTENSION_TYPE_NAME_KEY.to_string(), + "arrow.json".to_string(), + ); + target_meta.insert(EXTENSION_TYPE_METADATA_KEY.to_string(), "{}".to_string()); + + let target_field = + Field::new("", DataType::Utf8, true).with_metadata(target_meta); + + let cast_expr = Expr::Cast(Cast { + expr: Box::new(col("foo")), + field: Arc::new(target_field), + }); + + let (_, result_field) = cast_expr.to_field(&schema).unwrap(); + assert_eq!( + result_field.metadata().get(EXTENSION_TYPE_NAME_KEY), + Some(&"arrow.json".to_string()), + "Extension type name should come from target field" + ); + assert_eq!( + result_field.metadata().get(EXTENSION_TYPE_METADATA_KEY), + Some(&"{}".to_string()), + "Extension type metadata should come from target field" + ); + assert_eq!( + result_field.metadata().get("custom_key"), + Some(&"custom_value".to_string()), + "Non-extension metadata should still be preserved from source" + ); + } + + #[test] + fn test_try_cast_extension_type_metadata() { + use crate::expr::TryCast; + use arrow_schema::extension::EXTENSION_TYPE_NAME_KEY; + + // Create a schema with a field that has extension type metadata + let mut source_meta = HashMap::new(); + source_meta.insert( + EXTENSION_TYPE_NAME_KEY.to_string(), + "arrow.uuid".to_string(), + ); + + let source_field = Field::new("foo", DataType::FixedSizeBinary(16), false) + .with_metadata(source_meta); + + let schema = MockExprSchema::new() + .with_data_type(DataType::FixedSizeBinary(16)) + .with_metadata(FieldMetadata::from(source_field.metadata().clone())); + + // TryCast should also strip extension metadata when target has none + let try_cast_expr = Expr::TryCast(TryCast { + expr: Box::new(col("foo")), + field: Arc::new(Field::new("", DataType::Utf8, true)), + }); + + let (_, result_field) = try_cast_expr.to_field(&schema).unwrap(); + assert!( + result_field + .metadata() + .get(EXTENSION_TYPE_NAME_KEY) + .is_none(), + "Extension type name should be stripped in TryCast" + ); + // TryCast should force nullable + assert!( + result_field.is_nullable(), + "TryCast result should be nullable" + ); + } } diff --git a/datafusion/physical-expr/Cargo.toml b/datafusion/physical-expr/Cargo.toml index 65ef2a3ceb216..0588a777230fb 100644 --- a/datafusion/physical-expr/Cargo.toml +++ b/datafusion/physical-expr/Cargo.toml @@ -51,6 +51,7 @@ proto = [ [dependencies] arrow = { workspace = true } +arrow-schema = { workspace = true } datafusion-common = { workspace = true } datafusion-expr = { workspace = true } datafusion-expr-common = { workspace = true } diff --git a/datafusion/physical-expr/src/expressions/cast.rs b/datafusion/physical-expr/src/expressions/cast.rs index 26f06b546ad1d..b3fd9bf31e192 100644 --- a/datafusion/physical-expr/src/expressions/cast.rs +++ b/datafusion/physical-expr/src/expressions/cast.rs @@ -24,6 +24,7 @@ use crate::physical_expr::PhysicalExpr; use arrow::compute::{CastOptions, can_cast_types}; use arrow::datatypes::{DataType, DataType::*, FieldRef, Schema}; use arrow::record_batch::RecordBatch; +use arrow_schema::extension::{EXTENSION_TYPE_METADATA_KEY, EXTENSION_TYPE_NAME_KEY}; use datafusion_common::datatype::DataTypeExt; use datafusion_common::format::DEFAULT_FORMAT_OPTIONS; use datafusion_common::nested_struct::{ @@ -151,18 +152,36 @@ impl CastExpr { } fn resolved_target_field(&self, input_schema: &Schema) -> Result { - if is_default_target_field(&self.target_field) { - self.expr.return_field(input_schema).map(|field| { + self.expr.return_field(input_schema).map(|source_field| { + if is_default_target_field(&self.target_field) { + // Type-only cast: derive from source field but strip extension metadata + let mut metadata = source_field.metadata().clone(); + metadata.remove(EXTENSION_TYPE_NAME_KEY); + metadata.remove(EXTENSION_TYPE_METADATA_KEY); + Arc::new( - field + source_field .as_ref() .clone() - .with_data_type(self.cast_type().clone()), + .with_data_type(self.cast_type().clone()) + .with_metadata(metadata), ) - }) - } else { - Ok(Arc::clone(&self.target_field)) - } + } else { + // Explicit target field: use target's attributes but handle metadata carefully + // - Start with source's non-extension metadata + // - Then add all target metadata (including extension type metadata) + let mut metadata = source_field.metadata().clone(); + metadata.remove(EXTENSION_TYPE_NAME_KEY); + metadata.remove(EXTENSION_TYPE_METADATA_KEY); + + // Target metadata takes precedence (including extension type metadata) + for (k, v) in self.target_field.metadata() { + metadata.insert(k.clone(), v.clone()); + } + + Arc::new(self.target_field.as_ref().clone().with_metadata(metadata)) + } + }) } /// Check if casting from the specified source type to the target type is a @@ -1208,6 +1227,85 @@ mod tests { Ok(()) } + + #[test] + fn type_only_cast_strips_extension_metadata() -> Result<()> { + // When using type-only cast (new()), extension metadata from source should NOT propagate + let source_meta = HashMap::from([ + ( + EXTENSION_TYPE_NAME_KEY.to_string(), + "arrow.uuid".to_string(), + ), + ("custom_key".to_string(), "custom_value".to_string()), + ]); + let schema = Schema::new(vec![ + Field::new("a", FixedSizeBinary(16), false).with_metadata(source_meta), + ]); + + let expr = CastExpr::new(col("a", &schema)?, Utf8, None); + + let field = expr.return_field(&schema)?; + assert!( + field.metadata().get(EXTENSION_TYPE_NAME_KEY).is_none(), + "Type-only cast should strip extension type name from source" + ); + assert_eq!( + field.metadata().get("custom_key"), + Some(&"custom_value".to_string()), + "Type-only cast should preserve non-extension metadata" + ); + + Ok(()) + } + + #[test] + fn field_aware_cast_applies_target_extension_metadata() -> Result<()> { + // When using field-aware cast, target's extension metadata should be applied + let source_meta = HashMap::from([ + ( + EXTENSION_TYPE_NAME_KEY.to_string(), + "source.type".to_string(), + ), + ("source_key".to_string(), "source_value".to_string()), + ]); + let target_meta = HashMap::from([ + ( + EXTENSION_TYPE_NAME_KEY.to_string(), + "target.type".to_string(), + ), + ( + EXTENSION_TYPE_METADATA_KEY.to_string(), + "target_ext_meta".to_string(), + ), + ]); + let schema = Schema::new(vec![ + Field::new("a", FixedSizeBinary(16), false).with_metadata(source_meta), + ]); + + let target_field = + Arc::new(Field::new("b", Utf8, true).with_metadata(target_meta)); + let expr = + CastExpr::new_with_target_field(col("a", &schema)?, target_field, None); + + let field = expr.return_field(&schema)?; + assert_eq!( + field.metadata().get(EXTENSION_TYPE_NAME_KEY), + Some(&"target.type".to_string()), + "Field-aware cast should use target's extension type name" + ); + assert_eq!( + field.metadata().get(EXTENSION_TYPE_METADATA_KEY), + Some(&"target_ext_meta".to_string()), + "Field-aware cast should use target's extension type metadata" + ); + assert_eq!( + field.metadata().get("source_key"), + Some(&"source_value".to_string()), + "Field-aware cast should preserve source's non-extension metadata" + ); + + Ok(()) + } } /// Tests for the `try_to_proto` / `try_from_proto` hooks. From b4fdb493ebb461cc4b217b220c39681ec0cabd6d Mon Sep 17 00:00:00 2001 From: Dewey Dunnington Date: Fri, 26 Jun 2026 12:26:44 -0500 Subject: [PATCH 02/19] don't call return_field for the source info rewrite --- .../physical-expr-adapter/src/rewrite.rs | 5 +- .../physical-expr/src/expressions/cast.rs | 57 +++++++++++++++++-- 2 files changed, 55 insertions(+), 7 deletions(-) diff --git a/datafusion/physical-expr-adapter/src/rewrite.rs b/datafusion/physical-expr-adapter/src/rewrite.rs index 7345a587ee6a4..787178a651dba 100644 --- a/datafusion/physical-expr-adapter/src/rewrite.rs +++ b/datafusion/physical-expr-adapter/src/rewrite.rs @@ -102,7 +102,7 @@ pub fn rewrite_file_row_index_expr( rewrite_scalar_udf::(expr, |_| { let source = Arc::new(Column::new(row_index_name, row_index_idx)); let target_field = Arc::new(Field::new("file_row_index", DataType::Int64, true)); - Ok(Arc::new(CastExpr::new_with_target_field( + Ok(Arc::new(CastExpr::new_with_exact_target_field( source, target_field, None, @@ -319,6 +319,8 @@ mod tests { assert_eq!(source.name(), "__datafusion_file_row_index"); assert_eq!(source.index(), 2); + // The row index column is at index 2, beyond the user-visible schema. + // With exact target field mode, return_field does not consult the source. let input_schema = Schema::new(vec![ Field::new("value", DataType::Int64, true), Field::new("__datafusion_file_row_index", DataType::Int64, false) @@ -331,6 +333,7 @@ mod tests { assert_eq!(return_field.name(), "file_row_index"); assert_eq!(return_field.data_type(), &DataType::Int64); assert!(return_field.is_nullable()); + // Exact target field does not preserve source metadata assert!(return_field.metadata().is_empty()); Ok(()) } diff --git a/datafusion/physical-expr/src/expressions/cast.rs b/datafusion/physical-expr/src/expressions/cast.rs index b3fd9bf31e192..a1b45573933ab 100644 --- a/datafusion/physical-expr/src/expressions/cast.rs +++ b/datafusion/physical-expr/src/expressions/cast.rs @@ -64,6 +64,10 @@ pub struct CastExpr { target_field: FieldRef, /// Cast options cast_options: CastOptions<'static>, + /// Whether to preserve non-extension metadata from the source field. + /// When true (default), source metadata is merged with target metadata. + /// When false, only the target field's metadata is used. + preserve_source_metadata: bool, } // Manually derive PartialEq and Hash to work around https://github.com/rust-lang/rust/issues/78808 @@ -72,6 +76,9 @@ impl PartialEq for CastExpr { self.expr.eq(&other.expr) && self.target_field.eq(&other.target_field) && self.cast_options.eq(&other.cast_options) + && self + .preserve_source_metadata + .eq(&other.preserve_source_metadata) } } @@ -80,6 +87,7 @@ impl Hash for CastExpr { self.expr.hash(state); self.target_field.hash(state); self.cast_options.hash(state); + self.preserve_source_metadata.hash(state); } } @@ -128,6 +136,29 @@ impl CastExpr { expr, target_field, cast_options: cast_options.unwrap_or(DEFAULT_CAST_OPTIONS), + preserve_source_metadata: true, + } + } + + /// Create a new `CastExpr` with an explicit target `FieldRef`, using the + /// target field exactly without merging source metadata. + /// + /// Unlike [`CastExpr::new_with_target_field`], this constructor does NOT + /// merge non-extension metadata from the source field. The target field's + /// metadata (if any) is used exactly as provided. This is useful when the + /// cast represents a complete type transformation where source metadata + /// should not propagate (e.g., rewriting placeholder functions like + /// `file_row_index()` to concrete column references). + pub fn new_with_exact_target_field( + expr: Arc, + target_field: FieldRef, + cast_options: Option>, + ) -> Self { + Self { + expr, + target_field, + cast_options: cast_options.unwrap_or(DEFAULT_CAST_OPTIONS), + preserve_source_metadata: false, } } @@ -151,7 +182,20 @@ impl CastExpr { &self.cast_options } + /// Whether source metadata is preserved through the cast. + pub fn preserve_source_metadata(&self) -> bool { + self.preserve_source_metadata + } + fn resolved_target_field(&self, input_schema: &Schema) -> Result { + // When using exact target field mode, return the target field directly + // without consulting the source expression (which may reference columns + // beyond the schema, e.g., virtual row-index columns appended at scan time). + if !self.preserve_source_metadata && !is_default_target_field(&self.target_field) + { + return Ok(Arc::clone(&self.target_field)); + } + self.expr.return_field(input_schema).map(|source_field| { if is_default_target_field(&self.target_field) { // Type-only cast: derive from source field but strip extension metadata @@ -167,7 +211,7 @@ impl CastExpr { .with_metadata(metadata), ) } else { - // Explicit target field: use target's attributes but handle metadata carefully + // Explicit target field with source metadata preservation: // - Start with source's non-extension metadata // - Then add all target metadata (including extension type metadata) let mut metadata = source_field.metadata().clone(); @@ -279,11 +323,12 @@ impl PhysicalExpr for CastExpr { self: Arc, children: Vec>, ) -> Result> { - Ok(Arc::new(CastExpr::new_with_target_field( - Arc::clone(&children[0]), - Arc::clone(&self.target_field), - Some(self.cast_options.clone()), - ))) + Ok(Arc::new(CastExpr { + expr: Arc::clone(&children[0]), + target_field: Arc::clone(&self.target_field), + cast_options: self.cast_options.clone(), + preserve_source_metadata: self.preserve_source_metadata, + })) } fn evaluate_bounds(&self, children: &[&Interval]) -> Result { From 1d1608b81c9334f28b5b7b0ab1b7934c23df0caf Mon Sep 17 00:00:00 2001 From: Dewey Dunnington Date: Wed, 1 Jul 2026 10:55:27 -0500 Subject: [PATCH 03/19] comments --- datafusion/expr/src/expr_schema.rs | 211 +++++++++--------- .../physical-expr/src/expressions/cast.rs | 21 +- datafusion/physical-expr/src/planner.rs | 61 ++++- 3 files changed, 176 insertions(+), 117 deletions(-) diff --git a/datafusion/expr/src/expr_schema.rs b/datafusion/expr/src/expr_schema.rs index 9e1ccdf04bca6..3305b54c390a6 100644 --- a/datafusion/expr/src/expr_schema.rs +++ b/datafusion/expr/src/expr_schema.rs @@ -1231,119 +1231,114 @@ mod tests { } #[test] - fn test_cast_extension_type_metadata() { - use crate::expr::Cast; + fn test_cast_and_try_cast_extension_type_metadata() { + use crate::expr::{Cast, TryCast}; use arrow_schema::extension::{ EXTENSION_TYPE_METADATA_KEY, EXTENSION_TYPE_NAME_KEY, }; - // Create a schema with a field that has extension type metadata - let mut source_meta = HashMap::new(); - source_meta.insert( - EXTENSION_TYPE_NAME_KEY.to_string(), - "arrow.uuid".to_string(), - ); - source_meta.insert("custom_key".to_string(), "custom_value".to_string()); - - let source_field = Field::new("foo", DataType::FixedSizeBinary(16), false) - .with_metadata(source_meta); - - let schema = MockExprSchema::new() - .with_data_type(DataType::FixedSizeBinary(16)) - .with_metadata(FieldMetadata::from(source_field.metadata().clone())); - - // Test 1: Cast to a type without extension metadata strips extension metadata - // but preserves non-extension metadata - let cast_expr = Expr::Cast(Cast { - expr: Box::new(col("foo")), - field: Arc::new(Field::new("", DataType::Utf8, true)), - }); - - let (_, result_field) = cast_expr.to_field(&schema).unwrap(); - assert!( - result_field - .metadata() - .get(EXTENSION_TYPE_NAME_KEY) - .is_none(), - "Extension type name should be stripped when target has no extension metadata" - ); - assert_eq!( - result_field.metadata().get("custom_key"), - Some(&"custom_value".to_string()), - "Non-extension metadata should be preserved" - ); - - // Test 2: Cast to a field with different extension type replaces extension metadata - let mut target_meta = HashMap::new(); - target_meta.insert( - EXTENSION_TYPE_NAME_KEY.to_string(), - "arrow.json".to_string(), - ); - target_meta.insert(EXTENSION_TYPE_METADATA_KEY.to_string(), "{}".to_string()); - - let target_field = - Field::new("", DataType::Utf8, true).with_metadata(target_meta); - - let cast_expr = Expr::Cast(Cast { - expr: Box::new(col("foo")), - field: Arc::new(target_field), - }); - - let (_, result_field) = cast_expr.to_field(&schema).unwrap(); - assert_eq!( - result_field.metadata().get(EXTENSION_TYPE_NAME_KEY), - Some(&"arrow.json".to_string()), - "Extension type name should come from target field" - ); - assert_eq!( - result_field.metadata().get(EXTENSION_TYPE_METADATA_KEY), - Some(&"{}".to_string()), - "Extension type metadata should come from target field" - ); - assert_eq!( - result_field.metadata().get("custom_key"), - Some(&"custom_value".to_string()), - "Non-extension metadata should still be preserved from source" - ); - } - - #[test] - fn test_try_cast_extension_type_metadata() { - use crate::expr::TryCast; - use arrow_schema::extension::EXTENSION_TYPE_NAME_KEY; - - // Create a schema with a field that has extension type metadata - let mut source_meta = HashMap::new(); - source_meta.insert( - EXTENSION_TYPE_NAME_KEY.to_string(), - "arrow.uuid".to_string(), - ); - - let source_field = Field::new("foo", DataType::FixedSizeBinary(16), false) - .with_metadata(source_meta); - - let schema = MockExprSchema::new() - .with_data_type(DataType::FixedSizeBinary(16)) - .with_metadata(FieldMetadata::from(source_field.metadata().clone())); + // Helper to build either Cast or TryCast expression + fn make_cast_expr( + expr: Expr, + target_field: FieldRef, + use_try_cast: bool, + ) -> Expr { + if use_try_cast { + Expr::TryCast(TryCast { + expr: Box::new(expr), + field: target_field, + }) + } else { + Expr::Cast(Cast { + expr: Box::new(expr), + field: target_field, + }) + } + } - // TryCast should also strip extension metadata when target has none - let try_cast_expr = Expr::TryCast(TryCast { - expr: Box::new(col("foo")), - field: Arc::new(Field::new("", DataType::Utf8, true)), - }); + // Run the same test logic for both Cast and TryCast + for use_try_cast in [false, true] { + let cast_name = if use_try_cast { "TryCast" } else { "Cast" }; + + // Create a schema with a field that has extension type metadata + let mut source_meta = HashMap::new(); + source_meta.insert( + EXTENSION_TYPE_NAME_KEY.to_string(), + "arrow.uuid".to_string(), + ); + source_meta.insert("custom_key".to_string(), "custom_value".to_string()); + + let source_field = Field::new("foo", DataType::FixedSizeBinary(16), false) + .with_metadata(source_meta); + + let schema = MockExprSchema::new() + .with_data_type(DataType::FixedSizeBinary(16)) + .with_metadata(FieldMetadata::from(source_field.metadata().clone())); + + // Test 1: Cast to a type without extension metadata strips extension metadata + // but preserves non-extension metadata + let cast_expr = make_cast_expr( + col("foo"), + Arc::new(Field::new("", DataType::Utf8, true)), + use_try_cast, + ); + + let (_, result_field) = cast_expr.to_field(&schema).unwrap(); + assert!( + result_field + .metadata() + .get(EXTENSION_TYPE_NAME_KEY) + .is_none(), + "{cast_name}: Extension type name should be stripped when target has no extension metadata" + ); + assert_eq!( + result_field.metadata().get("custom_key"), + Some(&"custom_value".to_string()), + "{cast_name}: Non-extension metadata should be preserved" + ); + if use_try_cast { + assert!( + result_field.is_nullable(), + "TryCast result should be nullable" + ); + } - let (_, result_field) = try_cast_expr.to_field(&schema).unwrap(); - assert!( - result_field - .metadata() - .get(EXTENSION_TYPE_NAME_KEY) - .is_none(), - "Extension type name should be stripped in TryCast" - ); - // TryCast should force nullable - assert!( - result_field.is_nullable(), - "TryCast result should be nullable" - ); + // Test 2: Cast to a field with different extension type replaces extension metadata + let mut target_meta = HashMap::new(); + target_meta.insert( + EXTENSION_TYPE_NAME_KEY.to_string(), + "arrow.json".to_string(), + ); + target_meta.insert(EXTENSION_TYPE_METADATA_KEY.to_string(), "{}".to_string()); + + let target_field = + Field::new("", DataType::Utf8, true).with_metadata(target_meta); + + let cast_expr = + make_cast_expr(col("foo"), Arc::new(target_field), use_try_cast); + + let (_, result_field) = cast_expr.to_field(&schema).unwrap(); + assert_eq!( + result_field.metadata().get(EXTENSION_TYPE_NAME_KEY), + Some(&"arrow.json".to_string()), + "{cast_name}: Extension type name should come from target field" + ); + assert_eq!( + result_field.metadata().get(EXTENSION_TYPE_METADATA_KEY), + Some(&"{}".to_string()), + "{cast_name}: Extension type metadata should come from target field" + ); + assert_eq!( + result_field.metadata().get("custom_key"), + Some(&"custom_value".to_string()), + "{cast_name}: Non-extension metadata should still be preserved from source" + ); + if use_try_cast { + assert!( + result_field.is_nullable(), + "TryCast result should be nullable" + ); + } + } } } diff --git a/datafusion/physical-expr/src/expressions/cast.rs b/datafusion/physical-expr/src/expressions/cast.rs index a1b45573933ab..cedc28aa09851 100644 --- a/datafusion/physical-expr/src/expressions/cast.rs +++ b/datafusion/physical-expr/src/expressions/cast.rs @@ -213,14 +213,22 @@ impl CastExpr { } else { // Explicit target field with source metadata preservation: // - Start with source's non-extension metadata - // - Then add all target metadata (including extension type metadata) + // - Add only extension type metadata from the target field + // This matches the logical layer's `cast_output_field` behavior. let mut metadata = source_field.metadata().clone(); metadata.remove(EXTENSION_TYPE_NAME_KEY); metadata.remove(EXTENSION_TYPE_METADATA_KEY); - // Target metadata takes precedence (including extension type metadata) - for (k, v) in self.target_field.metadata() { - metadata.insert(k.clone(), v.clone()); + // Add extension type metadata from the target field if present + let target_metadata = self.target_field.metadata(); + if let Some(name) = target_metadata.get(EXTENSION_TYPE_NAME_KEY) { + metadata.insert(EXTENSION_TYPE_NAME_KEY.to_string(), name.clone()); + } + if let Some(ext_meta) = target_metadata.get(EXTENSION_TYPE_METADATA_KEY) { + metadata.insert( + EXTENSION_TYPE_METADATA_KEY.to_string(), + ext_meta.clone(), + ); } Arc::new(self.target_field.as_ref().clone().with_metadata(metadata)) @@ -1322,6 +1330,7 @@ mod tests { EXTENSION_TYPE_METADATA_KEY.to_string(), "target_ext_meta".to_string(), ), + ("target_key".to_string(), "target_value".to_string()), ]); let schema = Schema::new(vec![ Field::new("a", FixedSizeBinary(16), false).with_metadata(source_meta), @@ -1348,6 +1357,10 @@ mod tests { Some(&"source_value".to_string()), "Field-aware cast should preserve source's non-extension metadata" ); + assert!( + field.metadata().get("target_key").is_none(), + "Field-aware cast should not preserve target's non-extension metadata" + ); Ok(()) } diff --git a/datafusion/physical-expr/src/planner.rs b/datafusion/physical-expr/src/planner.rs index d0d0508a106a5..4a9b593ffd3f0 100644 --- a/datafusion/physical-expr/src/planner.rs +++ b/datafusion/physical-expr/src/planner.rs @@ -598,6 +598,7 @@ pub fn logical2physical(expr: &Expr, schema: &Schema) -> Arc { mod tests { use arrow::array::{ArrayRef, BooleanArray, RecordBatch, StringArray}; use arrow::datatypes::{DataType, Field}; + use arrow_schema::extension::{EXTENSION_TYPE_METADATA_KEY, EXTENSION_TYPE_NAME_KEY}; use datafusion_expr::col; use super::*; @@ -645,9 +646,21 @@ mod tests { #[test] fn test_cast_lowering_preserves_target_field_metadata() -> Result<()> { let schema = test_cast_schema(); + + // Target field with both extension metadata and custom metadata. + // Per cast_output_field semantics, only extension metadata should propagate. let target_field = Arc::new( - Field::new("cast_target", DataType::Int64, true) - .with_metadata([("target_meta".to_string(), "1".to_string())].into()), + Field::new("cast_target", DataType::Int64, true).with_metadata( + [ + ( + EXTENSION_TYPE_NAME_KEY.to_string(), + "arrow.json".to_string(), + ), + (EXTENSION_TYPE_METADATA_KEY.to_string(), "{}".to_string()), + ("custom_target_meta".to_string(), "ignored".to_string()), + ] + .into(), + ), ); let cast_expr = Expr::Cast(Cast::new_from_field( Box::new(col("a")), @@ -657,8 +670,25 @@ mod tests { let physical = lower_cast_expr(&cast_expr, &schema)?; let cast = as_planner_cast(&physical); + // The CastExpr stores the target field as given assert_eq!(cast.target_field(), &target_field); - assert_eq!(physical.return_field(&schema)?, target_field); + + // But return_field should only propagate extension metadata from target, + // aligning with logical-layer cast_output_field semantics + let returned = physical.return_field(&schema)?; + assert_eq!( + returned.metadata().get(EXTENSION_TYPE_NAME_KEY), + Some(&"arrow.json".to_string()) + ); + assert_eq!( + returned.metadata().get(EXTENSION_TYPE_METADATA_KEY), + Some(&"{}".to_string()) + ); + // Custom target metadata should NOT propagate + assert!( + returned.metadata().get("custom_target_meta").is_none(), + "Non-extension target metadata should not propagate" + ); assert!(physical.nullable(&schema)?); Ok(()) @@ -684,9 +714,19 @@ mod tests { #[test] fn test_cast_lowering_preserves_same_type_field_semantics() -> Result<()> { let schema = test_cast_schema(); + + // Same-type cast with extension metadata on target. + // Per cast_output_field semantics, only extension metadata should propagate. let target_field = Arc::new( Field::new("same_type_cast", DataType::Int32, true).with_metadata( - [("target_meta".to_string(), "same-type".to_string())].into(), + [ + ( + EXTENSION_TYPE_NAME_KEY.to_string(), + "arrow.opaque".to_string(), + ), + ("custom_meta".to_string(), "ignored".to_string()), + ] + .into(), ), ); let cast_expr = Expr::Cast(Cast::new_from_field( @@ -698,7 +738,18 @@ mod tests { let cast = as_planner_cast(&physical); assert_eq!(cast.target_field(), &target_field); - assert_eq!(physical.return_field(&schema)?, target_field); + + // return_field should only have extension metadata from target + let returned = physical.return_field(&schema)?; + assert_eq!( + returned.metadata().get(EXTENSION_TYPE_NAME_KEY), + Some(&"arrow.opaque".to_string()) + ); + // Custom target metadata should NOT propagate + assert!( + returned.metadata().get("custom_meta").is_none(), + "Non-extension target metadata should not propagate" + ); assert!(physical.nullable(&schema)?); Ok(()) From 944cb030e98571ad3c0c3f8f34cc9bbeaf8bf5b5 Mon Sep 17 00:00:00 2001 From: Dewey Dunnington Date: Wed, 1 Jul 2026 14:43:27 -0500 Subject: [PATCH 04/19] update the cast representation to remove is_default_target_field() --- .../physical-expr-adapter/src/rewrite.rs | 2 +- .../src/schema_rewriter.rs | 5 +- .../src/equivalence/properties/dependency.rs | 2 +- .../physical-expr/src/expressions/cast.rs | 370 ++++++++++++------ datafusion/physical-expr/src/planner.rs | 25 +- datafusion/physical-plan/src/common.rs | 2 +- datafusion/pruning/src/pruning_predicate.rs | 3 +- 7 files changed, 275 insertions(+), 134 deletions(-) diff --git a/datafusion/physical-expr-adapter/src/rewrite.rs b/datafusion/physical-expr-adapter/src/rewrite.rs index 787178a651dba..ea9416a1ea01b 100644 --- a/datafusion/physical-expr-adapter/src/rewrite.rs +++ b/datafusion/physical-expr-adapter/src/rewrite.rs @@ -104,7 +104,7 @@ pub fn rewrite_file_row_index_expr( let target_field = Arc::new(Field::new("file_row_index", DataType::Int64, true)); Ok(Arc::new(CastExpr::new_with_exact_target_field( source, - target_field, + &target_field, None, ))) }) diff --git a/datafusion/physical-expr-adapter/src/schema_rewriter.rs b/datafusion/physical-expr-adapter/src/schema_rewriter.rs index d9eed669ba98f..5452e1ceba561 100644 --- a/datafusion/physical-expr-adapter/src/schema_rewriter.rs +++ b/datafusion/physical-expr-adapter/src/schema_rewriter.rs @@ -447,9 +447,10 @@ impl DefaultPhysicalExprAdapterRewriter { )) })?; + let target_field = Arc::new(logical_field.clone()); Ok(Transformed::yes(Arc::new(CastExpr::new_with_target_field( Arc::new(resolved_column), - Arc::new(logical_field.clone()), + &target_field, None, )))) } @@ -851,7 +852,7 @@ mod tests { let expected = Arc::new(CastExpr::new_with_target_field( Arc::new(Column::new("data", 0)), - logical_field, + &logical_field, None, )) as Arc; diff --git a/datafusion/physical-expr/src/equivalence/properties/dependency.rs b/datafusion/physical-expr/src/equivalence/properties/dependency.rs index 2ebc71559fcf4..64a089ce4e71b 100644 --- a/datafusion/physical-expr/src/equivalence/properties/dependency.rs +++ b/datafusion/physical-expr/src/equivalence/properties/dependency.rs @@ -940,7 +940,7 @@ mod tests { let col_c = col("c", schema.as_ref())?; let cast_c = Arc::new(CastExpr::new_with_target_field( col_c, - Arc::new(Field::new("c", DataType::Date32, true)), + &Arc::new(Field::new("c", DataType::Date32, true)), None, )) as _; let required_sort = vec![PhysicalSortExpr::new_default(col("c", &schema)?)]; diff --git a/datafusion/physical-expr/src/expressions/cast.rs b/datafusion/physical-expr/src/expressions/cast.rs index cedc28aa09851..3ee6474ef6199 100644 --- a/datafusion/physical-expr/src/expressions/cast.rs +++ b/datafusion/physical-expr/src/expressions/cast.rs @@ -15,6 +15,7 @@ // specific language governing permissions and limitations // under the License. +use std::collections::HashMap; use std::fmt; use std::hash::Hash; use std::sync::Arc; @@ -22,10 +23,9 @@ use std::sync::Arc; use crate::physical_expr::PhysicalExpr; use arrow::compute::{CastOptions, can_cast_types}; -use arrow::datatypes::{DataType, DataType::*, FieldRef, Schema}; +use arrow::datatypes::{DataType, DataType::*, Field, FieldRef, Schema}; use arrow::record_batch::RecordBatch; use arrow_schema::extension::{EXTENSION_TYPE_METADATA_KEY, EXTENSION_TYPE_NAME_KEY}; -use datafusion_common::datatype::DataTypeExt; use datafusion_common::format::DEFAULT_FORMAT_OPTIONS; use datafusion_common::nested_struct::{ requires_nested_struct_cast, validate_data_type_compatibility, @@ -60,83 +60,110 @@ fn can_cast_named_struct_types(source: &DataType, target: &DataType) -> bool { pub struct CastExpr { /// The expression to cast pub expr: Arc, - /// Field metadata describing the desired output after casting - target_field: FieldRef, + /// The target data type to cast to + target_type: DataType, + /// Explicit metadata for the output field, or `None` to pass through source metadata + /// (with extension type keys stripped). + /// + /// When `merge_source_metadata` is `true`, this contains extension type metadata + /// to overlay on top of source's non-extension metadata. + /// When `merge_source_metadata` is `false`, this is the exact metadata to use. + target_metadata: Option>, + /// Explicit nullability for the output field, or `None` to pass through source nullability. + target_nullable: Option, + /// When `true` and `target_metadata` is `Some`, merge source's non-extension metadata + /// with the target's extension metadata. When `false`, use `target_metadata` exactly + /// without consulting the source. + merge_source_metadata: bool, /// Cast options cast_options: CastOptions<'static>, - /// Whether to preserve non-extension metadata from the source field. - /// When true (default), source metadata is merged with target metadata. - /// When false, only the target field's metadata is used. - preserve_source_metadata: bool, } // Manually derive PartialEq and Hash to work around https://github.com/rust-lang/rust/issues/78808 impl PartialEq for CastExpr { fn eq(&self, other: &Self) -> bool { self.expr.eq(&other.expr) - && self.target_field.eq(&other.target_field) + && self.target_type.eq(&other.target_type) + && self.target_metadata.eq(&other.target_metadata) + && self.target_nullable.eq(&other.target_nullable) + && self.merge_source_metadata.eq(&other.merge_source_metadata) && self.cast_options.eq(&other.cast_options) - && self - .preserve_source_metadata - .eq(&other.preserve_source_metadata) } } impl Hash for CastExpr { fn hash(&self, state: &mut H) { self.expr.hash(state); - self.target_field.hash(state); + self.target_type.hash(state); + // Hash the metadata by iterating over sorted keys for deterministic ordering + if let Some(metadata) = &self.target_metadata { + let mut entries: Vec<_> = metadata.iter().collect(); + entries.sort_by_key(|(k, _)| *k); + for (k, v) in entries { + k.hash(state); + v.hash(state); + } + } + self.target_nullable.hash(state); + self.merge_source_metadata.hash(state); self.cast_options.hash(state); - self.preserve_source_metadata.hash(state); } } impl CastExpr { /// Create a new `CastExpr` using only a `DataType`. /// - /// This constructor is provided for compatibility with existing call sites - /// that only know the target type. It synthesizes a ``Field`` with the - /// given type (**nullable by default**) and no name metadata. Callers that - /// already have a `FieldRef` (for example, coming from schema inference or a - /// resolved column) should prefer [`CastExpr::new_with_target_field`], which - /// preserves the field's name, nullability, and other metadata. In other - /// words: + /// This constructor creates a type-only cast where metadata and nullability + /// are passed through from the source expression (with extension type keys + /// stripped from metadata). This is the most common use case when you only + /// need to change the data type. /// - /// * use `new()` when only a `DataType` is available and you want the legacy - /// semantics of a type-only cast - /// * use `new_with_target_field()` when you need explicit field - /// metadata/name/nullability preserved + /// For explicit control over the output field's metadata and nullability, + /// use [`CastExpr::new_with_target_field`] or the individual builder methods. pub fn new( expr: Arc, cast_type: DataType, cast_options: Option>, ) -> Self { - Self::new_with_target_field( + Self { expr, - cast_type.into_nullable_field_ref(), - cast_options, - ) + target_type: cast_type, + target_metadata: None, + target_nullable: None, + merge_source_metadata: true, + cast_options: cast_options.unwrap_or(DEFAULT_CAST_OPTIONS), + } } /// Create a new `CastExpr` with an explicit target `FieldRef`. /// - /// The provided `target_field` is used verbatim for the expression's - /// return schema, so the field's name, nullability, and other metadata are - /// preserved. This is the preferred constructor when the caller already - /// has field information (for example, during logical-to-physical planning). + /// The provided `target_field` determines the output characteristics: + /// - The field's data type becomes the cast target type + /// - The field's metadata is used to extract extension type information, + /// which is then merged with source metadata (pass-through behavior) + /// - The field's nullability is preserved + /// + /// This is the preferred constructor when the caller already has field + /// information (for example, during logical-to-physical planning) but still + /// wants source metadata to pass through. /// - /// See [`CastExpr::new`] for the compatibility constructor that only accepts - /// a `DataType`. + /// See [`CastExpr::new`] for type-only casts and + /// [`CastExpr::new_with_exact_target_field`] for exact field matching. pub fn new_with_target_field( expr: Arc, - target_field: FieldRef, + target_field: &FieldRef, cast_options: Option>, ) -> Self { + // Extract only extension type metadata from the target field to merge + // with source metadata (pass-through behavior with extension type overlay) + let extension_metadata = extract_extension_metadata(target_field.metadata()); Self { expr, - target_field, + target_type: target_field.data_type().clone(), + target_metadata: extension_metadata, + target_nullable: Some(target_field.is_nullable()), + merge_source_metadata: true, cast_options: cast_options.unwrap_or(DEFAULT_CAST_OPTIONS), - preserve_source_metadata: true, } } @@ -145,20 +172,22 @@ impl CastExpr { /// /// Unlike [`CastExpr::new_with_target_field`], this constructor does NOT /// merge non-extension metadata from the source field. The target field's - /// metadata (if any) is used exactly as provided. This is useful when the - /// cast represents a complete type transformation where source metadata - /// should not propagate (e.g., rewriting placeholder functions like + /// metadata is used exactly as provided. This is useful when the cast + /// represents a complete type transformation where source metadata should + /// not propagate (e.g., rewriting placeholder functions like /// `file_row_index()` to concrete column references). pub fn new_with_exact_target_field( expr: Arc, - target_field: FieldRef, + target_field: &FieldRef, cast_options: Option>, ) -> Self { Self { expr, - target_field, + target_type: target_field.data_type().clone(), + target_metadata: Some(target_field.metadata().clone()), + target_nullable: Some(target_field.is_nullable()), + merge_source_metadata: false, cast_options: cast_options.unwrap_or(DEFAULT_CAST_OPTIONS), - preserve_source_metadata: false, } } @@ -169,12 +198,37 @@ impl CastExpr { /// The data type to cast to pub fn cast_type(&self) -> &DataType { - self.target_field.data_type() + &self.target_type + } + + /// Explicit metadata for the output field, or `None` to pass through source metadata. + pub fn target_metadata(&self) -> Option<&HashMap> { + self.target_metadata.as_ref() + } + + /// Explicit nullability for the output field, or `None` to pass through source nullability. + pub fn target_nullable(&self) -> Option { + self.target_nullable } - /// Field metadata describing the output column after casting. - pub fn target_field(&self) -> &FieldRef { - &self.target_field + /// Returns a computed target `FieldRef` for compatibility with existing callers. + /// + /// This synthesizes a field based on the current configuration. The returned + /// field may not match what `return_field()` returns when evaluated against + /// a schema, since `return_field()` may incorporate source field information. + /// + /// Prefer using [`cast_type()`], [`target_metadata()`], and [`target_nullable()`] + /// for direct access to the individual components. + /// + /// [`cast_type()`]: CastExpr::cast_type + /// [`target_metadata()`]: CastExpr::target_metadata + /// [`target_nullable()`]: CastExpr::target_nullable + pub fn target_field(&self) -> FieldRef { + let metadata = self.target_metadata.clone().unwrap_or_default(); + let nullable = self.target_nullable.unwrap_or(true); + Arc::new( + Field::new("", self.target_type.clone(), nullable).with_metadata(metadata), + ) } /// The cast options @@ -182,57 +236,72 @@ impl CastExpr { &self.cast_options } - /// Whether source metadata is preserved through the cast. - pub fn preserve_source_metadata(&self) -> bool { - self.preserve_source_metadata + /// Whether this cast has explicit metadata (vs pass-through from source). + pub fn has_explicit_metadata(&self) -> bool { + self.target_metadata.is_some() + } + + /// Whether this cast has explicit nullability (vs pass-through from source). + pub fn has_explicit_nullability(&self) -> bool { + self.target_nullable.is_some() } fn resolved_target_field(&self, input_schema: &Schema) -> Result { - // When using exact target field mode, return the target field directly - // without consulting the source expression (which may reference columns - // beyond the schema, e.g., virtual row-index columns appended at scan time). - if !self.preserve_source_metadata && !is_default_target_field(&self.target_field) + // If we're not merging with source AND both metadata and nullability are + // explicit, we can build the field without consulting the source expression. + // This is important for cases where the source references columns beyond + // the schema (e.g., virtual row-index columns appended at scan time). + if !self.merge_source_metadata + && let (Some(metadata), Some(nullable)) = + (&self.target_metadata, self.target_nullable) { - return Ok(Arc::clone(&self.target_field)); + return Ok(Arc::new( + Field::new("", self.target_type.clone(), nullable) + .with_metadata(metadata.clone()), + )); } self.expr.return_field(input_schema).map(|source_field| { - if is_default_target_field(&self.target_field) { - // Type-only cast: derive from source field but strip extension metadata - let mut metadata = source_field.metadata().clone(); - metadata.remove(EXTENSION_TYPE_NAME_KEY); - metadata.remove(EXTENSION_TYPE_METADATA_KEY); - - Arc::new( - source_field - .as_ref() - .clone() - .with_data_type(self.cast_type().clone()) - .with_metadata(metadata), - ) - } else { - // Explicit target field with source metadata preservation: - // - Start with source's non-extension metadata - // - Add only extension type metadata from the target field - // This matches the logical layer's `cast_output_field` behavior. - let mut metadata = source_field.metadata().clone(); - metadata.remove(EXTENSION_TYPE_NAME_KEY); - metadata.remove(EXTENSION_TYPE_METADATA_KEY); - - // Add extension type metadata from the target field if present - let target_metadata = self.target_field.metadata(); - if let Some(name) = target_metadata.get(EXTENSION_TYPE_NAME_KEY) { - metadata.insert(EXTENSION_TYPE_NAME_KEY.to_string(), name.clone()); - } - if let Some(ext_meta) = target_metadata.get(EXTENSION_TYPE_METADATA_KEY) { - metadata.insert( - EXTENSION_TYPE_METADATA_KEY.to_string(), - ext_meta.clone(), + // Build metadata based on merge_source_metadata flag + let metadata = if self.merge_source_metadata { + // Merge mode: start with source non-extension metadata + let mut meta = source_field.metadata().clone(); + meta.remove(EXTENSION_TYPE_NAME_KEY); + meta.remove(EXTENSION_TYPE_METADATA_KEY); + // Then overlay any explicit metadata (extension keys from target) + if let Some(explicit_metadata) = &self.target_metadata { + meta.extend( + explicit_metadata + .iter() + .map(|(k, v)| (k.clone(), v.clone())), ); } - - Arc::new(self.target_field.as_ref().clone().with_metadata(metadata)) - } + meta + } else if let Some(explicit_metadata) = &self.target_metadata { + // Exact mode with explicit metadata: use it directly + explicit_metadata.clone() + } else { + // Exact mode but no explicit metadata: pass through from source + // (stripping extension keys) + let mut meta = source_field.metadata().clone(); + meta.remove(EXTENSION_TYPE_NAME_KEY); + meta.remove(EXTENSION_TYPE_METADATA_KEY); + meta + }; + + // Determine nullability: explicit or from source + let nullable = self + .target_nullable + .unwrap_or_else(|| source_field.is_nullable()); + + Arc::new( + source_field + .as_ref() + .clone() + .with_data_type(self.target_type.clone()) + .with_nullable(nullable) + .with_metadata(metadata), + ) }) } @@ -265,10 +334,23 @@ impl CastExpr { } } -fn is_default_target_field(target_field: &FieldRef) -> bool { - target_field.name().is_empty() - && target_field.is_nullable() - && target_field.metadata().is_empty() +/// Extract only the extension type metadata keys from a metadata map. +/// Returns `None` if no extension metadata is present (indicating pass-through). +fn extract_extension_metadata( + metadata: &HashMap, +) -> Option> { + let mut ext_meta = HashMap::new(); + if let Some(name) = metadata.get(EXTENSION_TYPE_NAME_KEY) { + ext_meta.insert(EXTENSION_TYPE_NAME_KEY.to_string(), name.clone()); + } + if let Some(meta) = metadata.get(EXTENSION_TYPE_METADATA_KEY) { + ext_meta.insert(EXTENSION_TYPE_METADATA_KEY.to_string(), meta.clone()); + } + if ext_meta.is_empty() { + None + } else { + Some(ext_meta) + } } pub(crate) fn is_order_preserving_cast_family( @@ -333,9 +415,11 @@ impl PhysicalExpr for CastExpr { ) -> Result> { Ok(Arc::new(CastExpr { expr: Arc::clone(&children[0]), - target_field: Arc::clone(&self.target_field), + target_type: self.target_type.clone(), + target_metadata: self.target_metadata.clone(), + target_nullable: self.target_nullable, + merge_source_metadata: self.merge_source_metadata, cast_options: self.cast_options.clone(), - preserve_source_metadata: self.preserve_source_metadata, })) } @@ -437,31 +521,50 @@ pub fn cast_with_options( cast_type: DataType, cast_options: Option>, ) -> Result> { - cast_with_target_field( - expr, - input_schema, - cast_type.into_nullable_field_ref(), - cast_options, - ) + let expr_type = expr.data_type(input_schema)?; + + // If the types match, no cast is needed for a type-only cast + if expr_type == cast_type { + return Ok(Arc::clone(&expr)); + } + + let can_build_cast = if requires_nested_struct_cast(&expr_type, &cast_type) { + can_cast_named_struct_types(&expr_type, &cast_type) + } else { + can_cast_types(&expr_type, &cast_type) + }; + + if !can_build_cast { + return not_impl_err!("Unsupported CAST from {expr_type} to {cast_type}"); + } + + Ok(Arc::new(CastExpr::new(expr, cast_type, cast_options))) } /// Return a PhysicalExpression representing `expr` casted to `target_field`, /// preserving any explicit field semantics such as name, nullability, and /// metadata. /// -/// If the input expression already has the same data type, this helper still -/// preserves an explicit `target_field` by constructing a field-aware -/// [`CastExpr`]. Only the default synthesized field created by the legacy -/// type-only API is elided back to the original child expression. +/// If the input expression already has the same data type and the target field +/// has no explicit metadata or nullability constraints, the original expression +/// is returned unchanged. pub fn cast_with_target_field( expr: Arc, input_schema: &Schema, - target_field: FieldRef, + target_field: &FieldRef, cast_options: Option>, ) -> Result> { let expr_type = expr.data_type(input_schema)?; let cast_type = target_field.data_type(); - if expr_type == *cast_type && is_default_target_field(&target_field) { + + // Check if this is a "default" target field (type-only cast with no explicit + // metadata or nullability constraints). This is the field created by + // `into_nullable_field_ref()` when only a DataType is known. + let is_type_only = target_field.name().is_empty() + && target_field.is_nullable() + && target_field.metadata().is_empty(); + + if expr_type == *cast_type && is_type_only { return Ok(Arc::clone(&expr)); } @@ -480,11 +583,22 @@ pub fn cast_with_target_field( return not_impl_err!("Unsupported CAST from {expr_type} to {cast_type}"); } - Ok(Arc::new(CastExpr::new_with_target_field( - expr, - target_field, - cast_options, - ))) + // For type-only casts, use CastExpr::new which preserves source metadata/nullability. + // For explicit target fields, use new_with_target_field which applies the target's + // extension metadata and nullability. + if is_type_only { + Ok(Arc::new(CastExpr::new( + expr, + cast_type.clone(), + cast_options, + ))) + } else { + Ok(Arc::new(CastExpr::new_with_target_field( + expr, + target_field, + cast_options, + ))) + } } /// Return a PhysicalExpression representing `expr` casted to @@ -537,9 +651,10 @@ mod tests { Arc::clone(&schema), vec![Arc::new(input_array) as ArrayRef], )?; + let target_field = Arc::new(target_field); let expr = CastExpr::new_with_target_field( col(column, schema.as_ref())?, - Arc::new(target_field), + &target_field, None, ); @@ -1055,26 +1170,32 @@ mod tests { #[test] fn field_aware_cast_preserves_target_field_semantics() -> Result<()> { + // Non-extension metadata from target field should NOT be preserved. + // Only extension metadata from target is used. let metadata = HashMap::from([("target_meta".to_string(), "1".to_string())]); for (child_nullable, target_nullable) in [(true, false), (false, true)] { let schema = Schema::new(vec![Field::new("a", Int32, child_nullable)]); + let target_field = Arc::new( + Field::new("cast_target", Int64, target_nullable) + .with_metadata(metadata.clone()), + ); let expr = CastExpr::new_with_target_field( col("a", &schema)?, - Arc::new( - Field::new("cast_target", Int64, target_nullable) - .with_metadata(metadata.clone()), - ), + &target_field, None, ); let field = expr.return_field(&schema)?; - assert_eq!(field.name(), "cast_target"); + // Field name comes from source + assert_eq!(field.name(), "a"); assert_eq!(field.data_type(), &Int64); + // Nullability comes from target assert_eq!(field.is_nullable(), target_nullable); - assert_eq!( - field.metadata().get("target_meta").map(String::as_str), - Some("1") + // Non-extension metadata from target should NOT propagate + assert!( + field.metadata().get("target_meta").is_none(), + "Non-extension metadata from target should not propagate" ); assert_eq!(expr.nullable(&schema)?, child_nullable || target_nullable); } @@ -1235,7 +1356,8 @@ mod tests { let literal = Arc::new(crate::expressions::Literal::new(ScalarValue::Struct( Arc::new(scalar_struct), ))); - let expr = CastExpr::new_with_target_field(literal, Arc::new(target_field), None); + let target_field = Arc::new(target_field); + let expr = CastExpr::new_with_target_field(literal, &target_field, None); let batch = RecordBatch::new_empty(schema); let result = expr.evaluate(&batch)?; @@ -1339,7 +1461,7 @@ mod tests { let target_field = Arc::new(Field::new("b", Utf8, true).with_metadata(target_meta)); let expr = - CastExpr::new_with_target_field(col("a", &schema)?, target_field, None); + CastExpr::new_with_target_field(col("a", &schema)?, &target_field, None); let field = expr.return_field(&schema)?; assert_eq!( diff --git a/datafusion/physical-expr/src/planner.rs b/datafusion/physical-expr/src/planner.rs index 4a9b593ffd3f0..2b91565a529b4 100644 --- a/datafusion/physical-expr/src/planner.rs +++ b/datafusion/physical-expr/src/planner.rs @@ -297,7 +297,7 @@ pub fn create_physical_expr( Expr::Cast(Cast { expr, field }) => expressions::cast_with_target_field( create_physical_expr(expr, input_dfschema, execution_props)?, input_schema, - Arc::clone(field), + field, None, ), Expr::TryCast(TryCast { expr, field }) => { @@ -670,8 +670,18 @@ mod tests { let physical = lower_cast_expr(&cast_expr, &schema)?; let cast = as_planner_cast(&physical); - // The CastExpr stores the target field as given - assert_eq!(cast.target_field(), &target_field); + // The CastExpr stores the target type and extension metadata + assert_eq!(cast.cast_type(), &DataType::Int64); + let target_metadata = cast.target_metadata().expect("should have metadata"); + assert_eq!( + target_metadata.get(EXTENSION_TYPE_NAME_KEY), + Some(&"arrow.json".to_string()) + ); + assert_eq!( + target_metadata.get(EXTENSION_TYPE_METADATA_KEY), + Some(&"{}".to_string()) + ); + assert_eq!(cast.target_nullable(), Some(true)); // But return_field should only propagate extension metadata from target, // aligning with logical-layer cast_output_field semantics @@ -737,7 +747,14 @@ mod tests { let physical = lower_cast_expr(&cast_expr, &schema)?; let cast = as_planner_cast(&physical); - assert_eq!(cast.target_field(), &target_field); + // The CastExpr stores the target type and extension metadata + assert_eq!(cast.cast_type(), &DataType::Int32); + let target_metadata = cast.target_metadata().expect("should have metadata"); + assert_eq!( + target_metadata.get(EXTENSION_TYPE_NAME_KEY), + Some(&"arrow.opaque".to_string()) + ); + assert_eq!(cast.target_nullable(), Some(true)); // return_field should only have extension metadata from target let returned = physical.return_field(&schema)?; diff --git a/datafusion/physical-plan/src/common.rs b/datafusion/physical-plan/src/common.rs index 0dafcf6bd3390..073ce72f651f7 100644 --- a/datafusion/physical-plan/src/common.rs +++ b/datafusion/physical-plan/src/common.rs @@ -162,7 +162,7 @@ pub fn project_plan_to_schema( let expr = if !input_field.is_nullable() && expected_field.is_nullable() { Arc::new(CastExpr::new_with_target_field( column, - Arc::clone(expected_field), + expected_field, None, )) as _ } else { diff --git a/datafusion/pruning/src/pruning_predicate.rs b/datafusion/pruning/src/pruning_predicate.rs index bacdd7032ead2..c97c7ef0f0097 100644 --- a/datafusion/pruning/src/pruning_predicate.rs +++ b/datafusion/pruning/src/pruning_predicate.rs @@ -1124,9 +1124,10 @@ fn rewrite_expr_to_prunable( scalar_expr, schema, )?; + let target_field = cast.target_field(); let left = Arc::new(phys_expr::CastExpr::new_with_target_field( left, - Arc::clone(cast.target_field()), + &target_field, None, )); // PruningPredicate does not support pruning on nested fields yet. From 83ae9eb0ee0ed63faa71dc62855a00bb792260b9 Mon Sep 17 00:00:00 2001 From: Dewey Dunnington Date: Wed, 1 Jul 2026 14:58:59 -0500 Subject: [PATCH 05/19] maybe cleaner --- .../physical-expr-adapter/src/rewrite.rs | 11 +- .../physical-expr/src/expressions/cast.rs | 156 +++++------------- datafusion/physical-expr/src/planner.rs | 35 ++-- 3 files changed, 68 insertions(+), 134 deletions(-) diff --git a/datafusion/physical-expr-adapter/src/rewrite.rs b/datafusion/physical-expr-adapter/src/rewrite.rs index ea9416a1ea01b..fdc4d2d012e59 100644 --- a/datafusion/physical-expr-adapter/src/rewrite.rs +++ b/datafusion/physical-expr-adapter/src/rewrite.rs @@ -102,7 +102,7 @@ pub fn rewrite_file_row_index_expr( rewrite_scalar_udf::(expr, |_| { let source = Arc::new(Column::new(row_index_name, row_index_idx)); let target_field = Arc::new(Field::new("file_row_index", DataType::Int64, true)); - Ok(Arc::new(CastExpr::new_with_exact_target_field( + Ok(Arc::new(CastExpr::new_with_target_field( source, &target_field, None, @@ -306,8 +306,9 @@ mod tests { .downcast_ref::() .expect("file row index expression should be a cast"); assert_eq!(cast_expr.cast_type(), &DataType::Int64); + // Note: target_field() does not preserve the name since CastExpr only stores + // type, metadata, and nullability. The name comes from the source expression. let target_field = cast_expr.target_field(); - assert_eq!(target_field.name(), "file_row_index"); assert_eq!(target_field.data_type(), &DataType::Int64); assert!(target_field.is_nullable()); assert!(target_field.metadata().is_empty()); @@ -320,7 +321,8 @@ mod tests { assert_eq!(source.index(), 2); // The row index column is at index 2, beyond the user-visible schema. - // With exact target field mode, return_field does not consult the source. + // When the source column lookup fails, the field name is empty. The + // correct field name would be provided by a parent projection/alias. let input_schema = Schema::new(vec![ Field::new("value", DataType::Int64, true), Field::new("__datafusion_file_row_index", DataType::Int64, false) @@ -330,7 +332,8 @@ mod tests { )])), ]); let return_field = expr.return_field(&input_schema)?; - assert_eq!(return_field.name(), "file_row_index"); + // Field name is empty because column index 2 is beyond the schema + assert_eq!(return_field.name(), ""); assert_eq!(return_field.data_type(), &DataType::Int64); assert!(return_field.is_nullable()); // Exact target field does not preserve source metadata diff --git a/datafusion/physical-expr/src/expressions/cast.rs b/datafusion/physical-expr/src/expressions/cast.rs index 3ee6474ef6199..ece13290ea0df 100644 --- a/datafusion/physical-expr/src/expressions/cast.rs +++ b/datafusion/physical-expr/src/expressions/cast.rs @@ -65,16 +65,11 @@ pub struct CastExpr { /// Explicit metadata for the output field, or `None` to pass through source metadata /// (with extension type keys stripped). /// - /// When `merge_source_metadata` is `true`, this contains extension type metadata - /// to overlay on top of source's non-extension metadata. - /// When `merge_source_metadata` is `false`, this is the exact metadata to use. + /// When `Some`, this is the exact metadata to use for the output field. + /// When `None`, source metadata passes through (stripping extension keys). target_metadata: Option>, /// Explicit nullability for the output field, or `None` to pass through source nullability. target_nullable: Option, - /// When `true` and `target_metadata` is `Some`, merge source's non-extension metadata - /// with the target's extension metadata. When `false`, use `target_metadata` exactly - /// without consulting the source. - merge_source_metadata: bool, /// Cast options cast_options: CastOptions<'static>, } @@ -86,7 +81,6 @@ impl PartialEq for CastExpr { && self.target_type.eq(&other.target_type) && self.target_metadata.eq(&other.target_metadata) && self.target_nullable.eq(&other.target_nullable) - && self.merge_source_metadata.eq(&other.merge_source_metadata) && self.cast_options.eq(&other.cast_options) } } @@ -105,7 +99,6 @@ impl Hash for CastExpr { } } self.target_nullable.hash(state); - self.merge_source_metadata.hash(state); self.cast_options.hash(state); } } @@ -130,7 +123,6 @@ impl CastExpr { target_type: cast_type, target_metadata: None, target_nullable: None, - merge_source_metadata: true, cast_options: cast_options.unwrap_or(DEFAULT_CAST_OPTIONS), } } @@ -139,54 +131,25 @@ impl CastExpr { /// /// The provided `target_field` determines the output characteristics: /// - The field's data type becomes the cast target type - /// - The field's metadata is used to extract extension type information, - /// which is then merged with source metadata (pass-through behavior) + /// - The field's metadata is used exactly as provided /// - The field's nullability is preserved /// - /// This is the preferred constructor when the caller already has field - /// information (for example, during logical-to-physical planning) but still - /// wants source metadata to pass through. + /// This is the preferred constructor when the caller has explicit field + /// information that should be used exactly (for example, during schema + /// enforcement or adapter layers). /// - /// See [`CastExpr::new`] for type-only casts and - /// [`CastExpr::new_with_exact_target_field`] for exact field matching. + /// See [`CastExpr::new`] for type-only casts where source metadata should + /// pass through. pub fn new_with_target_field( expr: Arc, target_field: &FieldRef, cast_options: Option>, - ) -> Self { - // Extract only extension type metadata from the target field to merge - // with source metadata (pass-through behavior with extension type overlay) - let extension_metadata = extract_extension_metadata(target_field.metadata()); - Self { - expr, - target_type: target_field.data_type().clone(), - target_metadata: extension_metadata, - target_nullable: Some(target_field.is_nullable()), - merge_source_metadata: true, - cast_options: cast_options.unwrap_or(DEFAULT_CAST_OPTIONS), - } - } - - /// Create a new `CastExpr` with an explicit target `FieldRef`, using the - /// target field exactly without merging source metadata. - /// - /// Unlike [`CastExpr::new_with_target_field`], this constructor does NOT - /// merge non-extension metadata from the source field. The target field's - /// metadata is used exactly as provided. This is useful when the cast - /// represents a complete type transformation where source metadata should - /// not propagate (e.g., rewriting placeholder functions like - /// `file_row_index()` to concrete column references). - pub fn new_with_exact_target_field( - expr: Arc, - target_field: &FieldRef, - cast_options: Option>, ) -> Self { Self { expr, target_type: target_field.data_type().clone(), target_metadata: Some(target_field.metadata().clone()), target_nullable: Some(target_field.is_nullable()), - merge_source_metadata: false, cast_options: cast_options.unwrap_or(DEFAULT_CAST_OPTIONS), } } @@ -247,42 +210,32 @@ impl CastExpr { } fn resolved_target_field(&self, input_schema: &Schema) -> Result { - // If we're not merging with source AND both metadata and nullability are - // explicit, we can build the field without consulting the source expression. - // This is important for cases where the source references columns beyond - // the schema (e.g., virtual row-index columns appended at scan time). - if !self.merge_source_metadata - && let (Some(metadata), Some(nullable)) = - (&self.target_metadata, self.target_nullable) + // Try to get the source field for the name. If both metadata and nullability + // are explicit, we can fall back to an empty name if the source lookup fails + // (e.g., for virtual row-index columns appended at scan time). + let source_result = self.expr.return_field(input_schema); + + if let (Some(metadata), Some(nullable)) = + (&self.target_metadata, self.target_nullable) { + // Both metadata and nullability are explicit + let name = source_result + .as_ref() + .map(|f| f.name().to_string()) + .unwrap_or_default(); return Ok(Arc::new( - Field::new("", self.target_type.clone(), nullable) + Field::new(name, self.target_type.clone(), nullable) .with_metadata(metadata.clone()), )); } - self.expr.return_field(input_schema).map(|source_field| { - // Build metadata based on merge_source_metadata flag - let metadata = if self.merge_source_metadata { - // Merge mode: start with source non-extension metadata - let mut meta = source_field.metadata().clone(); - meta.remove(EXTENSION_TYPE_NAME_KEY); - meta.remove(EXTENSION_TYPE_METADATA_KEY); - // Then overlay any explicit metadata (extension keys from target) - if let Some(explicit_metadata) = &self.target_metadata { - meta.extend( - explicit_metadata - .iter() - .map(|(k, v)| (k.clone(), v.clone())), - ); - } - meta - } else if let Some(explicit_metadata) = &self.target_metadata { - // Exact mode with explicit metadata: use it directly + // Need source field for metadata or nullability + source_result.map(|source_field| { + // Build metadata: use explicit if provided, otherwise pass through + // from source (stripping extension keys) + let metadata = if let Some(explicit_metadata) = &self.target_metadata { explicit_metadata.clone() } else { - // Exact mode but no explicit metadata: pass through from source - // (stripping extension keys) let mut meta = source_field.metadata().clone(); meta.remove(EXTENSION_TYPE_NAME_KEY); meta.remove(EXTENSION_TYPE_METADATA_KEY); @@ -334,25 +287,6 @@ impl CastExpr { } } -/// Extract only the extension type metadata keys from a metadata map. -/// Returns `None` if no extension metadata is present (indicating pass-through). -fn extract_extension_metadata( - metadata: &HashMap, -) -> Option> { - let mut ext_meta = HashMap::new(); - if let Some(name) = metadata.get(EXTENSION_TYPE_NAME_KEY) { - ext_meta.insert(EXTENSION_TYPE_NAME_KEY.to_string(), name.clone()); - } - if let Some(meta) = metadata.get(EXTENSION_TYPE_METADATA_KEY) { - ext_meta.insert(EXTENSION_TYPE_METADATA_KEY.to_string(), meta.clone()); - } - if ext_meta.is_empty() { - None - } else { - Some(ext_meta) - } -} - pub(crate) fn is_order_preserving_cast_family( source_type: &DataType, target_type: &DataType, @@ -418,7 +352,6 @@ impl PhysicalExpr for CastExpr { target_type: self.target_type.clone(), target_metadata: self.target_metadata.clone(), target_nullable: self.target_nullable, - merge_source_metadata: self.merge_source_metadata, cast_options: self.cast_options.clone(), })) } @@ -1170,8 +1103,7 @@ mod tests { #[test] fn field_aware_cast_preserves_target_field_semantics() -> Result<()> { - // Non-extension metadata from target field should NOT be preserved. - // Only extension metadata from target is used. + // Target field metadata should be preserved exactly (no merging with source). let metadata = HashMap::from([("target_meta".to_string(), "1".to_string())]); for (child_nullable, target_nullable) in [(true, false), (false, true)] { @@ -1180,11 +1112,8 @@ mod tests { Field::new("cast_target", Int64, target_nullable) .with_metadata(metadata.clone()), ); - let expr = CastExpr::new_with_target_field( - col("a", &schema)?, - &target_field, - None, - ); + let expr = + CastExpr::new_with_target_field(col("a", &schema)?, &target_field, None); let field = expr.return_field(&schema)?; // Field name comes from source @@ -1192,10 +1121,11 @@ mod tests { assert_eq!(field.data_type(), &Int64); // Nullability comes from target assert_eq!(field.is_nullable(), target_nullable); - // Non-extension metadata from target should NOT propagate - assert!( - field.metadata().get("target_meta").is_none(), - "Non-extension metadata from target should not propagate" + // Target metadata should be preserved exactly + assert_eq!( + field.metadata().get("target_meta"), + Some(&"1".to_string()), + "Target metadata should be preserved exactly" ); assert_eq!(expr.nullable(&schema)?, child_nullable || target_nullable); } @@ -1434,8 +1364,8 @@ mod tests { } #[test] - fn field_aware_cast_applies_target_extension_metadata() -> Result<()> { - // When using field-aware cast, target's extension metadata should be applied + fn field_aware_cast_uses_exact_target_metadata() -> Result<()> { + // When using field-aware cast, target's metadata should be used exactly let source_meta = HashMap::from([ ( EXTENSION_TYPE_NAME_KEY.to_string(), @@ -1474,14 +1404,14 @@ mod tests { Some(&"target_ext_meta".to_string()), "Field-aware cast should use target's extension type metadata" ); - assert_eq!( - field.metadata().get("source_key"), - Some(&"source_value".to_string()), - "Field-aware cast should preserve source's non-extension metadata" - ); assert!( - field.metadata().get("target_key").is_none(), - "Field-aware cast should not preserve target's non-extension metadata" + field.metadata().get("source_key").is_none(), + "Field-aware cast should NOT preserve source metadata" + ); + assert_eq!( + field.metadata().get("target_key"), + Some(&"target_value".to_string()), + "Field-aware cast should preserve target's non-extension metadata" ); Ok(()) diff --git a/datafusion/physical-expr/src/planner.rs b/datafusion/physical-expr/src/planner.rs index 2b91565a529b4..969032acc6633 100644 --- a/datafusion/physical-expr/src/planner.rs +++ b/datafusion/physical-expr/src/planner.rs @@ -648,7 +648,7 @@ mod tests { let schema = test_cast_schema(); // Target field with both extension metadata and custom metadata. - // Per cast_output_field semantics, only extension metadata should propagate. + // With exact target metadata semantics, all target metadata should propagate. let target_field = Arc::new( Field::new("cast_target", DataType::Int64, true).with_metadata( [ @@ -657,7 +657,7 @@ mod tests { "arrow.json".to_string(), ), (EXTENSION_TYPE_METADATA_KEY.to_string(), "{}".to_string()), - ("custom_target_meta".to_string(), "ignored".to_string()), + ("custom_target_meta".to_string(), "custom_value".to_string()), ] .into(), ), @@ -670,7 +670,7 @@ mod tests { let physical = lower_cast_expr(&cast_expr, &schema)?; let cast = as_planner_cast(&physical); - // The CastExpr stores the target type and extension metadata + // The CastExpr stores the target type and all target metadata assert_eq!(cast.cast_type(), &DataType::Int64); let target_metadata = cast.target_metadata().expect("should have metadata"); assert_eq!( @@ -683,8 +683,7 @@ mod tests { ); assert_eq!(cast.target_nullable(), Some(true)); - // But return_field should only propagate extension metadata from target, - // aligning with logical-layer cast_output_field semantics + // return_field should have all target metadata (exact semantics) let returned = physical.return_field(&schema)?; assert_eq!( returned.metadata().get(EXTENSION_TYPE_NAME_KEY), @@ -694,10 +693,11 @@ mod tests { returned.metadata().get(EXTENSION_TYPE_METADATA_KEY), Some(&"{}".to_string()) ); - // Custom target metadata should NOT propagate - assert!( - returned.metadata().get("custom_target_meta").is_none(), - "Non-extension target metadata should not propagate" + // All target metadata should propagate with exact semantics + assert_eq!( + returned.metadata().get("custom_target_meta"), + Some(&"custom_value".to_string()), + "All target metadata should propagate with exact semantics" ); assert!(physical.nullable(&schema)?); @@ -726,7 +726,7 @@ mod tests { let schema = test_cast_schema(); // Same-type cast with extension metadata on target. - // Per cast_output_field semantics, only extension metadata should propagate. + // With exact target metadata semantics, all target metadata should propagate. let target_field = Arc::new( Field::new("same_type_cast", DataType::Int32, true).with_metadata( [ @@ -734,7 +734,7 @@ mod tests { EXTENSION_TYPE_NAME_KEY.to_string(), "arrow.opaque".to_string(), ), - ("custom_meta".to_string(), "ignored".to_string()), + ("custom_meta".to_string(), "custom_value".to_string()), ] .into(), ), @@ -747,7 +747,7 @@ mod tests { let physical = lower_cast_expr(&cast_expr, &schema)?; let cast = as_planner_cast(&physical); - // The CastExpr stores the target type and extension metadata + // The CastExpr stores the target type and all target metadata assert_eq!(cast.cast_type(), &DataType::Int32); let target_metadata = cast.target_metadata().expect("should have metadata"); assert_eq!( @@ -756,16 +756,17 @@ mod tests { ); assert_eq!(cast.target_nullable(), Some(true)); - // return_field should only have extension metadata from target + // return_field should have all target metadata (exact semantics) let returned = physical.return_field(&schema)?; assert_eq!( returned.metadata().get(EXTENSION_TYPE_NAME_KEY), Some(&"arrow.opaque".to_string()) ); - // Custom target metadata should NOT propagate - assert!( - returned.metadata().get("custom_meta").is_none(), - "Non-extension target metadata should not propagate" + // All target metadata should propagate with exact semantics + assert_eq!( + returned.metadata().get("custom_meta"), + Some(&"custom_value".to_string()), + "All target metadata should propagate with exact semantics" ); assert!(physical.nullable(&schema)?); From 6b076ecd146c18f838a10eb8611d996fe43d4a96 Mon Sep 17 00:00:00 2001 From: Dewey Dunnington Date: Wed, 1 Jul 2026 15:05:43 -0500 Subject: [PATCH 06/19] minimize the diff --- datafusion/physical-expr-adapter/src/schema_rewriter.rs | 3 +-- datafusion/physical-expr/src/expressions/cast.rs | 3 +-- datafusion/physical-expr/src/planner.rs | 1 + datafusion/pruning/src/pruning_predicate.rs | 3 +-- 4 files changed, 4 insertions(+), 6 deletions(-) diff --git a/datafusion/physical-expr-adapter/src/schema_rewriter.rs b/datafusion/physical-expr-adapter/src/schema_rewriter.rs index 5452e1ceba561..90e092a1c6b65 100644 --- a/datafusion/physical-expr-adapter/src/schema_rewriter.rs +++ b/datafusion/physical-expr-adapter/src/schema_rewriter.rs @@ -447,10 +447,9 @@ impl DefaultPhysicalExprAdapterRewriter { )) })?; - let target_field = Arc::new(logical_field.clone()); Ok(Transformed::yes(Arc::new(CastExpr::new_with_target_field( Arc::new(resolved_column), - &target_field, + &Arc::new(logical_field.clone()), None, )))) } diff --git a/datafusion/physical-expr/src/expressions/cast.rs b/datafusion/physical-expr/src/expressions/cast.rs index ece13290ea0df..d0180adde9b6a 100644 --- a/datafusion/physical-expr/src/expressions/cast.rs +++ b/datafusion/physical-expr/src/expressions/cast.rs @@ -584,10 +584,9 @@ mod tests { Arc::clone(&schema), vec![Arc::new(input_array) as ArrayRef], )?; - let target_field = Arc::new(target_field); let expr = CastExpr::new_with_target_field( col(column, schema.as_ref())?, - &target_field, + &Arc::new(target_field), None, ); diff --git a/datafusion/physical-expr/src/planner.rs b/datafusion/physical-expr/src/planner.rs index 969032acc6633..209fa8d5f5200 100644 --- a/datafusion/physical-expr/src/planner.rs +++ b/datafusion/physical-expr/src/planner.rs @@ -294,6 +294,7 @@ pub fn create_physical_expr( }; Ok(expressions::case(expr, when_then_expr, else_expr)?) } + // TODO I think we need to check for the "default target field" here Expr::Cast(Cast { expr, field }) => expressions::cast_with_target_field( create_physical_expr(expr, input_dfschema, execution_props)?, input_schema, diff --git a/datafusion/pruning/src/pruning_predicate.rs b/datafusion/pruning/src/pruning_predicate.rs index c97c7ef0f0097..779544e0bb2d0 100644 --- a/datafusion/pruning/src/pruning_predicate.rs +++ b/datafusion/pruning/src/pruning_predicate.rs @@ -1124,10 +1124,9 @@ fn rewrite_expr_to_prunable( scalar_expr, schema, )?; - let target_field = cast.target_field(); let left = Arc::new(phys_expr::CastExpr::new_with_target_field( left, - &target_field, + &cast.target_field(), None, )); // PruningPredicate does not support pruning on nested fields yet. From 7d9525d4ad4a77b0d16b19e353ac022df1426431 Mon Sep 17 00:00:00 2001 From: Dewey Dunnington Date: Wed, 1 Jul 2026 15:24:29 -0500 Subject: [PATCH 07/19] align logical cast rules --- datafusion/expr/src/expr_schema.rs | 52 +++++++++++++------------ datafusion/physical-expr/src/planner.rs | 1 - 2 files changed, 28 insertions(+), 25 deletions(-) diff --git a/datafusion/expr/src/expr_schema.rs b/datafusion/expr/src/expr_schema.rs index 3305b54c390a6..4753d7ec42916 100644 --- a/datafusion/expr/src/expr_schema.rs +++ b/datafusion/expr/src/expr_schema.rs @@ -72,10 +72,15 @@ pub trait ExprSchemable { /// Derives the output field for a cast expression from the source and target fields. /// /// Metadata handling: -/// - Source field metadata is propagated by default -/// - Extension type metadata keys (`ARROW:extension:name` and `ARROW:extension:metadata`) -/// are taken from the target field, overwriting any extension metadata from the source. -/// This ensures casting does not incorrectly propagate extension type identity. +/// - Type-only casts (i.e., target_field == DataType::SomeDataType.into_nullable_field()) +/// propagate non extension-type metadata from the source. This is for backward compatibility +/// (casts have propagated source metadata for many if not all previous versions), recognizing +/// that the return type of :: should have the +/// return type of (e.g., casting arrow.json to utf8). +/// - All other casts preserve target metdata exactly. This ensures in particular that output +/// metadata when casting to an extension type contains the extension information in the +/// output field. Callers that wish to have some mix of source and target metadata can use +/// Alias or construct an output field themselves (whose metadata will be used directly). /// /// For `TryCast`, `force_nullable` is `true` since a failed cast returns NULL. fn cast_output_field( @@ -85,21 +90,21 @@ fn cast_output_field( ) -> Arc { use arrow_schema::extension::{EXTENSION_TYPE_METADATA_KEY, EXTENSION_TYPE_NAME_KEY}; - // Start with source metadata - let mut metadata = source_field.metadata().clone(); - - // Remove any extension type metadata from source - these should not propagate through casts - metadata.remove(EXTENSION_TYPE_NAME_KEY); - metadata.remove(EXTENSION_TYPE_METADATA_KEY); - - // Add extension type metadata from the target field if present - let target_metadata = target_field.metadata(); - if let Some(name) = target_metadata.get(EXTENSION_TYPE_NAME_KEY) { - metadata.insert(EXTENSION_TYPE_NAME_KEY.to_string(), name.clone()); - } - if let Some(ext_meta) = target_metadata.get(EXTENSION_TYPE_METADATA_KEY) { - metadata.insert(EXTENSION_TYPE_METADATA_KEY.to_string(), ext_meta.clone()); - } + // Check if this is a "type-only" cast (target_field == DataType::X.into_nullable_field()) + let is_type_only = target_field.name().is_empty() + && target_field.is_nullable() + && target_field.metadata().is_empty(); + + let metadata = if is_type_only { + // Type-only cast: propagate source metadata, stripping extension type keys + let mut meta = source_field.metadata().clone(); + meta.remove(EXTENSION_TYPE_NAME_KEY); + meta.remove(EXTENSION_TYPE_METADATA_KEY); + meta + } else { + // Explicit target field: use target metadata exactly + target_field.metadata().clone() + }; let mut f = source_field .as_ref() @@ -1303,7 +1308,7 @@ mod tests { ); } - // Test 2: Cast to a field with different extension type replaces extension metadata + // Test 2: Cast to a field with explicit metadata uses target metadata exactly let mut target_meta = HashMap::new(); target_meta.insert( EXTENSION_TYPE_NAME_KEY.to_string(), @@ -1328,10 +1333,9 @@ mod tests { Some(&"{}".to_string()), "{cast_name}: Extension type metadata should come from target field" ); - assert_eq!( - result_field.metadata().get("custom_key"), - Some(&"custom_value".to_string()), - "{cast_name}: Non-extension metadata should still be preserved from source" + assert!( + result_field.metadata().get("custom_key").is_none(), + "{cast_name}: Source metadata should NOT propagate when target has explicit metadata" ); if use_try_cast { assert!( diff --git a/datafusion/physical-expr/src/planner.rs b/datafusion/physical-expr/src/planner.rs index 209fa8d5f5200..969032acc6633 100644 --- a/datafusion/physical-expr/src/planner.rs +++ b/datafusion/physical-expr/src/planner.rs @@ -294,7 +294,6 @@ pub fn create_physical_expr( }; Ok(expressions::case(expr, when_then_expr, else_expr)?) } - // TODO I think we need to check for the "default target field" here Expr::Cast(Cast { expr, field }) => expressions::cast_with_target_field( create_physical_expr(expr, input_dfschema, execution_props)?, input_schema, From 5e202a7a794d4bcd035d02f9cd54e185a814d7a4 Mon Sep 17 00:00:00 2001 From: Dewey Dunnington Date: Wed, 1 Jul 2026 15:27:51 -0500 Subject: [PATCH 08/19] typo --- datafusion/expr/src/expr_schema.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datafusion/expr/src/expr_schema.rs b/datafusion/expr/src/expr_schema.rs index 4753d7ec42916..c61c104165540 100644 --- a/datafusion/expr/src/expr_schema.rs +++ b/datafusion/expr/src/expr_schema.rs @@ -77,7 +77,7 @@ pub trait ExprSchemable { /// (casts have propagated source metadata for many if not all previous versions), recognizing /// that the return type of :: should have the /// return type of (e.g., casting arrow.json to utf8). -/// - All other casts preserve target metdata exactly. This ensures in particular that output +/// - All other casts preserve target meatdata exactly. This ensures in particular that output /// metadata when casting to an extension type contains the extension information in the /// output field. Callers that wish to have some mix of source and target metadata can use /// Alias or construct an output field themselves (whose metadata will be used directly). From b8ddb433da61a8a0457b707d038b2fa713dda2f7 Mon Sep 17 00:00:00 2001 From: Dewey Dunnington Date: Wed, 1 Jul 2026 15:29:00 -0500 Subject: [PATCH 09/19] another typo --- datafusion/expr/src/expr_schema.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datafusion/expr/src/expr_schema.rs b/datafusion/expr/src/expr_schema.rs index c61c104165540..7e17952cce712 100644 --- a/datafusion/expr/src/expr_schema.rs +++ b/datafusion/expr/src/expr_schema.rs @@ -77,7 +77,7 @@ pub trait ExprSchemable { /// (casts have propagated source metadata for many if not all previous versions), recognizing /// that the return type of :: should have the /// return type of (e.g., casting arrow.json to utf8). -/// - All other casts preserve target meatdata exactly. This ensures in particular that output +/// - All other casts preserve target metadata exactly. This ensures in particular that output /// metadata when casting to an extension type contains the extension information in the /// output field. Callers that wish to have some mix of source and target metadata can use /// Alias or construct an output field themselves (whose metadata will be used directly). From 0eb421f1eb55b375098661494f1e196d1d484f2a Mon Sep 17 00:00:00 2001 From: Dewey Dunnington Date: Wed, 1 Jul 2026 15:40:39 -0500 Subject: [PATCH 10/19] implement try cast --- .../physical-expr/src/expressions/try_cast.rs | 116 ++++++++++++++++-- 1 file changed, 103 insertions(+), 13 deletions(-) diff --git a/datafusion/physical-expr/src/expressions/try_cast.rs b/datafusion/physical-expr/src/expressions/try_cast.rs index 65b953fd181b7..fe7c834ee499d 100644 --- a/datafusion/physical-expr/src/expressions/try_cast.rs +++ b/datafusion/physical-expr/src/expressions/try_cast.rs @@ -15,6 +15,7 @@ // specific language governing permissions and limitations // under the License. +use std::collections::HashMap; use std::fmt; use std::hash::Hash; use std::sync::Arc; @@ -22,26 +23,35 @@ use std::sync::Arc; use crate::PhysicalExpr; use arrow::compute; use arrow::compute::CastOptions; -use arrow::datatypes::{DataType, FieldRef, Schema}; +use arrow::datatypes::{DataType, Field, FieldRef, Schema}; use arrow::record_batch::RecordBatch; +use arrow_schema::extension::{EXTENSION_TYPE_METADATA_KEY, EXTENSION_TYPE_NAME_KEY}; use compute::can_cast_types; use datafusion_common::format::DEFAULT_FORMAT_OPTIONS; use datafusion_common::{Result, not_impl_err}; use datafusion_expr::ColumnarValue; /// TRY_CAST expression casts an expression to a specific data type and returns NULL on invalid cast -#[derive(Debug, Eq)] +#[derive(Debug, Clone, Eq)] pub struct TryCastExpr { /// The expression to cast expr: Arc, /// The data type to cast to cast_type: DataType, + /// Explicit metadata for the output field, or `None` to pass through source metadata + /// (with extension type keys stripped). + /// + /// When `Some`, this is the exact metadata to use for the output field. + /// When `None`, source metadata passes through (stripping extension keys). + target_metadata: Option>, } // Manually derive PartialEq and Hash to work around https://github.com/rust-lang/rust/issues/78808 impl PartialEq for TryCastExpr { fn eq(&self, other: &Self) -> bool { - self.expr.eq(&other.expr) && self.cast_type == other.cast_type + self.expr.eq(&other.expr) + && self.cast_type == other.cast_type + && self.target_metadata == other.target_metadata } } @@ -49,13 +59,51 @@ impl Hash for TryCastExpr { fn hash(&self, state: &mut H) { self.expr.hash(state); self.cast_type.hash(state); + // Hash the metadata by iterating over sorted keys for deterministic ordering + if let Some(metadata) = &self.target_metadata { + let mut entries: Vec<_> = metadata.iter().collect(); + entries.sort_by_key(|(k, _)| *k); + for (k, v) in entries { + k.hash(state); + v.hash(state); + } + } } } impl TryCastExpr { - /// Create a new CastExpr + /// Create a new `TryCastExpr` using only a `DataType`. + /// + /// This constructor creates a type-only cast where metadata is passed through + /// from the source expression (with extension type keys stripped). + /// TRY_CAST results are always nullable since failed casts return NULL. pub fn new(expr: Arc, cast_type: DataType) -> Self { - Self { expr, cast_type } + Self { + expr, + cast_type, + target_metadata: None, + } + } + + /// Create a new `TryCastExpr` with an explicit target `FieldRef`. + /// + /// The provided `target_field` determines the output characteristics: + /// - The field's data type becomes the cast target type + /// - The field's metadata is used exactly as provided + /// + /// TRY_CAST results are always nullable since failed casts return NULL. + /// + /// See [`TryCastExpr::new`] for type-only casts where source metadata should + /// pass through. + pub fn new_with_target_field( + expr: Arc, + target_field: &FieldRef, + ) -> Self { + Self { + expr, + cast_type: target_field.data_type().clone(), + target_metadata: Some(target_field.metadata().clone()), + } } /// The expression to cast @@ -67,6 +115,20 @@ impl TryCastExpr { pub fn cast_type(&self) -> &DataType { &self.cast_type } + + /// Explicit metadata for the output field, or `None` to pass through source metadata. + pub fn target_metadata(&self) -> Option<&HashMap> { + self.target_metadata.as_ref() + } + + /// Returns a computed target `FieldRef` for compatibility with existing callers. + /// + /// This synthesizes a field based on the current configuration. TRY_CAST + /// results are always nullable. + pub fn target_field(&self) -> FieldRef { + let metadata = self.target_metadata.clone().unwrap_or_default(); + Arc::new(Field::new("", self.cast_type.clone(), true).with_metadata(metadata)) + } } impl fmt::Display for TryCastExpr { @@ -94,10 +156,37 @@ impl PhysicalExpr for TryCastExpr { } fn return_field(&self, input_schema: &Schema) -> Result { - self.expr - .return_field(input_schema) - .map(|f| f.as_ref().clone().with_data_type(self.cast_type.clone())) - .map(Arc::new) + // If metadata is explicit, we can build the field without source + // (though we still try to get source for the name) + let source_result = self.expr.return_field(input_schema); + + if let Some(metadata) = &self.target_metadata { + // Explicit metadata: use it exactly, TRY_CAST is always nullable + let name = source_result + .as_ref() + .map(|f| f.name().to_string()) + .unwrap_or_default(); + return Ok(Arc::new( + Field::new(name, self.cast_type.clone(), true) + .with_metadata(metadata.clone()), + )); + } + + // Pass-through metadata from source (stripping extension keys) + source_result.map(|source_field| { + let mut metadata = source_field.metadata().clone(); + metadata.remove(EXTENSION_TYPE_NAME_KEY); + metadata.remove(EXTENSION_TYPE_METADATA_KEY); + + Arc::new( + source_field + .as_ref() + .clone() + .with_data_type(self.cast_type.clone()) + .with_nullable(true) // TRY_CAST is always nullable + .with_metadata(metadata), + ) + }) } fn children(&self) -> Vec<&Arc> { @@ -108,10 +197,11 @@ impl PhysicalExpr for TryCastExpr { self: Arc, children: Vec>, ) -> Result> { - Ok(Arc::new(TryCastExpr::new( - Arc::clone(&children[0]), - self.cast_type.clone(), - ))) + Ok(Arc::new(TryCastExpr { + expr: Arc::clone(&children[0]), + cast_type: self.cast_type.clone(), + target_metadata: self.target_metadata.clone(), + })) } fn fmt_sql(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { From bbeab9e073104c2eb282dc8656a4c0c11ce5a348 Mon Sep 17 00:00:00 2001 From: Dewey Dunnington Date: Wed, 1 Jul 2026 15:42:51 -0500 Subject: [PATCH 11/19] fix build --- .../examples/custom_data_source/custom_file_casts.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/datafusion-examples/examples/custom_data_source/custom_file_casts.rs b/datafusion-examples/examples/custom_data_source/custom_file_casts.rs index 71addc6d1bcb0..202c0a71257e9 100644 --- a/datafusion-examples/examples/custom_data_source/custom_file_casts.rs +++ b/datafusion-examples/examples/custom_data_source/custom_file_casts.rs @@ -188,10 +188,11 @@ impl PhysicalExprAdapter for CustomCastsPhysicalExprAdapter { if let Some(cast) = expr.downcast_ref::() { let input_data_type = cast.expr().data_type(&self.physical_file_schema)?; - let output_data_type = cast.target_field().data_type(); + let output_field = cast.target_field(); if !cast.is_bigger_cast(&input_data_type) { return not_impl_err!( - "Unsupported CAST from {input_data_type} to {output_data_type}" + "Unsupported CAST from {input_data_type} to {}", + output_field.data_type() ); } } From 2adb3cc1c716341b4231f7931a0080c91558a57f Mon Sep 17 00:00:00 2001 From: Dewey Dunnington Date: Wed, 1 Jul 2026 16:10:32 -0500 Subject: [PATCH 12/19] fix docs --- datafusion/expr/src/expr_schema.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/datafusion/expr/src/expr_schema.rs b/datafusion/expr/src/expr_schema.rs index 7e17952cce712..7a07321523414 100644 --- a/datafusion/expr/src/expr_schema.rs +++ b/datafusion/expr/src/expr_schema.rs @@ -75,8 +75,8 @@ pub trait ExprSchemable { /// - Type-only casts (i.e., target_field == DataType::SomeDataType.into_nullable_field()) /// propagate non extension-type metadata from the source. This is for backward compatibility /// (casts have propagated source metadata for many if not all previous versions), recognizing -/// that the return type of :: should have the -/// return type of (e.g., casting arrow.json to utf8). +/// that the return type of `::` should have the +/// return type of `` (e.g., casting arrow.json to utf8). /// - All other casts preserve target metadata exactly. This ensures in particular that output /// metadata when casting to an extension type contains the extension information in the /// output field. Callers that wish to have some mix of source and target metadata can use From 3aaa9ed118fbf94412b67475a229ecd52fefbaed Mon Sep 17 00:00:00 2001 From: Dewey Dunnington Date: Wed, 1 Jul 2026 21:35:36 -0500 Subject: [PATCH 13/19] ensure try_cast semantics are the same --- datafusion/expr/src/expr_schema.rs | 3 +- .../physical-expr/src/expressions/mod.rs | 2 +- .../physical-expr/src/expressions/try_cast.rs | 200 ++++++++++++++++++ datafusion/physical-expr/src/planner.rs | 116 ++++++---- 4 files changed, 275 insertions(+), 46 deletions(-) diff --git a/datafusion/expr/src/expr_schema.rs b/datafusion/expr/src/expr_schema.rs index 7a07321523414..a60e29427faec 100644 --- a/datafusion/expr/src/expr_schema.rs +++ b/datafusion/expr/src/expr_schema.rs @@ -31,6 +31,7 @@ use crate::{LogicalPlan, Projection, Subquery, WindowFunctionDefinition, utils}; use arrow::compute::can_cast_types; use arrow::datatypes::FieldRef; use arrow::datatypes::{DataType, Field}; +use arrow_schema::extension::{EXTENSION_TYPE_METADATA_KEY, EXTENSION_TYPE_NAME_KEY}; use datafusion_common::datatype::FieldExt; use datafusion_common::{ Column, DataFusionError, ExprSchema, Result, ScalarValue, Spans, TableReference, @@ -88,8 +89,6 @@ fn cast_output_field( target_field: &FieldRef, force_nullable: bool, ) -> Arc { - use arrow_schema::extension::{EXTENSION_TYPE_METADATA_KEY, EXTENSION_TYPE_NAME_KEY}; - // Check if this is a "type-only" cast (target_field == DataType::X.into_nullable_field()) let is_type_only = target_field.name().is_empty() && target_field.is_nullable() diff --git a/datafusion/physical-expr/src/expressions/mod.rs b/datafusion/physical-expr/src/expressions/mod.rs index 05a04f88dcadf..f97690f923395 100644 --- a/datafusion/physical-expr/src/expressions/mod.rs +++ b/datafusion/physical-expr/src/expressions/mod.rs @@ -59,7 +59,7 @@ pub use literal::{Literal, lit}; pub use negative::{NegativeExpr, negative}; pub use no_op::NoOp; pub use not::{NotExpr, not}; -pub use try_cast::{TryCastExpr, try_cast}; +pub use try_cast::{TryCastExpr, try_cast, try_cast_with_target_field}; pub use unknown_column::UnKnownColumn; pub(crate) use cast::cast_with_target_field; diff --git a/datafusion/physical-expr/src/expressions/try_cast.rs b/datafusion/physical-expr/src/expressions/try_cast.rs index fe7c834ee499d..6fc20eee4d5fb 100644 --- a/datafusion/physical-expr/src/expressions/try_cast.rs +++ b/datafusion/physical-expr/src/expressions/try_cast.rs @@ -280,6 +280,50 @@ pub fn try_cast( } } +/// Return a PhysicalExpression representing `expr` casted to `target_field`, +/// preserving any explicit field semantics such as metadata. +/// +/// TRY_CAST results are always nullable since failed casts return NULL. +/// +/// If the input expression already has the same data type and the target field +/// has no explicit metadata constraints, the original expression is returned +/// unchanged. +pub fn try_cast_with_target_field( + expr: Arc, + input_schema: &Schema, + target_field: &FieldRef, +) -> Result> { + let expr_type = expr.data_type(input_schema)?; + let cast_type = target_field.data_type(); + + // Check if this is a "default" target field (type-only cast with no explicit + // metadata constraints). This is the field created by `into_nullable_field_ref()` + // when only a DataType is known. + let is_type_only = target_field.name().is_empty() + && target_field.is_nullable() + && target_field.metadata().is_empty(); + + if expr_type == *cast_type && is_type_only { + return Ok(Arc::clone(&expr)); + } + + if !can_cast_types(&expr_type, cast_type) { + return not_impl_err!("Unsupported TRY_CAST from {expr_type} to {cast_type}"); + } + + // For type-only casts, use TryCastExpr::new which preserves source metadata. + // For explicit target fields, use new_with_target_field which applies the target's + // metadata exactly. + if is_type_only { + Ok(Arc::new(TryCastExpr::new(expr, cast_type.clone()))) + } else { + Ok(Arc::new(TryCastExpr::new_with_target_field( + expr, + target_field, + ))) + } +} + #[cfg(test)] mod tests { use super::*; @@ -732,6 +776,162 @@ mod tests { Ok(()) } + + #[test] + fn field_aware_try_cast_uses_exact_target_metadata() -> Result<()> { + // When using field-aware cast, target's metadata should be used exactly + let source_meta = HashMap::from([ + ( + EXTENSION_TYPE_NAME_KEY.to_string(), + "source.type".to_string(), + ), + ("source_key".to_string(), "source_value".to_string()), + ]); + let target_meta = HashMap::from([ + ( + EXTENSION_TYPE_NAME_KEY.to_string(), + "target.type".to_string(), + ), + ( + EXTENSION_TYPE_METADATA_KEY.to_string(), + "target_ext_meta".to_string(), + ), + ("target_key".to_string(), "target_value".to_string()), + ]); + let schema = Schema::new(vec![ + Field::new("a", DataType::FixedSizeBinary(16), false) + .with_metadata(source_meta), + ]); + + let target_field = + Arc::new(Field::new("b", DataType::Utf8, true).with_metadata(target_meta)); + let expr = TryCastExpr::new_with_target_field(col("a", &schema)?, &target_field); + + let field = expr.return_field(&schema)?; + assert_eq!( + field.metadata().get(EXTENSION_TYPE_NAME_KEY), + Some(&"target.type".to_string()), + "Field-aware try_cast should use target's extension type name" + ); + assert_eq!( + field.metadata().get(EXTENSION_TYPE_METADATA_KEY), + Some(&"target_ext_meta".to_string()), + "Field-aware try_cast should use target's extension type metadata" + ); + assert!( + field.metadata().get("source_key").is_none(), + "Field-aware try_cast should NOT preserve source metadata" + ); + assert_eq!( + field.metadata().get("target_key"), + Some(&"target_value".to_string()), + "Field-aware try_cast should preserve target's non-extension metadata" + ); + // TRY_CAST is always nullable + assert!(field.is_nullable()); + + Ok(()) + } + + #[test] + fn field_aware_try_cast_preserves_target_field_semantics() -> Result<()> { + // Target field metadata should be preserved exactly (no merging with source). + // TRY_CAST is always nullable regardless of target field's nullability. + let metadata = HashMap::from([("target_meta".to_string(), "1".to_string())]); + + for child_nullable in [true, false] { + let schema = + Schema::new(vec![Field::new("a", DataType::Int32, child_nullable)]); + let target_field = Arc::new( + Field::new("cast_target", DataType::Int64, false) // target says non-nullable + .with_metadata(metadata.clone()), + ); + let expr = + TryCastExpr::new_with_target_field(col("a", &schema)?, &target_field); + + let field = expr.return_field(&schema)?; + // Field name comes from source + assert_eq!(field.name(), "a"); + assert_eq!(field.data_type(), &DataType::Int64); + // TRY_CAST is ALWAYS nullable (ignores target field's nullability) + assert!(field.is_nullable(), "TRY_CAST should always be nullable"); + // Target metadata should be preserved exactly + assert_eq!( + field.metadata().get("target_meta"), + Some(&"1".to_string()), + "Target metadata should be preserved exactly" + ); + assert!( + expr.nullable(&schema)?, + "TRY_CAST should always be nullable" + ); + } + + Ok(()) + } + + #[test] + fn type_only_try_cast_strips_extension_keys() -> Result<()> { + // Type-only cast should strip extension keys but preserve other source metadata + let source_meta = HashMap::from([ + ( + EXTENSION_TYPE_NAME_KEY.to_string(), + "source.extension".to_string(), + ), + ( + EXTENSION_TYPE_METADATA_KEY.to_string(), + "ext_meta".to_string(), + ), + ("custom_key".to_string(), "custom_value".to_string()), + ]); + let schema = Schema::new(vec![ + Field::new("a", DataType::Int32, false).with_metadata(source_meta), + ]); + + let expr = TryCastExpr::new(col("a", &schema)?, DataType::Int64); + let field = expr.return_field(&schema)?; + + // Extension keys should be stripped + assert!( + field.metadata().get(EXTENSION_TYPE_NAME_KEY).is_none(), + "Type-only try_cast should strip extension type name" + ); + assert!( + field.metadata().get(EXTENSION_TYPE_METADATA_KEY).is_none(), + "Type-only try_cast should strip extension type metadata" + ); + // Non-extension metadata should pass through + assert_eq!( + field.metadata().get("custom_key"), + Some(&"custom_value".to_string()), + "Type-only try_cast should preserve non-extension metadata" + ); + // Field name preserved, type changed, always nullable + assert_eq!(field.name(), "a"); + assert_eq!(field.data_type(), &DataType::Int64); + assert!(field.is_nullable()); + + Ok(()) + } + + #[test] + fn type_only_try_cast_is_always_nullable() -> Result<()> { + // TRY_CAST is always nullable even when source is non-nullable + let schema = Schema::new(vec![Field::new("a", DataType::Int32, false)]); + let expr = TryCastExpr::new(col("a", &schema)?, DataType::Int64); + + let field = expr.return_field(&schema)?; + + assert_eq!(field.name(), "a"); + assert_eq!(field.data_type(), &DataType::Int64); + assert!(field.is_nullable(), "TRY_CAST should always be nullable"); + assert!( + expr.nullable(&schema)?, + "TRY_CAST should always be nullable" + ); + + Ok(()) + } } #[cfg(all(test, feature = "proto"))] diff --git a/datafusion/physical-expr/src/planner.rs b/datafusion/physical-expr/src/planner.rs index 969032acc6633..c0b8340da1b8d 100644 --- a/datafusion/physical-expr/src/planner.rs +++ b/datafusion/physical-expr/src/planner.rs @@ -27,7 +27,7 @@ use crate::{ use arrow::datatypes::Schema; use datafusion_common::config::ConfigOptions; use datafusion_common::datatype::FieldExt; -use datafusion_common::metadata::{FieldMetadata, format_type_and_metadata}; +use datafusion_common::metadata::FieldMetadata; use datafusion_common::{ DFSchema, Result, ScalarValue, TableReference, ToDFSchema, exec_err, internal_datafusion_err, not_impl_err, plan_datafusion_err, plan_err, @@ -40,7 +40,7 @@ use datafusion_expr::expr::{ use datafusion_expr::var_provider::VarType; use datafusion_expr::var_provider::is_system_variables; use datafusion_expr::{ - Between, BinaryExpr, Expr, ExprSchemable, Like, Operator, TryCast, binary_expr, lit, + Between, BinaryExpr, Expr, Like, Operator, TryCast, binary_expr, lit, }; /// [PhysicalExpr] evaluate DataFusion expressions such as `A + 1`, or `CAST(c1 @@ -301,22 +301,10 @@ pub fn create_physical_expr( None, ), Expr::TryCast(TryCast { expr, field }) => { - if !field.metadata().is_empty() { - let (_, src_field) = expr.to_field(input_dfschema)?; - return plan_err!( - "TryCast from {} to {} is not supported", - format_type_and_metadata( - src_field.data_type(), - Some(src_field.metadata()), - ), - format_type_and_metadata(field.data_type(), Some(field.metadata())) - ); - } - - expressions::try_cast( + expressions::try_cast_with_target_field( create_physical_expr(expr, input_dfschema, execution_props)?, input_schema, - field.data_type().clone(), + field, ) } Expr::Not(expr) => { @@ -618,6 +606,14 @@ mod tests { .expect("planner should lower logical CAST to CastExpr") } + fn as_planner_try_cast( + physical: &Arc, + ) -> &expressions::TryCastExpr { + physical + .downcast_ref::() + .expect("planner should lower logical TRY_CAST to TryCastExpr") + } + #[test] fn test_create_physical_expr_scalar_input_output() -> Result<()> { let expr = col("letter").eq(lit("A")); @@ -739,36 +735,70 @@ mod tests { .into(), ), ); - let cast_expr = Expr::Cast(Cast::new_from_field( - Box::new(col("a")), - Arc::clone(&target_field), - )); - let physical = lower_cast_expr(&cast_expr, &schema)?; - let cast = as_planner_cast(&physical); + for use_try_cast in [false, true] { + // For error labelling + let cast_name = if use_try_cast { "TRY_CAST" } else { "CAST" }; - // The CastExpr stores the target type and all target metadata - assert_eq!(cast.cast_type(), &DataType::Int32); - let target_metadata = cast.target_metadata().expect("should have metadata"); - assert_eq!( - target_metadata.get(EXTENSION_TYPE_NAME_KEY), - Some(&"arrow.opaque".to_string()) - ); - assert_eq!(cast.target_nullable(), Some(true)); + let cast_expr = if use_try_cast { + Expr::TryCast(TryCast::new_from_field( + Box::new(col("a")), + Arc::clone(&target_field), + )) + } else { + Expr::Cast(Cast::new_from_field( + Box::new(col("a")), + Arc::clone(&target_field), + )) + }; - // return_field should have all target metadata (exact semantics) - let returned = physical.return_field(&schema)?; - assert_eq!( - returned.metadata().get(EXTENSION_TYPE_NAME_KEY), - Some(&"arrow.opaque".to_string()) - ); - // All target metadata should propagate with exact semantics - assert_eq!( - returned.metadata().get("custom_meta"), - Some(&"custom_value".to_string()), - "All target metadata should propagate with exact semantics" - ); - assert!(physical.nullable(&schema)?); + let physical = lower_cast_expr(&cast_expr, &schema)?; + + // Extract common fields - both CastExpr and TryCastExpr have these + let (cast_type, target_metadata, target_nullable) = if use_try_cast { + let cast = as_planner_try_cast(&physical); + (cast.cast_type(), cast.target_metadata(), None) + } else { + let cast = as_planner_cast(&physical); + ( + cast.cast_type(), + cast.target_metadata(), + cast.target_nullable(), + ) + }; + + // Verify the physical expression stores correct metadata (same for both) + assert_eq!(cast_type, &DataType::Int32, "{cast_name}: cast_type"); + let target_metadata = target_metadata.expect("should have metadata"); + assert_eq!( + target_metadata.get(EXTENSION_TYPE_NAME_KEY), + Some(&"arrow.opaque".to_string()), + "{cast_name}: extension type name" + ); + + // Only CastExpr tracks target_nullable (TryCast is always nullable) + if !use_try_cast { + assert_eq!(target_nullable, Some(true), "{cast_name}: target_nullable"); + } + + // return_field should have all target metadata (exact semantics) + let returned = physical.return_field(&schema)?; + assert_eq!( + returned.metadata().get(EXTENSION_TYPE_NAME_KEY), + Some(&"arrow.opaque".to_string()), + "{cast_name}: return_field extension type name" + ); + // All target metadata should propagate with exact semantics + assert_eq!( + returned.metadata().get("custom_meta"), + Some(&"custom_value".to_string()), + "{cast_name}: All target metadata should propagate with exact semantics" + ); + assert!( + physical.nullable(&schema)?, + "{cast_name}: should be nullable" + ); + } Ok(()) } From fb988475d487524f194f5b3740d86a0f7cee87d1 Mon Sep 17 00:00:00 2001 From: Dewey Dunnington Date: Wed, 1 Jul 2026 21:46:58 -0500 Subject: [PATCH 14/19] sql logic tests --- .../cast_extension_type_metadata.slt | 54 ++++++++++++++++++- 1 file changed, 52 insertions(+), 2 deletions(-) diff --git a/datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt b/datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt index 425d8ac16eaee..2291a1d88ce25 100644 --- a/datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt +++ b/datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt @@ -45,5 +45,55 @@ FROM ( ---- 00010203040506070809000102030506 arrow.uuid -statement error DataFusion error: Optimizer rule 'simplify_expressions' failed[\s\S]*TryCast from FixedSizeBinary\(16\) to FixedSizeBinary\(16\)<\{"ARROW:extension:name": "arrow\.uuid"\}> is not supported -SELECT TRY_CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID); +# TRY_CAST to extension type should also preserve extension metadata +query ?T +SELECT + TRY_CAST( + arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') + AS UUID + ), + arrow_metadata( + TRY_CAST( + arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') + AS UUID + ), + 'ARROW:extension:name' + ); +---- +00010203040506070809000102030506 arrow.uuid + +# TRY_CAST to UUID from a subquery +query ?T +SELECT + TRY_CAST(raw AS UUID), + arrow_metadata(TRY_CAST(raw AS UUID), 'ARROW:extension:name') +FROM ( + VALUES ( + arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') + ) +) AS uuids(raw); +---- +00010203040506070809000102030506 arrow.uuid + +# arrow_cast from UUID to same underlying type (FixedSizeBinary(16)) preserves +# extension metadata because the cast is optimized away as a no-op (same physical type) +query ?T +SELECT + arrow_cast(uuid_val, 'FixedSizeBinary(16)'), + arrow_metadata(arrow_cast(uuid_val, 'FixedSizeBinary(16)'), 'ARROW:extension:name') +FROM ( + SELECT CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID) AS uuid_val +); +---- +00010203040506070809000102030506 arrow.uuid + +# arrow_cast to a different type strips extension metadata (type-only cast semantics) +query ?T +SELECT + arrow_cast(uuid_val, 'Binary'), + arrow_metadata(arrow_cast(uuid_val, 'Binary'), 'ARROW:extension:name') +FROM ( + SELECT CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID) AS uuid_val +); +---- +00010203040506070809000102030506 NULL From efb95ce2585766d59a540e03856828f6d09a96a6 Mon Sep 17 00:00:00 2001 From: Dewey Dunnington Date: Wed, 1 Jul 2026 22:03:37 -0500 Subject: [PATCH 15/19] slightly better --- datafusion/functions/src/core/arrow_cast.rs | 26 ++++++++++++++++--- .../functions/src/core/arrow_try_cast.rs | 24 ++++++++++++++--- .../physical-expr/src/expressions/cast.rs | 12 ++++++++- .../physical-expr/src/expressions/try_cast.rs | 18 ++++++++++--- .../cast_extension_type_metadata.slt | 6 ++--- 5 files changed, 71 insertions(+), 15 deletions(-) diff --git a/datafusion/functions/src/core/arrow_cast.rs b/datafusion/functions/src/core/arrow_cast.rs index 0b67883c17c87..929a92abcca23 100644 --- a/datafusion/functions/src/core/arrow_cast.rs +++ b/datafusion/functions/src/core/arrow_cast.rs @@ -27,8 +27,8 @@ use datafusion_common::{ use datafusion_expr::simplify::{ExprSimplifyResult, SimplifyContext}; use datafusion_expr::{ - Coercion, ColumnarValue, Documentation, Expr, ReturnFieldArgs, ScalarFunctionArgs, - ScalarUDFImpl, Signature, TypeSignatureClass, Volatility, + Coercion, ColumnarValue, Documentation, Expr, ExprSchemable, ReturnFieldArgs, + ScalarFunctionArgs, ScalarUDFImpl, Signature, TypeSignatureClass, Volatility, }; use datafusion_macros::user_doc; @@ -161,9 +161,27 @@ impl ScalarUDFImpl for ArrowCastFunc { let [source_arg, type_arg] = take_function_args(self.name(), args)?; let target_type = data_type_from_type_arg(self.name(), &type_arg)?; let source_type = info.get_data_type(&source_arg)?; + + // We can skip the cast only if: + // 1. The source and target types are the same + // 2. The source has no extension metadata that needs to be stripped let new_expr = if source_type == target_type { - // the argument's data type is already the correct type - source_arg + // Check if source has extension metadata + let source_field = source_arg.to_field(info.schema())?; + let has_extension_metadata = source_field + .1 + .metadata() + .contains_key("ARROW:extension:name"); + if has_extension_metadata { + // Need to create a cast to strip extension metadata + Expr::Cast(datafusion_expr::Cast { + expr: Box::new(source_arg), + field: target_type.into_nullable_field_ref(), + }) + } else { + // the argument's data type is already the correct type + source_arg + } } else { // Use an actual cast to get the correct type Expr::Cast(datafusion_expr::Cast { diff --git a/datafusion/functions/src/core/arrow_try_cast.rs b/datafusion/functions/src/core/arrow_try_cast.rs index d27b29ba5736d..0914695b60370 100644 --- a/datafusion/functions/src/core/arrow_try_cast.rs +++ b/datafusion/functions/src/core/arrow_try_cast.rs @@ -26,8 +26,8 @@ use datafusion_common::{ use datafusion_expr::simplify::{ExprSimplifyResult, SimplifyContext}; use datafusion_expr::{ - Coercion, ColumnarValue, Documentation, Expr, ReturnFieldArgs, ScalarFunctionArgs, - ScalarUDFImpl, Signature, TypeSignatureClass, Volatility, + Coercion, ColumnarValue, Documentation, Expr, ExprSchemable, ReturnFieldArgs, + ScalarFunctionArgs, ScalarUDFImpl, Signature, TypeSignatureClass, Volatility, }; use datafusion_macros::user_doc; @@ -134,8 +134,26 @@ impl ScalarUDFImpl for ArrowTryCastFunc { let target_type = data_type_from_type_arg(self.name(), &type_arg)?; let source_type = info.get_data_type(&source_arg)?; + + // We can skip the cast only if: + // 1. The source and target types are the same + // 2. The source has no extension metadata that needs to be stripped let new_expr = if source_type == target_type { - source_arg + // Check if source has extension metadata + let source_field = source_arg.to_field(info.schema())?; + let has_extension_metadata = source_field + .1 + .metadata() + .contains_key("ARROW:extension:name"); + if has_extension_metadata { + // Need to create a try_cast to strip extension metadata + Expr::TryCast(datafusion_expr::TryCast { + expr: Box::new(source_arg), + field: target_type.into_nullable_field_ref(), + }) + } else { + source_arg + } } else { Expr::TryCast(datafusion_expr::TryCast { expr: Box::new(source_arg), diff --git a/datafusion/physical-expr/src/expressions/cast.rs b/datafusion/physical-expr/src/expressions/cast.rs index d0180adde9b6a..2fc1f9e372fc2 100644 --- a/datafusion/physical-expr/src/expressions/cast.rs +++ b/datafusion/physical-expr/src/expressions/cast.rs @@ -497,8 +497,18 @@ pub fn cast_with_target_field( && target_field.is_nullable() && target_field.metadata().is_empty(); + // For same-type casts, we can skip creating a CastExpr only if: + // 1. The target is type-only (no explicit metadata) + // 2. The source has no extension metadata that needs to be stripped + // Otherwise we need the CastExpr to strip extension metadata from the source. if expr_type == *cast_type && is_type_only { - return Ok(Arc::clone(&expr)); + let source_field = expr.return_field(input_schema)?; + let has_extension_metadata = source_field + .metadata() + .contains_key(EXTENSION_TYPE_NAME_KEY); + if !has_extension_metadata { + return Ok(Arc::clone(&expr)); + } } let can_build_cast = if requires_nested_struct_cast(&expr_type, cast_type) { diff --git a/datafusion/physical-expr/src/expressions/try_cast.rs b/datafusion/physical-expr/src/expressions/try_cast.rs index 6fc20eee4d5fb..88c86398d8c91 100644 --- a/datafusion/physical-expr/src/expressions/try_cast.rs +++ b/datafusion/physical-expr/src/expressions/try_cast.rs @@ -285,9 +285,9 @@ pub fn try_cast( /// /// TRY_CAST results are always nullable since failed casts return NULL. /// -/// If the input expression already has the same data type and the target field -/// has no explicit metadata constraints, the original expression is returned -/// unchanged. +/// If the input expression already has the same data type, the target field +/// has no explicit metadata constraints, and the source has no extension +/// metadata to strip, the original expression is returned unchanged. pub fn try_cast_with_target_field( expr: Arc, input_schema: &Schema, @@ -303,8 +303,18 @@ pub fn try_cast_with_target_field( && target_field.is_nullable() && target_field.metadata().is_empty(); + // For same-type casts, we can skip creating a TryCastExpr only if: + // 1. The target is type-only (no explicit metadata) + // 2. The source has no extension metadata that needs to be stripped + // Otherwise we need the TryCastExpr to strip extension metadata from the source. if expr_type == *cast_type && is_type_only { - return Ok(Arc::clone(&expr)); + let source_field = expr.return_field(input_schema)?; + let has_extension_metadata = source_field + .metadata() + .contains_key(EXTENSION_TYPE_NAME_KEY); + if !has_extension_metadata { + return Ok(Arc::clone(&expr)); + } } if !can_cast_types(&expr_type, cast_type) { diff --git a/datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt b/datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt index 2291a1d88ce25..01a19454e9a80 100644 --- a/datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt +++ b/datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt @@ -75,8 +75,8 @@ FROM ( ---- 00010203040506070809000102030506 arrow.uuid -# arrow_cast from UUID to same underlying type (FixedSizeBinary(16)) preserves -# extension metadata because the cast is optimized away as a no-op (same physical type) +# arrow_cast from UUID to same underlying type (FixedSizeBinary(16)) strips +# extension metadata (type-only cast semantics) query ?T SELECT arrow_cast(uuid_val, 'FixedSizeBinary(16)'), @@ -85,7 +85,7 @@ FROM ( SELECT CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID) AS uuid_val ); ---- -00010203040506070809000102030506 arrow.uuid +00010203040506070809000102030506 NULL # arrow_cast to a different type strips extension metadata (type-only cast semantics) query ?T From 7d1ceaa5646695e3e78b6f03d254a8d2eb0a51d9 Mon Sep 17 00:00:00 2001 From: Dewey Dunnington Date: Wed, 1 Jul 2026 22:09:47 -0500 Subject: [PATCH 16/19] better --- datafusion/expr/src/expr_schema.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/datafusion/expr/src/expr_schema.rs b/datafusion/expr/src/expr_schema.rs index a60e29427faec..7418af51e4c8d 100644 --- a/datafusion/expr/src/expr_schema.rs +++ b/datafusion/expr/src/expr_schema.rs @@ -484,7 +484,8 @@ impl ExprSchemable for Expr { /// - **Aliases**: Merge underlying expr metadata with alias-specific metadata, preferring the alias metadata /// - **Binary expressions**: field metadata is empty /// - **Boolean expressions**: field metadata is empty - /// - **Cast expressions**: determined by the input expression's field metadata handling + /// - **Cast expressions**: Type-only casts pass through source metadata (stripping extension + /// type keys); casts with explicit target fields use target metadata exactly /// - **Scalar functions**: Generate metadata via function's [`return_field_from_args`] method, /// with the default implementation returning empty field metadata /// - **Aggregate functions**: Generate metadata via function's [`return_field`] method, From 8a771c8e69acc0d59977193e221d3576421abc73 Mon Sep 17 00:00:00 2001 From: Dewey Dunnington Date: Mon, 31 Aug 2026 12:07:15 -0500 Subject: [PATCH 17/19] fix cast target field borrowing --- .../physical-expr-adapter/src/schema_rewriter.rs | 10 ++++++---- datafusion/physical-plan/src/union.rs | 7 ++----- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/datafusion/physical-expr-adapter/src/schema_rewriter.rs b/datafusion/physical-expr-adapter/src/schema_rewriter.rs index b731fcbea72dd..628ceb610a675 100644 --- a/datafusion/physical-expr-adapter/src/schema_rewriter.rs +++ b/datafusion/physical-expr-adapter/src/schema_rewriter.rs @@ -486,7 +486,8 @@ impl DefaultPhysicalExprAdapterRewriter { return Ok(None); }; - let DataType::Struct(logical_struct_fields) = cast.target_field().data_type() + let cast_target_field = cast.target_field(); + let DataType::Struct(logical_struct_fields) = cast_target_field.data_type() else { return Ok(None); }; @@ -554,14 +555,15 @@ impl DefaultPhysicalExprAdapterRewriter { || (!contains_type(source_type, &is_struct) && !contains_type(target_type, &is_struct))) { - let Some(target_field) = retain_field_path(cast.target_field(), &field_path) + let cast_target_field = cast.target_field(); + let Some(target_field) = retain_field_path(&cast_target_field, &field_path) else { return Ok(None); }; let mut args = get_field_expr.args().to_vec(); args[0] = Arc::new(CastExpr::new_with_target_field( Arc::clone(inner), - target_field, + &target_field, Some(cast.cast_options().clone()), )); return Arc::clone(expr).with_new_children(args).map(Some); @@ -589,7 +591,7 @@ impl DefaultPhysicalExprAdapterRewriter { } Ok(Some(Arc::new(CastExpr::new_with_target_field( extracted, - logical_return_field, + &logical_return_field, Some(cast.cast_options().clone()), )))) } diff --git a/datafusion/physical-plan/src/union.rs b/datafusion/physical-plan/src/union.rs index 557d8f21d136b..499f8d8454da1 100644 --- a/datafusion/physical-plan/src/union.rs +++ b/datafusion/physical-plan/src/union.rs @@ -114,11 +114,8 @@ fn coerce_schema( let expr = if input_field == target_field { column } else { - Arc::new(CastExpr::new_with_target_field( - column, - Arc::clone(target_field), - None, - )) as Arc + Arc::new(CastExpr::new_with_target_field(column, target_field, None)) + as Arc }; Ok(ProjectionExpr { expr, From e7dcfda10b756e13603687f7c158ae904627103f Mon Sep 17 00:00:00 2001 From: Tim Saucer Date: Mon, 31 Aug 2026 15:57:15 -0400 Subject: [PATCH 18/19] Keep cast metadata changes non-breaking 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>` / `Option` 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. --- .../physical-expr-adapter/src/rewrite.rs | 5 +- .../src/schema_rewriter.rs | 14 +- .../src/equivalence/properties/dependency.rs | 2 +- .../physical-expr/src/expressions/cast.rs | 172 ++++++++++-------- .../physical-expr/src/expressions/try_cast.rs | 86 +++++---- datafusion/physical-plan/src/common.rs | 2 +- datafusion/physical-plan/src/union.rs | 7 +- datafusion/pruning/src/pruning_predicate.rs | 2 +- 8 files changed, 165 insertions(+), 125 deletions(-) diff --git a/datafusion/physical-expr-adapter/src/rewrite.rs b/datafusion/physical-expr-adapter/src/rewrite.rs index fdc4d2d012e59..3d31feca96031 100644 --- a/datafusion/physical-expr-adapter/src/rewrite.rs +++ b/datafusion/physical-expr-adapter/src/rewrite.rs @@ -104,7 +104,7 @@ pub fn rewrite_file_row_index_expr( let target_field = Arc::new(Field::new("file_row_index", DataType::Int64, true)); Ok(Arc::new(CastExpr::new_with_target_field( source, - &target_field, + target_field, None, ))) }) @@ -306,9 +306,8 @@ mod tests { .downcast_ref::() .expect("file row index expression should be a cast"); assert_eq!(cast_expr.cast_type(), &DataType::Int64); - // Note: target_field() does not preserve the name since CastExpr only stores - // type, metadata, and nullability. The name comes from the source expression. let target_field = cast_expr.target_field(); + assert_eq!(target_field.name(), "file_row_index"); assert_eq!(target_field.data_type(), &DataType::Int64); assert!(target_field.is_nullable()); assert!(target_field.metadata().is_empty()); diff --git a/datafusion/physical-expr-adapter/src/schema_rewriter.rs b/datafusion/physical-expr-adapter/src/schema_rewriter.rs index 628ceb610a675..2548ffc6fb1b7 100644 --- a/datafusion/physical-expr-adapter/src/schema_rewriter.rs +++ b/datafusion/physical-expr-adapter/src/schema_rewriter.rs @@ -486,8 +486,7 @@ impl DefaultPhysicalExprAdapterRewriter { return Ok(None); }; - let cast_target_field = cast.target_field(); - let DataType::Struct(logical_struct_fields) = cast_target_field.data_type() + let DataType::Struct(logical_struct_fields) = cast.target_field().data_type() else { return Ok(None); }; @@ -555,15 +554,14 @@ impl DefaultPhysicalExprAdapterRewriter { || (!contains_type(source_type, &is_struct) && !contains_type(target_type, &is_struct))) { - let cast_target_field = cast.target_field(); - let Some(target_field) = retain_field_path(&cast_target_field, &field_path) + let Some(target_field) = retain_field_path(cast.target_field(), &field_path) else { return Ok(None); }; let mut args = get_field_expr.args().to_vec(); args[0] = Arc::new(CastExpr::new_with_target_field( Arc::clone(inner), - &target_field, + target_field, Some(cast.cast_options().clone()), )); return Arc::clone(expr).with_new_children(args).map(Some); @@ -591,7 +589,7 @@ impl DefaultPhysicalExprAdapterRewriter { } Ok(Some(Arc::new(CastExpr::new_with_target_field( extracted, - &logical_return_field, + logical_return_field, Some(cast.cast_options().clone()), )))) } @@ -746,7 +744,7 @@ impl DefaultPhysicalExprAdapterRewriter { Ok(Transformed::yes(Arc::new(CastExpr::new_with_target_field( Arc::new(resolved_column), - &Arc::new(logical_field.clone()), + Arc::new(logical_field.clone()), None, )))) } @@ -1149,7 +1147,7 @@ mod tests { let expected = Arc::new(CastExpr::new_with_target_field( Arc::new(Column::new("data", 0)), - &logical_field, + logical_field, None, )) as Arc; diff --git a/datafusion/physical-expr/src/equivalence/properties/dependency.rs b/datafusion/physical-expr/src/equivalence/properties/dependency.rs index b207426400c7d..bd8bef84de2d8 100644 --- a/datafusion/physical-expr/src/equivalence/properties/dependency.rs +++ b/datafusion/physical-expr/src/equivalence/properties/dependency.rs @@ -940,7 +940,7 @@ mod tests { let col_c = col("c", schema.as_ref())?; let cast_c = Arc::new(CastExpr::new_with_target_field( col_c, - &Arc::new(Field::new("c", DataType::Date32, true)), + Arc::new(Field::new("c", DataType::Date32, true)), None, )) as _; let required_sort = vec![PhysicalSortExpr::new_default(col("c", &schema)?)]; diff --git a/datafusion/physical-expr/src/expressions/cast.rs b/datafusion/physical-expr/src/expressions/cast.rs index 6ab120713d4cc..565d396dad1b5 100644 --- a/datafusion/physical-expr/src/expressions/cast.rs +++ b/datafusion/physical-expr/src/expressions/cast.rs @@ -26,6 +26,7 @@ use arrow::compute::{CastOptions, can_cast_types}; use arrow::datatypes::{DataType, DataType::*, Field, FieldRef, Schema}; use arrow::record_batch::RecordBatch; use arrow_schema::extension::{EXTENSION_TYPE_METADATA_KEY, EXTENSION_TYPE_NAME_KEY}; +use datafusion_common::datatype::DataTypeExt; use datafusion_common::format::DEFAULT_FORMAT_OPTIONS; use datafusion_common::nested_struct::{ requires_nested_struct_cast, validate_data_type_compatibility, @@ -60,16 +61,18 @@ fn can_cast_named_struct_types(source: &DataType, target: &DataType) -> bool { pub struct CastExpr { /// The expression to cast pub expr: Arc, - /// The target data type to cast to - target_type: DataType, - /// Explicit metadata for the output field, or `None` to pass through source metadata - /// (with extension type keys stripped). + /// The target field. /// - /// When `Some`, this is the exact metadata to use for the output field. - /// When `None`, source metadata passes through (stripping extension keys). - target_metadata: Option>, - /// Explicit nullability for the output field, or `None` to pass through source nullability. - target_nullable: Option, + /// For a type-only cast (see [`CastExpr::new`]) this is a field synthesized + /// from the target data type alone and only its data type is meaningful. + /// For a cast built from an explicit field (see + /// [`CastExpr::new_with_target_field`]) its metadata and nullability are + /// applied to the output field as-is. + target_field: FieldRef, + /// Whether `target_field` was supplied by the caller (as opposed to being + /// synthesized from a `DataType`), and therefore whether its metadata and + /// nullability describe the output field exactly. + explicit_target: bool, /// Cast options cast_options: CastOptions<'static>, } @@ -77,10 +80,12 @@ pub struct CastExpr { // Manually derive PartialEq and Hash to work around https://github.com/rust-lang/rust/issues/78808 impl PartialEq for CastExpr { fn eq(&self, other: &Self) -> bool { + // Compare the semantically meaningful parts of the target field only: + // the field name never affects the output of this expression. self.expr.eq(&other.expr) - && self.target_type.eq(&other.target_type) - && self.target_metadata.eq(&other.target_metadata) - && self.target_nullable.eq(&other.target_nullable) + && self.cast_type().eq(other.cast_type()) + && self.target_metadata().eq(&other.target_metadata()) + && self.target_nullable().eq(&other.target_nullable()) && self.cast_options.eq(&other.cast_options) } } @@ -88,9 +93,9 @@ impl PartialEq for CastExpr { impl Hash for CastExpr { fn hash(&self, state: &mut H) { self.expr.hash(state); - self.target_type.hash(state); + self.cast_type().hash(state); // Hash the metadata by iterating over sorted keys for deterministic ordering - if let Some(metadata) = &self.target_metadata { + if let Some(metadata) = self.target_metadata() { let mut entries: Vec<_> = metadata.iter().collect(); entries.sort_by_key(|(k, _)| *k); for (k, v) in entries { @@ -98,7 +103,7 @@ impl Hash for CastExpr { v.hash(state); } } - self.target_nullable.hash(state); + self.target_nullable().hash(state); self.cast_options.hash(state); } } @@ -120,9 +125,8 @@ impl CastExpr { ) -> Self { Self { expr, - target_type: cast_type, - target_metadata: None, - target_nullable: None, + target_field: cast_type.into_nullable_field_ref(), + explicit_target: false, cast_options: cast_options.unwrap_or(DEFAULT_CAST_OPTIONS), } } @@ -142,14 +146,13 @@ impl CastExpr { /// pass through. pub fn new_with_target_field( expr: Arc, - target_field: &FieldRef, + target_field: FieldRef, cast_options: Option>, ) -> Self { Self { expr, - target_type: target_field.data_type().clone(), - target_metadata: Some(target_field.metadata().clone()), - target_nullable: Some(target_field.is_nullable()), + target_field, + explicit_target: true, cast_options: cast_options.unwrap_or(DEFAULT_CAST_OPTIONS), } } @@ -161,37 +164,35 @@ impl CastExpr { /// The data type to cast to pub fn cast_type(&self) -> &DataType { - &self.target_type + self.target_field.data_type() } /// Explicit metadata for the output field, or `None` to pass through source metadata. pub fn target_metadata(&self) -> Option<&HashMap> { - self.target_metadata.as_ref() + self.explicit_target.then(|| self.target_field.metadata()) } /// Explicit nullability for the output field, or `None` to pass through source nullability. pub fn target_nullable(&self) -> Option { - self.target_nullable + self.explicit_target + .then(|| self.target_field.is_nullable()) } - /// Returns a computed target `FieldRef` for compatibility with existing callers. + /// The target field this cast was constructed with. /// - /// This synthesizes a field based on the current configuration. The returned + /// For a type-only cast this is a field synthesized from the target data + /// type alone; only its data type is meaningful. Note that the returned /// field may not match what `return_field()` returns when evaluated against /// a schema, since `return_field()` may incorporate source field information. /// - /// Prefer using [`cast_type()`], [`target_metadata()`], and [`target_nullable()`] + /// Prefer [`cast_type()`], [`target_metadata()`], and [`target_nullable()`] /// for direct access to the individual components. /// /// [`cast_type()`]: CastExpr::cast_type /// [`target_metadata()`]: CastExpr::target_metadata /// [`target_nullable()`]: CastExpr::target_nullable - pub fn target_field(&self) -> FieldRef { - let metadata = self.target_metadata.clone().unwrap_or_default(); - let nullable = self.target_nullable.unwrap_or(true); - Arc::new( - Field::new("", self.target_type.clone(), nullable).with_metadata(metadata), - ) + pub fn target_field(&self) -> &FieldRef { + &self.target_field } /// The cast options @@ -201,58 +202,48 @@ impl CastExpr { /// Whether this cast has explicit metadata (vs pass-through from source). pub fn has_explicit_metadata(&self) -> bool { - self.target_metadata.is_some() + self.explicit_target } /// Whether this cast has explicit nullability (vs pass-through from source). pub fn has_explicit_nullability(&self) -> bool { - self.target_nullable.is_some() + self.explicit_target } fn resolved_target_field(&self, input_schema: &Schema) -> Result { - // Try to get the source field for the name. If both metadata and nullability - // are explicit, we can fall back to an empty name if the source lookup fails + // Try to get the source field for the name. If the target field is + // explicit, we can fall back to an empty name if the source lookup fails // (e.g., for virtual row-index columns appended at scan time). let source_result = self.expr.return_field(input_schema); - if let (Some(metadata), Some(nullable)) = - (&self.target_metadata, self.target_nullable) - { - // Both metadata and nullability are explicit + if self.explicit_target { + // Metadata and nullability come from the target field verbatim let name = source_result .as_ref() .map(|f| f.name().to_string()) .unwrap_or_default(); return Ok(Arc::new( - Field::new(name, self.target_type.clone(), nullable) - .with_metadata(metadata.clone()), + Field::new( + name, + self.cast_type().clone(), + self.target_field.is_nullable(), + ) + .with_metadata(self.target_field.metadata().clone()), )); } - // Need source field for metadata or nullability + // Type-only cast: pass through the source metadata and nullability, + // stripping extension type keys (the cast is to a plain storage type). source_result.map(|source_field| { - // Build metadata: use explicit if provided, otherwise pass through - // from source (stripping extension keys) - let metadata = if let Some(explicit_metadata) = &self.target_metadata { - explicit_metadata.clone() - } else { - let mut meta = source_field.metadata().clone(); - meta.remove(EXTENSION_TYPE_NAME_KEY); - meta.remove(EXTENSION_TYPE_METADATA_KEY); - meta - }; - - // Determine nullability: explicit or from source - let nullable = self - .target_nullable - .unwrap_or_else(|| source_field.is_nullable()); + let mut metadata = source_field.metadata().clone(); + metadata.remove(EXTENSION_TYPE_NAME_KEY); + metadata.remove(EXTENSION_TYPE_METADATA_KEY); Arc::new( source_field .as_ref() .clone() - .with_data_type(self.target_type.clone()) - .with_nullable(nullable) + .with_data_type(self.cast_type().clone()) .with_metadata(metadata), ) }) @@ -356,9 +347,8 @@ impl PhysicalExpr for CastExpr { ) -> Result> { Ok(Arc::new(CastExpr { expr: Arc::clone(&children[0]), - target_type: self.target_type.clone(), - target_metadata: self.target_metadata.clone(), - target_nullable: self.target_nullable, + target_field: Arc::clone(&self.target_field), + explicit_target: self.explicit_target, cast_options: self.cast_options.clone(), })) } @@ -545,7 +535,7 @@ pub fn cast_with_target_field( } else { Ok(Arc::new(CastExpr::new_with_target_field( expr, - target_field, + Arc::clone(target_field), cast_options, ))) } @@ -603,7 +593,7 @@ mod tests { )?; let expr = CastExpr::new_with_target_field( col(column, schema.as_ref())?, - &Arc::new(target_field), + Arc::new(target_field), None, ); @@ -1128,8 +1118,11 @@ mod tests { Field::new("cast_target", Int64, target_nullable) .with_metadata(metadata.clone()), ); - let expr = - CastExpr::new_with_target_field(col("a", &schema)?, &target_field, None); + let expr = CastExpr::new_with_target_field( + col("a", &schema)?, + Arc::clone(&target_field), + None, + ); let field = expr.return_field(&schema)?; // Field name comes from source @@ -1149,6 +1142,38 @@ mod tests { Ok(()) } + #[test] + fn target_field_accessor_returns_the_constructed_field() -> Result<()> { + let schema = Schema::new(vec![Field::new("a", Int32, true)]); + let metadata = HashMap::from([("target_meta".to_string(), "1".to_string())]); + let target_field = + Arc::new(Field::new("cast_target", Int64, false).with_metadata(metadata)); + + let expr = CastExpr::new_with_target_field( + col("a", &schema)?, + Arc::clone(&target_field), + None, + ); + + // The field is returned verbatim, including its name. + assert_eq!(expr.target_field(), &target_field); + assert_eq!(expr.cast_type(), &Int64); + assert_eq!(expr.target_metadata(), Some(target_field.metadata())); + assert_eq!(expr.target_nullable(), Some(false)); + assert!(expr.has_explicit_metadata()); + assert!(expr.has_explicit_nullability()); + + // A type-only cast reports no explicit target. + let type_only = CastExpr::new(col("a", &schema)?, Int64, None); + assert_eq!(type_only.cast_type(), &Int64); + assert_eq!(type_only.target_metadata(), None); + assert_eq!(type_only.target_nullable(), None); + assert!(!type_only.has_explicit_metadata()); + assert!(!type_only.has_explicit_nullability()); + + Ok(()) + } + #[test] fn type_only_cast_preserves_legacy_field_name_and_nullability() -> Result<()> { let schema = Schema::new(vec![Field::new("a", Int32, false)]); @@ -1303,7 +1328,7 @@ mod tests { Arc::new(scalar_struct), ))); let target_field = Arc::new(target_field); - let expr = CastExpr::new_with_target_field(literal, &target_field, None); + let expr = CastExpr::new_with_target_field(literal, target_field, None); let batch = RecordBatch::new_empty(schema); let result = expr.evaluate(&batch)?; @@ -1406,8 +1431,11 @@ mod tests { let target_field = Arc::new(Field::new("b", Utf8, true).with_metadata(target_meta)); - let expr = - CastExpr::new_with_target_field(col("a", &schema)?, &target_field, None); + let expr = CastExpr::new_with_target_field( + col("a", &schema)?, + Arc::clone(&target_field), + None, + ); let field = expr.return_field(&schema)?; assert_eq!( diff --git a/datafusion/physical-expr/src/expressions/try_cast.rs b/datafusion/physical-expr/src/expressions/try_cast.rs index 88c86398d8c91..c054026724fb1 100644 --- a/datafusion/physical-expr/src/expressions/try_cast.rs +++ b/datafusion/physical-expr/src/expressions/try_cast.rs @@ -27,6 +27,7 @@ use arrow::datatypes::{DataType, Field, FieldRef, Schema}; use arrow::record_batch::RecordBatch; use arrow_schema::extension::{EXTENSION_TYPE_METADATA_KEY, EXTENSION_TYPE_NAME_KEY}; use compute::can_cast_types; +use datafusion_common::datatype::DataTypeExt; use datafusion_common::format::DEFAULT_FORMAT_OPTIONS; use datafusion_common::{Result, not_impl_err}; use datafusion_expr::ColumnarValue; @@ -36,31 +37,37 @@ use datafusion_expr::ColumnarValue; pub struct TryCastExpr { /// The expression to cast expr: Arc, - /// The data type to cast to - cast_type: DataType, - /// Explicit metadata for the output field, or `None` to pass through source metadata - /// (with extension type keys stripped). + /// The target field. /// - /// When `Some`, this is the exact metadata to use for the output field. - /// When `None`, source metadata passes through (stripping extension keys). - target_metadata: Option>, + /// For a type-only cast (see [`TryCastExpr::new`]) this is a field + /// synthesized from the target data type alone and only its data type is + /// meaningful. For a cast built from an explicit field (see + /// [`TryCastExpr::new_with_target_field`]) its metadata is applied to the + /// output field as-is. + target_field: FieldRef, + /// Whether `target_field` was supplied by the caller (as opposed to being + /// synthesized from a `DataType`), and therefore whether its metadata + /// describes the output field exactly. + explicit_target: bool, } // Manually derive PartialEq and Hash to work around https://github.com/rust-lang/rust/issues/78808 impl PartialEq for TryCastExpr { fn eq(&self, other: &Self) -> bool { + // Compare the semantically meaningful parts of the target field only: + // the field name never affects the output of this expression. self.expr.eq(&other.expr) - && self.cast_type == other.cast_type - && self.target_metadata == other.target_metadata + && self.cast_type() == other.cast_type() + && self.target_metadata() == other.target_metadata() } } impl Hash for TryCastExpr { fn hash(&self, state: &mut H) { self.expr.hash(state); - self.cast_type.hash(state); + self.cast_type().hash(state); // Hash the metadata by iterating over sorted keys for deterministic ordering - if let Some(metadata) = &self.target_metadata { + if let Some(metadata) = self.target_metadata() { let mut entries: Vec<_> = metadata.iter().collect(); entries.sort_by_key(|(k, _)| *k); for (k, v) in entries { @@ -80,8 +87,8 @@ impl TryCastExpr { pub fn new(expr: Arc, cast_type: DataType) -> Self { Self { expr, - cast_type, - target_metadata: None, + target_field: cast_type.into_nullable_field_ref(), + explicit_target: false, } } @@ -97,12 +104,12 @@ impl TryCastExpr { /// pass through. pub fn new_with_target_field( expr: Arc, - target_field: &FieldRef, + target_field: FieldRef, ) -> Self { Self { expr, - cast_type: target_field.data_type().clone(), - target_metadata: Some(target_field.metadata().clone()), + target_field, + explicit_target: true, } } @@ -113,33 +120,33 @@ impl TryCastExpr { /// The data type to cast to pub fn cast_type(&self) -> &DataType { - &self.cast_type + self.target_field.data_type() } /// Explicit metadata for the output field, or `None` to pass through source metadata. pub fn target_metadata(&self) -> Option<&HashMap> { - self.target_metadata.as_ref() + self.explicit_target.then(|| self.target_field.metadata()) } - /// Returns a computed target `FieldRef` for compatibility with existing callers. + /// The target field this cast was constructed with. /// - /// This synthesizes a field based on the current configuration. TRY_CAST - /// results are always nullable. - pub fn target_field(&self) -> FieldRef { - let metadata = self.target_metadata.clone().unwrap_or_default(); - Arc::new(Field::new("", self.cast_type.clone(), true).with_metadata(metadata)) + /// For a type-only cast this is a field synthesized from the target data + /// type alone; only its data type is meaningful. TRY_CAST results are + /// always nullable regardless of the target field's nullability. + pub fn target_field(&self) -> &FieldRef { + &self.target_field } } impl fmt::Display for TryCastExpr { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "TRY_CAST({} AS {})", self.expr, self.cast_type) + write!(f, "TRY_CAST({} AS {})", self.expr, self.cast_type()) } } impl PhysicalExpr for TryCastExpr { fn data_type(&self, _input_schema: &Schema) -> Result { - Ok(self.cast_type.clone()) + Ok(self.cast_type().clone()) } fn nullable(&self, _input_schema: &Schema) -> Result { @@ -152,7 +159,7 @@ impl PhysicalExpr for TryCastExpr { safe: true, format_options: DEFAULT_FORMAT_OPTIONS, }; - value.cast_to(&self.cast_type, Some(&options)) + value.cast_to(self.cast_type(), Some(&options)) } fn return_field(&self, input_schema: &Schema) -> Result { @@ -160,14 +167,14 @@ impl PhysicalExpr for TryCastExpr { // (though we still try to get source for the name) let source_result = self.expr.return_field(input_schema); - if let Some(metadata) = &self.target_metadata { + if let Some(metadata) = self.target_metadata() { // Explicit metadata: use it exactly, TRY_CAST is always nullable let name = source_result .as_ref() .map(|f| f.name().to_string()) .unwrap_or_default(); return Ok(Arc::new( - Field::new(name, self.cast_type.clone(), true) + Field::new(name, self.cast_type().clone(), true) .with_metadata(metadata.clone()), )); } @@ -182,7 +189,7 @@ impl PhysicalExpr for TryCastExpr { source_field .as_ref() .clone() - .with_data_type(self.cast_type.clone()) + .with_data_type(self.cast_type().clone()) .with_nullable(true) // TRY_CAST is always nullable .with_metadata(metadata), ) @@ -199,15 +206,15 @@ impl PhysicalExpr for TryCastExpr { ) -> Result> { Ok(Arc::new(TryCastExpr { expr: Arc::clone(&children[0]), - cast_type: self.cast_type.clone(), - target_metadata: self.target_metadata.clone(), + target_field: Arc::clone(&self.target_field), + explicit_target: self.explicit_target, })) } fn fmt_sql(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "TRY_CAST(")?; self.expr.fmt_sql(f)?; - write!(f, " AS {:?})", self.cast_type) + write!(f, " AS {:?})", self.cast_type()) } #[cfg(feature = "proto")] @@ -329,7 +336,7 @@ pub fn try_cast_with_target_field( } else { Ok(Arc::new(TryCastExpr::new_with_target_field( expr, - target_field, + Arc::clone(target_field), ))) } } @@ -815,7 +822,10 @@ mod tests { let target_field = Arc::new(Field::new("b", DataType::Utf8, true).with_metadata(target_meta)); - let expr = TryCastExpr::new_with_target_field(col("a", &schema)?, &target_field); + let expr = TryCastExpr::new_with_target_field( + col("a", &schema)?, + Arc::clone(&target_field), + ); let field = expr.return_field(&schema)?; assert_eq!( @@ -856,8 +866,10 @@ mod tests { Field::new("cast_target", DataType::Int64, false) // target says non-nullable .with_metadata(metadata.clone()), ); - let expr = - TryCastExpr::new_with_target_field(col("a", &schema)?, &target_field); + let expr = TryCastExpr::new_with_target_field( + col("a", &schema)?, + Arc::clone(&target_field), + ); let field = expr.return_field(&schema)?; // Field name comes from source diff --git a/datafusion/physical-plan/src/common.rs b/datafusion/physical-plan/src/common.rs index 616b0f3f23fae..734ec96debc85 100644 --- a/datafusion/physical-plan/src/common.rs +++ b/datafusion/physical-plan/src/common.rs @@ -162,7 +162,7 @@ pub fn project_plan_to_schema( let expr = if !input_field.is_nullable() && expected_field.is_nullable() { Arc::new(CastExpr::new_with_target_field( column, - expected_field, + Arc::clone(expected_field), None, )) as _ } else { diff --git a/datafusion/physical-plan/src/union.rs b/datafusion/physical-plan/src/union.rs index 499f8d8454da1..557d8f21d136b 100644 --- a/datafusion/physical-plan/src/union.rs +++ b/datafusion/physical-plan/src/union.rs @@ -114,8 +114,11 @@ fn coerce_schema( let expr = if input_field == target_field { column } else { - Arc::new(CastExpr::new_with_target_field(column, target_field, None)) - as Arc + Arc::new(CastExpr::new_with_target_field( + column, + Arc::clone(target_field), + None, + )) as Arc }; Ok(ProjectionExpr { expr, diff --git a/datafusion/pruning/src/pruning_predicate.rs b/datafusion/pruning/src/pruning_predicate.rs index 98cb1da80684a..4985cadac72b5 100644 --- a/datafusion/pruning/src/pruning_predicate.rs +++ b/datafusion/pruning/src/pruning_predicate.rs @@ -1236,7 +1236,7 @@ fn rewrite_expr_to_prunable( )?; let left = Arc::new(phys_expr::CastExpr::new_with_target_field( left, - &cast.target_field(), + Arc::clone(cast.target_field()), None, )); // PruningPredicate does not support pruning on nested fields yet. From e0771007c8b47e24d4bd1079944b0aa1eba9d9e4 Mon Sep 17 00:00:00 2001 From: Dewey Dunnington Date: Mon, 31 Aug 2026 16:25:29 -0500 Subject: [PATCH 19/19] add expr proto fix --- .../proto/src/logical_plan/from_proto.rs | 6 +++-- .../tests/cases/roundtrip_logical_plan.rs | 26 +++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/datafusion/proto/src/logical_plan/from_proto.rs b/datafusion/proto/src/logical_plan/from_proto.rs index d4d0ea7292ffe..2830edd35760f 100644 --- a/datafusion/proto/src/logical_plan/from_proto.rs +++ b/datafusion/proto/src/logical_plan/from_proto.rs @@ -429,7 +429,8 @@ pub fn parse_expr( let data_type: DataType = cast.arrow_type.as_ref().required("arrow_type")?; let field = data_type .into_nullable_field() - .with_nullable(cast.nullable.unwrap_or(true)); + .with_nullable(cast.nullable.unwrap_or(true)) + .with_metadata(cast.metadata.clone()); Ok(Expr::Cast(Cast::new_from_field(expr, Arc::new(field)))) } ExprType::TryCast(cast) => { @@ -442,7 +443,8 @@ pub fn parse_expr( let data_type: DataType = cast.arrow_type.as_ref().required("arrow_type")?; let field = data_type .into_nullable_field() - .with_nullable(cast.nullable.unwrap_or(true)); + .with_nullable(cast.nullable.unwrap_or(true)) + .with_metadata(cast.metadata.clone()); Ok(Expr::TryCast(TryCast::new_from_field( expr, Arc::new(field), diff --git a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs index 7c916db9c3cd1..750b20323ad2e 100644 --- a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs +++ b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs @@ -2693,6 +2693,19 @@ fn roundtrip_cast() { let ctx = SessionContext::new(); roundtrip_expr_test(test_expr, ctx); + + let field = + Field::new("", DataType::Boolean, false).with_metadata(HashMap::from([( + String::from("key"), + String::from("value"), + )])); + let test_expr = Expr::Cast(Cast::new_from_field( + Box::new(lit(1.0_f32)), + Arc::new(field), + )); + + let ctx = SessionContext::new(); + roundtrip_expr_test(test_expr, ctx); } #[test] @@ -2703,6 +2716,19 @@ fn roundtrip_try_cast() { let ctx = SessionContext::new(); roundtrip_expr_test(test_expr, ctx); + let field = + Field::new("", DataType::Boolean, false).with_metadata(HashMap::from([( + String::from("key"), + String::from("value"), + )])); + let test_expr = Expr::TryCast(TryCast::new_from_field( + Box::new(lit(1.0_f32)), + Arc::new(field), + )); + + let ctx = SessionContext::new(); + roundtrip_expr_test(test_expr, ctx); + let test_expr = Expr::TryCast(TryCast::new(Box::new(lit("not a bool")), DataType::Boolean));