paleolimbot commented on code in PR #24865:
URL: https://github.com/apache/datafusion/pull/24865#discussion_r4396516244
##########
datafusion/common/src/metadata.rs:
##########
@@ -117,6 +118,75 @@ pub fn check_metadata_with_storage_equal(
Ok(())
}
+/// Compute the field metadata to propagate for an expression whose result
+/// may come from any one of several inputs coerced to a common type, such as
+/// `COALESCE`, `NVL2`, or `CASE WHEN`.
+///
+/// Metadata is merged across the fields whose data type is not `Null` (an
+/// untyped NULL literal carries no metadata and cannot contribute a typed
+/// value), with earlier fields taking precedence for repeated keys. Unrelated
+/// keys are not required to agree: metadata that arrived with the data (e.g.
+/// from an Arrow file or an embedded Arrow schema in a Parquet file) must not
+/// cause an extension type to be silently dropped from the result.
+///
+/// The Arrow extension type keys are handled as a unit, like in
+/// [`Expr::Cast`] output field derivation:
+/// - Differing values of `ARROW:extension:name` produce a planning error;
+/// the inputs are of different logical types even though they were coerced
+/// to a common storage type.
+/// - `ARROW:extension:metadata` is taken from the field that supplied the
+/// extension name, never combined from another field. Byte-wise differences
+/// in the extension metadata of fields that agree on the extension name are
+/// tolerated, since extension parameters are often JSON-encoded, where byte
+/// equality is too strict.
+///
+/// [`Expr::Cast`]:
https://docs.rs/datafusion/latest/datafusion/logical_expr/enum.Expr.html#variant.Cast
+pub fn coerced_fields_metadata<'a>(
Review Comment:
I recently discovered `intersect_metadata_for_union()`, which silently
discards non-matching metadata keys. I like this version better (I want a
louder error when the types don't match), but either is OK with me if we want
to keep the existing logic for consistency.
##########
datafusion/functions/src/core/coalesce.rs:
##########
@@ -86,7 +87,14 @@ impl ScalarUDFImpl for CoalesceFunc {
.find_or_first(|d| !d.is_null())
.unwrap()
.clone();
- Ok(Field::new(self.name(), return_type, nullable).into())
+ // Propagate field metadata (e.g. Arrow extension types) from the
+ // arguments that can supply a value; see `coerced_fields_metadata`
+ // for the merge semantics.
+ let metadata =
+ coerced_fields_metadata(args.arg_fields.iter().map(|f|
f.as_ref()))?;
+ Ok(metadata
+ .add_to_field(Field::new(self.name(), return_type, nullable))
+ .into())
Review Comment:
Happy to hear another opinion, but I do think this is a bug worth fixing
regardless of whether https://github.com/apache/datafusion/pull/26145 is a good
idea.
--
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]
