Skip to content

Conversation

@hhhizzz
Copy link
Contributor

@hhhizzz hhhizzz commented Oct 20, 2025

Which issue does this PR close?

Rationale for this change

Related to #8607
We need to know how many encoding are support to create a decoder slot.

What changes are included in this PR?

Update the thrift_enum to know the fields count of enum Encoding, and the value is passed to EncodingMask And the ENCODING_SLOTS

Are these changes tested?

  1. Originally I think add a UT can prevent failure after the new encoding are introduced, then I realized the counts are already transferred, the UT is not required, the original tests can already cover the code.

Are there any user-facing changes?

No

@github-actions github-actions bot added the parquet Changes to the parquet crate label Oct 20, 2025
Copy link
Contributor

@etseidl etseidl left a comment

Choose a reason for hiding this comment

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

Thanks @hhhizzz, this looks like a nice addition. There are some hand coded enums in basic.rs (ConvertedType, Compression, EdgeInterpolationAlgorithm) that will also need to be modified to support the new constants. We'll also want to update parquet/THRIFT.md to reflect this change, but that can be a follow-on PR.

@hhhizzz
Copy link
Contributor Author

hhhizzz commented Oct 21, 2025

@etseidl Thanks, looks like there's some more change to update ConvertedType, Compression, EdgeInterpolationAlgorithm to use macro. I'll do some investigation and finish them in the following PR.

@etseidl
Copy link
Contributor

etseidl commented Oct 21, 2025

I just submitted #8680 so ConvertedType will benefit from this PR as well.

#[allow(deprecated)]
#[doc = "Returns a slice containing every variant of this enum."]
#[allow(dead_code)]
pub const VARIANTS: &'static [Self] = &[
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 quite cool

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 @hhhizzz and @etseidl

@alamb alamb merged commit 729b258 into apache:main Oct 24, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

parquet Changes to the parquet crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants