gene-bordegaray commented on code in PR #23169:
URL: https://github.com/apache/datafusion/pull/23169#discussion_r3873590286
##########
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 also had this check originally in my PR but changed my apporach because
this is fragile and not very explicit for new readers of this code
Say you had this:
```rust
// Type only target
Cast::new(expr.clone(), DataType::FixedSizeBinary(16));
// Purpsely empty target
Cast::new_from_field(
expr,
Arc::new(Field::new("", DataType::FixedSizeBinary(16), true)),
);
```
So now both produce a blank, nullable field with empty metadata because it
identifies as type-only
The approach here #24725 avoids this but its more invasive because change
the pub Cast.field type and a protobuf field
--
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]
