Skip to content

Conversation

scovich
Copy link
Contributor

@scovich scovich commented Sep 29, 2025

Which issue does this PR close?

Rationale for this change

While pathfinding variant unshredding code, I noticed that there's a lot of variant metadata cloning going on. It's a 32-byte object, so cloning is not free. And it turns out that ~all read-only metadata builder usage in the code base can work just as easily with a borrowed reference because ultimately we're working with bytes borrowed from a binary view that holds the metadata column.

What changes are included in this PR?

Update the ReadOnlyMetadataBuilder to borrow the VariantMetadata it uses, and update call sites accordingly.

Are these changes tested?

Yes, the change affects several unit tests.

Are there any user-facing changes?

Signature of ReadOnlyMetadataBuilder::new has changed.

@github-actions github-actions bot added the parquet-variant parquet-variant* crates label Sep 29, 2025
@scovich
Copy link
Contributor Author

scovich commented Sep 29, 2025

CC @alamb -- should be a quickie, assuming you agree with the premise.

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

makes sense to me -- thank @scovich

@scovich scovich force-pushed the read-only-metadata-references branch from 8b4fe52 to d3590bb Compare September 29, 2025 20:53
@scovich
Copy link
Contributor Author

scovich commented Sep 29, 2025

Upstream conflicts resolved, should be ready for merge now.

@mbrobbel mbrobbel merged commit 686d085 into apache:main Sep 30, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parquet-variant parquet-variant* crates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants