Skip to content

Encapsulate metadata for literals on to a FieldMetadata structure - #16317

Merged
alamb merged 4 commits into
apache:mainfrom
alamb:alamb/maybe_better_metadata
Jun 11, 2025
Merged

Encapsulate metadata for literals on to a FieldMetadata structure#16317
alamb merged 4 commits into
apache:mainfrom
alamb:alamb/maybe_better_metadata

Conversation

@alamb

@alambalamb commented Jun 7, 2025

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

As discussed on #16170 (comment), we made an initial PR to add metadata to Expr::Literal as part of DataFusion 48, but the API could use a bit of fine tuning to:

  1. Encapsulate the details more (so we don't have to migrated between different Map types, for example)
  2. Make cloning faster

What changes are included in this PR?

  1. Encapsulate the representation of metadata into a struct FieldMetadata
  2. Wrap the inner with an Arc to make it fast to clone

Are these changes tested?

Are there any user-facing changes?

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation sql SQL Planner logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates optimizer Optimizer rules core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) substrait Changes to the substrait crate catalog Related to the catalog crate proto Related to proto crate functions Changes to functions implementation datasource Changes to the datasource crate ffi Changes to the ffi crate labels Jun 7, 2025
@alamb
alambforce-pushed the alamb/maybe_better_metadata branch from 9752ed6 to a916b4cCompareJune 7, 2025 11:03
@github-actionsgithub-actionsBot removed documentation Improvements or additions to documentation sql SQL Planner sqllogictest SQL Logic Tests (.slt) substrait Changes to the substrait crate catalog Related to the catalog crate proto Related to proto crate functions Changes to functions implementation datasource Changes to the datasource crate ffi Changes to the ffi crate labels Jun 7, 2025
@alambalamb changed the title Alamb/maybe better metadataEncapsulate metadata for literals on to a FieldMetadata structureJun 7, 2025
@alamb
alamb marked this pull request as draft June 7, 2025 11:05
This was referenced Jun 7, 2025
Merged
Closed
@alamb

alamb commented Jun 7, 2025•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

I also found we can further unify the metadata handling for Expr::Alias as well, but would like to propose doing that as a follow on PR:

alamb
alamb commented Jun 7, 2025
View reviewed changes
Comment threaddatafusion/expr/src/expr.rs
/// A constant value along with associated metadata
Literal(ScalarValue, Option<BTreeMap<String, String>>),
/// A constant value along with associated [`FieldMetadata`].
Literal(ScalarValue, Option<FieldMetadata>),

@alambalambJun 7, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of this PR is to put all metadata handling in a Struct to make it easier to work with (and more efficient)

Comment threaddatafusion/expr/src/expr_rewriter/mod.rs
metadata
.map(|m| m.into_iter().collect::<HashMap<String, String>>()),
)))
Ok(Transformed::yes(lit_with_metadata(utf8_val, metadata)))

@alambalambJun 7, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Comment threaddatafusion/physical-expr/src/planner.rs
false => Some(new_metadata),
};

let metadata = metadata.as_ref().map(|m| FieldMetadata::from(m.clone()));

@alambalambJun 7, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remove this clone in a follow on PR:

@alamb
alamb marked this pull request as ready for review June 7, 2025 11:38
@alambalamb mentioned this pull request Jun 7, 2025
Merged
@alamb
alamb requested a review from timsaucerJune 7, 2025 11:49
timsaucer
timsaucer approved these changes Jun 7, 2025
View reviewed changes

@timsaucertimsaucer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. This is a very nice cleanup of the prior code.

Comment threaddatafusion/expr/src/expr.rs
@alamb
alamb merged commit e4166b3 into apache:mainJun 11, 2025
@alamb

alamb commented Jun 11, 2025

Copy link
Copy Markdown
ContributorAuthor

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)

@alamb
alamb deleted the alamb/maybe_better_metadata branch June 11, 2025 04:14
paleolimbot
paleolimbot reviewed Jun 12, 2025
View reviewed changes

@paleolimbotpaleolimbot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@timsaucertimsaucertimsaucer approved these changes
+1 more reviewer
@paleolimbotpaleolimbotpaleolimbot left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

coreCore DataFusion cratelogical-exprLogical plan and expressionsoptimizerOptimizer rulesphysical-exprChanges to the physical-expr crates

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@alamb@paleolimbot@timsaucer