paleolimbot commented on code in PR #23169:
URL: https://github.com/apache/datafusion/pull/23169#discussion_r3898456307
##########
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:
I don't think so...if there's extension metadata but no extension name, I
think it's just metadata without any special handling (or more likely, the
producer screwed up). Happy to add if you feel strongly.
--
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]
