gene-bordegaray commented on code in PR #23169:
URL: https://github.com/apache/datafusion/pull/23169#discussion_r3897030669
##########
datafusion/physical-expr/src/expressions/cast.rs:
##########
@@ -372,32 +461,61 @@ pub fn cast_with_options(
cast_type: DataType,
cast_options: Option<CastOptions<'static>>,
) -> Result<Arc<dyn PhysicalExpr>> {
- 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<dyn PhysicalExpr>,
input_schema: &Schema,
- target_field: FieldRef,
+ target_field: &FieldRef,
cast_options: Option<CastOptions<'static>>,
) -> Result<Arc<dyn PhysicalExpr>> {
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) {
- return Ok(Arc::clone(&expr));
+
+ // 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();
+
+ // 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 {
+ let source_field = expr.return_field(input_schema)?;
+ let has_extension_metadata = source_field
+ .metadata()
+ .contains_key(EXTENSION_TYPE_NAME_KEY);
Review Comment:
should this be:
```rust
let metadata = source_field.metadata();
let has_extension_metadata = metadata.contains_key(EXTENSION_TYPE_NAME_KEY
|| metadata.contains_key(EXTENSION_TYPE_METADATA_KEY);
```
same with the corresponding line in `try_cast`
##########
datafusion/physical-expr-adapter/src/schema_rewriter.rs:
##########
@@ -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();
Review Comment:
I think we can use `cast_target_field` here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
