Skip to content

Conversation

scovich
Copy link
Contributor

@scovich scovich commented Sep 29, 2025

Which issue does this PR close?

Rationale for this change

VariantValueArrayBuilder was already a bit inconvenient to work with, because it provides no implementation of VariantBuilderExt. Pathfinding for variant unshredding exposed this as an actual problem and not merely an inconvenience.

What changes are included in this PR?

Add a thin wrapper for VariantValueArrayBuilder for which we can impl VariantBuilderExt -- thus allowing the value builder to participate fully in the variant builder ecosystem.

Are these changes tested?

Yes, existing unit tests updated to use the new capability.

Are there any user-facing changes?

New pub method VariantValueArrayBuilder::as_builder_ext

@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

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.

Thanks @scovich -- this looks good to me other than the naming, which we can sort out as a follow on PR if necessary

let mut metadata_builder = ReadOnlyMetadataBuilder::new(value.metadata().clone());
let state = builder.parent_state(&mut metadata_builder);
ObjectBuilder::new(state, false)
let mut builder = value_builder.as_builder_ext(value.metadata().clone());
Copy link
Contributor

Choose a reason for hiding this comment

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

this is much nicer I agree

@alamb alamb merged commit 422da15 into apache:main Sep 29, 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.

2 participants