Skip to content

Conversation

klion26
Copy link
Member

@klion26 klion26 commented Sep 5, 2025

Which issue does this PR close?

Rationale for this change

Add the Variant::as_u* functions`

Are these changes tested?

Added doc tests

Are there any user-facing changes?

No

@github-actions github-actions bot added the parquet-variant parquet-variant* crates label Sep 5, 2025
@klion26
Copy link
Member Author

klion26 commented Sep 5, 2025

@alamb Please help review this when you're free, thanks.

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.

Thank you @klion26 -- looks good to me

Would it be possible to add some tests for converting Decimal as well?

/// let v1 = Variant::from(123i64);
/// assert_eq!(v1.as_u8(), Some(123u8));
///
/// // but not a variant that cant fit into the range
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// // but not a variant that cant fit into the range
/// // but not a variant that can't fit into the range

/// let v1 = Variant::from(123i64);
/// assert_eq!(v1.as_u16(), Some(123u16));
///
/// // but not a variant that cant fit into the range
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// // but not a variant that cant fit into the range
/// // but not a variant that can't fit into the range

/// let v1 = Variant::from(123i64);
/// assert_eq!(v1.as_u32(), Some(123u32));
///
/// // but not a variant that cant fit into the range
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// // but not a variant that cant fit into the range
/// // but not a variant that can't fit into the range

@klion26
Copy link
Member Author

klion26 commented Sep 5, 2025

@alamb thanks for the review, I've updated the code, please take another look.

@alamb
Copy link
Contributor

alamb commented Sep 5, 2025

Thanks @klion26 -- looks great to me

Copy link
Contributor

@scovich scovich left a comment

Choose a reason for hiding this comment

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

LGTM! The doc tests are a nice way to exercise the code (dual purpose)

@alamb alamb merged commit cd676cd into apache:main Sep 5, 2025
12 checks passed
@klion26 klion26 deleted the add_as_u branch September 8, 2025 02:06
@klion26
Copy link
Member Author

klion26 commented Sep 8, 2025

@alamb @scovich Thanks for the review and merging!

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.

[Variant] Add Vairant::as_u*
3 participants