Re: [PR] Encapsulate metadata for literals on to a `FieldMetadata` structure [datafusion]
alamb commented on PR #16317: URL: https://github.com/apache/datafusion/pull/16317#issuecomment-2961187091 Here is a follow on PR to also use the same structure in `Expr::Alias` (which also had metadata, but was stored as a HashMap) - https://github.com/apache/datafusion/pull/16320/files -- 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]
Re: [PR] Encapsulate metadata for literals on to a `FieldMetadata` structure [datafusion]
alamb merged PR #16317: URL: https://github.com/apache/datafusion/pull/16317 -- 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]
Re: [PR] Encapsulate metadata for literals on to a `FieldMetadata` structure [datafusion]
alamb commented on code in PR #16317:
URL: https://github.com/apache/datafusion/pull/16317#discussion_r2134620674
##
datafusion/expr/src/expr.rs:
##
@@ -413,6 +413,162 @@ impl<'a> TreeNodeContainer<'a, Self> for Expr {
}
}
+/// Literal metadata
+///
+/// Stores metadata associated with a literal expressions
+/// and is designed to be fast to `clone`.
+///
+/// This structure is used to store metadata associated with a literal
expression, and it
+/// corresponds to the `metadata` field on [`Field`].
+///
+/// # Example: Create [`FieldMetadata`] from a [`Field`]
+/// ```
+/// # use std::collections::HashMap;
+/// # use datafusion_expr::expr::FieldMetadata;
+/// # use arrow::datatypes::{Field, DataType};
+/// # let field = Field::new("c1", DataType::Int32, true)
+/// # .with_metadata(HashMap::from([("foo".to_string(), "bar".to_string())]));
+/// // Create a new `FieldMetadata` instance from a `Field`
+/// let metadata = FieldMetadata::new_from_field(&field);
+/// // There is also a `From` impl:
+/// let metadata = FieldMetadata::from(&field);
+/// ```
+///
+/// # Example: Update a [`Field`] with [`FieldMetadata`]
+/// ```
+/// # use datafusion_expr::expr::FieldMetadata;
+/// # use arrow::datatypes::{Field, DataType};
+/// # let field = Field::new("c1", DataType::Int32, true);
+/// # let metadata = FieldMetadata::new_from_field(&field);
+/// // Add any metadata from `FieldMetadata` to `Field`
+/// let updated_field = metadata.add_to_field(field);
+/// ```
+///
+#[derive(Clone, PartialEq, Eq, PartialOrd, Hash, Debug)]
+pub struct FieldMetadata {
+/// The inner metadata of a literal expression, which is a map of string
+/// keys to string values.
+///
+/// Note this is not a `HashMap because `HashMap` does not provide
+/// implementations for traits like `Debug` and `Hash`.
+inner: Arc>,
+}
+
+impl FieldMetadata {
+/// Create a new empty metadata instance.
+pub fn new_empty() -> Self {
+Self {
+inner: Arc::new(BTreeMap::new()),
+}
+}
Review Comment:
Done in
https://github.com/apache/datafusion/pull/16317/commits/c2759e9b06f352d40e6f13ded3f1e9094095ee62
--
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]
Re: [PR] Encapsulate metadata for literals on to a `FieldMetadata` structure [datafusion]
timsaucer commented on code in PR #16317:
URL: https://github.com/apache/datafusion/pull/16317#discussion_r2133859411
##
datafusion/expr/src/expr.rs:
##
@@ -413,6 +413,162 @@ impl<'a> TreeNodeContainer<'a, Self> for Expr {
}
}
+/// Literal metadata
+///
+/// Stores metadata associated with a literal expressions
+/// and is designed to be fast to `clone`.
+///
+/// This structure is used to store metadata associated with a literal
expression, and it
+/// corresponds to the `metadata` field on [`Field`].
+///
+/// # Example: Create [`FieldMetadata`] from a [`Field`]
+/// ```
+/// # use std::collections::HashMap;
+/// # use datafusion_expr::expr::FieldMetadata;
+/// # use arrow::datatypes::{Field, DataType};
+/// # let field = Field::new("c1", DataType::Int32, true)
+/// # .with_metadata(HashMap::from([("foo".to_string(), "bar".to_string())]));
+/// // Create a new `FieldMetadata` instance from a `Field`
+/// let metadata = FieldMetadata::new_from_field(&field);
+/// // There is also a `From` impl:
+/// let metadata = FieldMetadata::from(&field);
+/// ```
+///
+/// # Example: Update a [`Field`] with [`FieldMetadata`]
+/// ```
+/// # use datafusion_expr::expr::FieldMetadata;
+/// # use arrow::datatypes::{Field, DataType};
+/// # let field = Field::new("c1", DataType::Int32, true);
+/// # let metadata = FieldMetadata::new_from_field(&field);
+/// // Add any metadata from `FieldMetadata` to `Field`
+/// let updated_field = metadata.add_to_field(field);
+/// ```
+///
+#[derive(Clone, PartialEq, Eq, PartialOrd, Hash, Debug)]
+pub struct FieldMetadata {
+/// The inner metadata of a literal expression, which is a map of string
+/// keys to string values.
+///
+/// Note this is not a `HashMap because `HashMap` does not provide
+/// implementations for traits like `Debug` and `Hash`.
+inner: Arc>,
+}
+
+impl FieldMetadata {
+/// Create a new empty metadata instance.
+pub fn new_empty() -> Self {
+Self {
+inner: Arc::new(BTreeMap::new()),
+}
+}
Review Comment:
minor: Could instead impl `Default`
--
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]
Re: [PR] Encapsulate metadata for literals on to a `FieldMetadata` structure [datafusion]
alamb commented on code in PR #16317:
URL: https://github.com/apache/datafusion/pull/16317#discussion_r2133768464
##
datafusion/expr/src/expr_rewriter/mod.rs:
##
@@ -390,11 +390,7 @@ mod test {
} else {
utf8_val
};
-Ok(Transformed::yes(lit_with_metadata(
-utf8_val,
-metadata
-.map(|m| m.into_iter().collect::>()),
-)))
+Ok(Transformed::yes(lit_with_metadata(utf8_val, metadata)))
Review Comment:
this is a pretty good example of the kind of simplification this PR allows
(don't have to translate back/forth between HashMap / BTreeMap in various
places)
##
datafusion/expr/src/expr.rs:
##
@@ -284,8 +284,8 @@ pub enum Expr {
Column(Column),
/// A named reference to a variable in a registry.
ScalarVariable(DataType, Vec),
-/// A constant value along with associated metadata
-Literal(ScalarValue, Option>),
+/// A constant value along with associated [`FieldMetadata`].
+Literal(ScalarValue, Option),
Review Comment:
The point of this PR is to put all metadata handling in a Struct to make it
easier to work with (and more efficient)
##
datafusion/physical-expr/src/planner.rs:
##
@@ -114,22 +115,11 @@ pub fn create_physical_expr(
match e {
Expr::Alias(Alias { expr, metadata, .. }) => {
if let Expr::Literal(v, prior_metadata) = expr.as_ref() {
-let mut new_metadata = prior_metadata
-.as_ref()
-.map(|m| {
-m.iter()
-.map(|(k, v)| (k.clone(), v.clone()))
-.collect::>()
-})
-.unwrap_or_default();
-if let Some(metadata) = metadata {
-new_metadata.extend(metadata.clone());
-}
-let new_metadata = match new_metadata.is_empty() {
-true => None,
-false => Some(new_metadata),
-};
-
+let metadata = metadata.as_ref().map(|m|
FieldMetadata::from(m.clone()));
Review Comment:
I remove this clone in a follow on PR:
- https://github.com/apache/datafusion/pull/16320
--
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]
Re: [PR] Encapsulate metadata for literals on to a `FieldMetadata` structure [datafusion]
alamb commented on PR #16317: URL: https://github.com/apache/datafusion/pull/16317#issuecomment-2952383640 I also found we can further unify the metadata handling for Expr::Alias as well, see - https://github.com/apache/datafusion/pull/16320 -- 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]
