paleolimbot commented on code in PR #23169:
URL: https://github.com/apache/datafusion/pull/23169#discussion_r3888276916
##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -69,18 +70,46 @@ 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:
+/// - 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 `<some extension type>::<some non extension
type>` should have the
+/// return type of `<some non extension type>` (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
+/// 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(
source_field: &FieldRef,
- target_type: &DataType,
+ target_field: &FieldRef,
force_nullable: bool,
) -> Arc<Field> {
+ // 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();
+
Review Comment:
I agree this is a kludge (borrowed from the Physical cast)...I did this to
avoid a breaking change to matches over Expr. The `CastTarget` in #24725 I
think doesn't quite go far enough if we're going to introduce a breaking change
here...casts as they currently exist can change the DataType, nullability,
and/or metadata. Giving full explicit ness might be:
```rust
struct CastTarget {
pub data_type: DataType,
pub nullability: Option<bool>, // None means pass through existing
pub metadata: Option<MetadataTarget>,
}
enum MetadataTarget {
Merge(FieldMetadata),
Replace(FieldMetadata),
}
```
--
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]
