paleolimbot commented on code in PR #24865:
URL: https://github.com/apache/datafusion/pull/24865#discussion_r4125755568
##########
datafusion/common/src/metadata.rs:
##########
@@ -392,3 +422,41 @@ impl From<&HashMap<String, String>> for FieldMetadata {
}
}
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ fn field_with(name: &str, dt: DataType, pairs: &[(&str, &str)]) -> Field {
+ let metadata: std::collections::HashMap<String, String> = pairs
+ .iter()
+ .map(|(k, v)| (k.to_string(), v.to_string()))
+ .collect();
+ Field::new(name, dt, true).with_metadata(metadata)
+ }
Review Comment:
This may already be the case, but if there is any precedent to draw from on
this one with respect to creating a field with some metadata for a test in
another file it would be good to use it here. I usually omit the helper because
`Field::new().with_metadata()` is sufficiently compact.
##########
datafusion/functions/src/core/coalesce.rs:
##########
@@ -146,3 +154,113 @@ impl ScalarUDFImpl for CoalesceFunc {
self.doc()
}
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use std::collections::HashMap;
+ use std::sync::Arc;
+
+ const EXTENSION_KEY: &str = "ARROW:extension:name";
+
+ fn field(name: &str, dt: DataType, nullable: bool) -> FieldRef {
+ Arc::new(Field::new(name, dt, nullable))
+ }
+
+ fn ext_field(name: &str, dt: DataType, extension_name: &str) -> FieldRef {
+ Arc::new(Field::new(name, dt, true).with_metadata(HashMap::from([(
+ EXTENSION_KEY.to_string(),
+ extension_name.to_string(),
+ )])))
+ }
+
+ fn return_field(arg_fields: &[FieldRef]) -> FieldRef {
+ let scalars = vec![None; arg_fields.len()];
+ CoalesceFunc::new()
+ .return_field_from_args(ReturnFieldArgs {
+ arg_fields,
+ scalar_arguments: &scalars,
+ })
+ .unwrap()
+ }
Review Comment:
Same note on consistency of helpers (maybe the same style within the PR if
there's nothing to draw on nearby)
##########
datafusion/common/src/metadata.rs:
##########
@@ -117,6 +117,36 @@ 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`.
+///
+/// Returns the first non-empty metadata among the fields whose data type is
+/// not `Null` (an untyped NULL literal carries no metadata and cannot
+/// contribute a typed value), or empty metadata if there is none.
+///
+/// The inputs are deliberately not required to agree on their metadata:
+/// byte-wise comparison is too strict for extension types whose parameters
+/// are JSON-encoded, and unrelated 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. Engines
+/// that want stricter behavior (e.g. rejecting conflicting extension types)
+/// can enforce it with an analyzer or optimizer rule, which can only observe
+/// the conflict if planning preserves the metadata in the first place.
+pub fn coerced_fields_metadata<'a>(
+ mut fields: impl Iterator<Item = &'a Field>,
Review Comment:
I often see this as `IntoIterator<Item = ...>`...would that be more
appropriate here?
##########
datafusion/common/src/metadata.rs:
##########
@@ -117,6 +117,36 @@ 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`.
+///
+/// Returns the first non-empty metadata among the fields whose data type is
+/// not `Null` (an untyped NULL literal carries no metadata and cannot
+/// contribute a typed value), or empty metadata if there is none.
Review Comment:
Another strategy used in some other places (e.g., cast, as of recently) is
to merge, but refuse to merge differing values of `ARROW:extension:name`. I
don't mind either way, but maybe that would be a good compromise between the
initial stricter version of this?
--
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]
