Skip to content

Conversation

scovich
Copy link
Contributor

@scovich scovich commented Sep 30, 2025

Which issue does this PR close?

Rationale for this change

The existing VariantAsPrimitive trait is kind of "backward" and requires very complex type bounds to use, e.g.:

impl<'a, T> VariantToPrimitiveArrowRowBuilder<'a, T>
where
    T: ArrowPrimitiveType,
    for<'m, 'v> Variant<'m, 'v>: VariantAsPrimitive<T>,

This is a code smell.

What changes are included in this PR?

"Reverse" the trait -- instead of extending Variant, the trait extends T: ArrowPrimitiveType. The resulting type bounds are much more intuitive:

impl<'a, T: PrimitiveFromVariant> VariantToPrimitiveArrowRowBuilder<'a, T>

Are these changes tested?

Existing unit tests cover this refactor.

Are there any user-facing changes?

No.

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

scovich commented Sep 30, 2025

CC @klion26

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.

looks good to me too -- thank you @scovich and @mbrobbel

@alamb alamb merged commit ef51f11 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