[thrift-remodel] Use thrift_enum macro for ConvertedType
#8680
Conversation
etseidl
changed the title
thrift_enum macro for ConvertedType
| /// | ||
| /// This struct was renamed from `LogicalType` in version 4.0.0. | ||
| /// If targeting Parquet format 2.4.0 or above, please use [LogicalType] instead. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] |
There was a problem hiding this comment.
this is a breaking API change I think given that ConvertedType is pub: https://docs.rs/parquet/latest/parquet/basic/enum.ConvertedType.html
Maybe to avoid the breaking change, we could deprecate ConvertedType instead 🤔
There was a problem hiding this comment.
this is a breaking API change I think given that
ConvertedTypeis pub: https://docs.rs/parquet/latest/parquet/basic/enum.ConvertedType.html
I'm just curious as to what makes this a breaking change? Adding derived impls or changing the discriminants to match the spec? I didn't think either would qualify.
Maybe to avoid the breaking change, we could deprecate ConvertedType instead 🤔
I like the way you think 😄. I think it's long overdue to standardize on logical type and stop using converted type altogether.
There was a problem hiding this comment.
I'm just curious as to what makes this a breaking change? Adding derived impls or changing the discriminants to match the spec? I didn't think either would qualify.
My reading of the diff of this PR is that it removes pub enum ConvertedType, which is a public struct
Thus if anyone has code that refers to ConvertedType, removing it would cause their code to stop compiling
There was a problem hiding this comment.
I like the way you think 😄. I think it's long overdue to standardize on logical type and stop using converted type altogether.
💯
There was a problem hiding this comment.
My reading of the diff of this PR is that it removes
pub enum ConvertedType, which is a public struct
Ahh. The thrift_enum macro makes all enums pub, so that doesn't change.
arrow-rs/parquet/src/parquet_macros.rs
Line 46 in d519bb8
Edit: I guess that fact could be better documented
| /// A BSON document embedded within a single BINARY column. | ||
| BSON = 20; | ||
|
|
||
| /// An interval of time |
There was a problem hiding this comment.
this certainly looks a lot nicer
# Which issue does this PR close? N/A # Rationale for this change It is not obvious that the thrift macros produce public enums only (e.g. see #8680 (comment)). This should be made clear in the documentation. # What changes are included in this PR? Add said clarification. # Are these changes tested? Documentation only, so no tests required. # Are there any user-facing changes? No, only changes to private documentation
2 participants
Which issue does this PR close?
Rationale for this change
While converting to the new Thrift model, the
ConvertedTypeenum was done manually due to theNONEvariant, which used the discriminant of0. This PR changes that to-1which allows thethrift_enummacro to be used instead. This improves code maintainability.What changes are included in this PR?
See above.
Are these changes tested?
Covered by existing tests
Are there any user-facing changes?
No, this only changes the discriminant value for a unit variant enum.