You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "etseidl (via GitHub)" <gi...@apache.org> on 2025/10/21 23:36:00 UTC
[PR] use macro for converted type [arrow-rs]
etseidl opened a new pull request, #8680:
URL: https://github.com/apache/arrow-rs/pull/8680
# Which issue does this PR close?
- Part of #5853.
# Rationale for this change
While converting to the new Thrift model, the `ConvertedType` enum was done manually due to the `NONE` variant, which used the discriminant of `0`. This PR changes that to `-1` which allows the `thrift_enum` macro 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [thrift-remodel] Use `thrift_enum` macro for `ConvertedType` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8680:
URL: https://github.com/apache/arrow-rs/pull/8680#issuecomment-3448419272
I created docs locally and verified that `ConvertedType` is indeed still pub and it looks like it is consistent with https://docs.rs/parquet/latest/parquet/basic/enum.ConvertedType.html
The only difference is (as expected) there are values assigned to each enum now
<img width="1920" height="1036" alt="Screenshot 2025-10-26 at 6 58 30 AM" src="https://github.com/user-attachments/assets/78c98015-a26d-47b2-977a-d68b62cb2377" />
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [thrift-remodel] Use `thrift_enum` macro for `ConvertedType` [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8680:
URL: https://github.com/apache/arrow-rs/pull/8680#discussion_r2460397853
##########
parquet/src/basic.rs:
##########
@@ -61,156 +61,112 @@ enum Type {
// ----------------------------------------------------------------------
// Mirrors thrift enum `ConvertedType`
-//
-// Cannot use macros because of added field `None`
// TODO(ets): Adding the `NONE` variant to this enum is a bit awkward. We should
-// look into removing it and using `Option<ConvertedType>` instead. Then all of this
-// handwritten code could go away.
-
+// look into removing it and using `Option<ConvertedType>` instead.
+thrift_enum!(
/// Common types (converted types) used by frameworks when using Parquet.
///
/// This helps map between types in those frameworks to the base types in Parquet.
/// This is only metadata and not needed to read or write the data.
///
/// 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)]
Review 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.
https://github.com/apache/arrow-rs/blob/d519bb800340fa1a5e2601ae51cba82be3a7aa4b/parquet/src/parquet_macros.rs#L46
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [thrift-remodel] Use `thrift_enum` macro for `ConvertedType` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8680:
URL: https://github.com/apache/arrow-rs/pull/8680#discussion_r2457139510
##########
parquet/src/basic.rs:
##########
@@ -61,156 +61,112 @@ enum Type {
// ----------------------------------------------------------------------
// Mirrors thrift enum `ConvertedType`
-//
-// Cannot use macros because of added field `None`
// TODO(ets): Adding the `NONE` variant to this enum is a bit awkward. We should
-// look into removing it and using `Option<ConvertedType>` instead. Then all of this
-// handwritten code could go away.
-
+// look into removing it and using `Option<ConvertedType>` instead.
+thrift_enum!(
/// Common types (converted types) used by frameworks when using Parquet.
///
/// This helps map between types in those frameworks to the base types in Parquet.
/// This is only metadata and not needed to read or write the data.
///
/// 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)]
Review 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 🤔
##########
parquet/src/basic.rs:
##########
@@ -61,156 +61,112 @@ enum Type {
// ----------------------------------------------------------------------
// Mirrors thrift enum `ConvertedType`
-//
-// Cannot use macros because of added field `None`
// TODO(ets): Adding the `NONE` variant to this enum is a bit awkward. We should
-// look into removing it and using `Option<ConvertedType>` instead. Then all of this
-// handwritten code could go away.
-
+// look into removing it and using `Option<ConvertedType>` instead.
+thrift_enum!(
/// Common types (converted types) used by frameworks when using Parquet.
///
/// This helps map between types in those frameworks to the base types in Parquet.
/// This is only metadata and not needed to read or write the data.
///
/// 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)]
-#[allow(non_camel_case_types)]
-pub enum ConvertedType {
- /// No type conversion.
- NONE,
- /// A BYTE_ARRAY actually contains UTF8 encoded chars.
- UTF8,
-
- /// A map is converted as an optional field containing a repeated key/value pair.
- MAP,
-
- /// A key/value pair is converted into a group of two fields.
- MAP_KEY_VALUE,
-
- /// A list is converted into an optional field containing a repeated field for its
- /// values.
- LIST,
-
- /// An enum is converted into a binary field
- ENUM,
-
- /// A decimal value.
- /// This may be used to annotate binary or fixed primitive types. The
- /// underlying byte array stores the unscaled value encoded as two's
- /// complement using big-endian byte order (the most significant byte is the
- /// zeroth element).
- ///
- /// This must be accompanied by a (maximum) precision and a scale in the
- /// SchemaElement. The precision specifies the number of digits in the decimal
- /// and the scale stores the location of the decimal point. For example 1.23
- /// would have precision 3 (3 total digits) and scale 2 (the decimal point is
- /// 2 digits over).
- DECIMAL,
+enum ConvertedType {
+ /// Not defined in the spec, used internally to indicate no type conversion
+ NONE = -1;
- /// A date stored as days since Unix epoch, encoded as the INT32 physical type.
- DATE,
+ /// A BYTE_ARRAY actually contains UTF8 encoded chars.
+ UTF8 = 0;
- /// The total number of milliseconds since midnight. The value is stored as an INT32
- /// physical type.
- TIME_MILLIS,
+ /// A map is converted as an optional field containing a repeated key/value pair.
+ MAP = 1;
- /// The total number of microseconds since midnight. The value is stored as an INT64
- /// physical type.
- TIME_MICROS,
+ /// A key/value pair is converted into a group of two fields.
+ MAP_KEY_VALUE = 2;
- /// Date and time recorded as milliseconds since the Unix epoch.
- /// Recorded as a physical type of INT64.
- TIMESTAMP_MILLIS,
+ /// A list is converted into an optional field containing a repeated field for its
+ /// values.
+ LIST = 3;
- /// Date and time recorded as microseconds since the Unix epoch.
- /// The value is stored as an INT64 physical type.
- TIMESTAMP_MICROS,
+ /// An enum is converted into a BYTE_ARRAY field
+ ENUM = 4;
- /// An unsigned 8 bit integer value stored as INT32 physical type.
- UINT_8,
+ /// A decimal value.
+ ///
+ /// This may be used to annotate BYTE_ARRAY or FIXED_LEN_BYTE_ARRAY primitive
+ /// types. The underlying byte array stores the unscaled value encoded as two's
+ /// complement using big-endian byte order (the most significant byte is the
+ /// zeroth element). The value of the decimal is the value * 10^{-scale}.
+ ///
+ /// This must be accompanied by a (maximum) precision and a scale in the
+ /// SchemaElement. The precision specifies the number of digits in the decimal
+ /// and the scale stores the location of the decimal point. For example 1.23
+ /// would have precision 3 (3 total digits) and scale 2 (the decimal point is
+ /// 2 digits over).
+ DECIMAL = 5;
- /// An unsigned 16 bit integer value stored as INT32 physical type.
- UINT_16,
+ /// A date stored as days since Unix epoch, encoded as the INT32 physical type.
+ DATE = 6;
- /// An unsigned 32 bit integer value stored as INT32 physical type.
- UINT_32,
+ /// The total number of milliseconds since midnight. The value is stored as an INT32
+ /// physical type.
+ TIME_MILLIS = 7;
- /// An unsigned 64 bit integer value stored as INT64 physical type.
- UINT_64,
+ /// The total number of microseconds since midnight. The value is stored as an INT64
+ /// physical type.
+ TIME_MICROS = 8;
- /// A signed 8 bit integer value stored as INT32 physical type.
- INT_8,
+ /// Date and time recorded as milliseconds since the Unix epoch.
+ /// Recorded as a physical type of INT64.
+ TIMESTAMP_MILLIS = 9;
- /// A signed 16 bit integer value stored as INT32 physical type.
- INT_16,
+ /// Date and time recorded as microseconds since the Unix epoch.
+ /// The value is stored as an INT64 physical type.
+ TIMESTAMP_MICROS = 10;
- /// A signed 32 bit integer value stored as INT32 physical type.
- INT_32,
+ /// An unsigned 8 bit integer value stored as INT32 physical type.
+ UINT_8 = 11;
- /// A signed 64 bit integer value stored as INT64 physical type.
- INT_64,
+ /// An unsigned 16 bit integer value stored as INT32 physical type.
+ UINT_16 = 12;
- /// A JSON document embedded within a single UTF8 column.
- JSON,
+ /// An unsigned 32 bit integer value stored as INT32 physical type.
+ UINT_32 = 13;
- /// A BSON document embedded within a single BINARY column.
- BSON,
+ /// An unsigned 64 bit integer value stored as INT64 physical type.
+ UINT_64 = 14;
- /// An interval of time.
- ///
- /// This type annotates data stored as a FIXED_LEN_BYTE_ARRAY of length 12.
- /// This data is composed of three separate little endian unsigned integers.
- /// Each stores a component of a duration of time. The first integer identifies
- /// the number of months associated with the duration, the second identifies
- /// the number of days associated with the duration and the third identifies
- /// the number of milliseconds associated with the provided duration.
- /// This duration of time is independent of any particular timezone or date.
- INTERVAL,
-}
+ /// A signed 8 bit integer value stored as INT32 physical type.
+ INT_8 = 15;
-impl<'a, R: ThriftCompactInputProtocol<'a>> ReadThrift<'a, R> for ConvertedType {
- fn read_thrift(prot: &mut R) -> Result<Self> {
- let val = prot.read_i32()?;
- Ok(match val {
- 0 => Self::UTF8,
- 1 => Self::MAP,
- 2 => Self::MAP_KEY_VALUE,
- 3 => Self::LIST,
- 4 => Self::ENUM,
- 5 => Self::DECIMAL,
- 6 => Self::DATE,
- 7 => Self::TIME_MILLIS,
- 8 => Self::TIME_MICROS,
- 9 => Self::TIMESTAMP_MILLIS,
- 10 => Self::TIMESTAMP_MICROS,
- 11 => Self::UINT_8,
- 12 => Self::UINT_16,
- 13 => Self::UINT_32,
- 14 => Self::UINT_64,
- 15 => Self::INT_8,
- 16 => Self::INT_16,
- 17 => Self::INT_32,
- 18 => Self::INT_64,
- 19 => Self::JSON,
- 20 => Self::BSON,
- 21 => Self::INTERVAL,
- _ => return Err(general_err!("Unexpected ConvertedType {}", val)),
- })
- }
-}
+ /// A signed 16 bit integer value stored as INT32 physical type.
+ INT_16 = 16;
-impl WriteThrift for ConvertedType {
- const ELEMENT_TYPE: ElementType = ElementType::I32;
+ /// A signed 32 bit integer value stored as INT32 physical type.
+ INT_32 = 17;
- fn write_thrift<W: Write>(&self, writer: &mut ThriftCompactOutputProtocol<W>) -> Result<()> {
- // because we've added NONE, the variant values are off by 1, so correct that here
- writer.write_i32(*self as i32 - 1)
- }
-}
+ /// A signed 64 bit integer value stored as INT64 physical type.
+ INT_64 = 18;
+
+ /// A JSON document embedded within a single UTF8 column.
+ JSON = 19;
-write_thrift_field!(ConvertedType, FieldType::I32);
+ /// A BSON document embedded within a single BINARY column.
+ BSON = 20;
+
+ /// An interval of time
Review Comment:
this certainly looks a lot nicer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [thrift-remodel] Use `thrift_enum` macro for `ConvertedType` [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8680:
URL: https://github.com/apache/arrow-rs/pull/8680#discussion_r2460359722
##########
parquet/src/basic.rs:
##########
@@ -61,156 +61,112 @@ enum Type {
// ----------------------------------------------------------------------
// Mirrors thrift enum `ConvertedType`
-//
-// Cannot use macros because of added field `None`
// TODO(ets): Adding the `NONE` variant to this enum is a bit awkward. We should
-// look into removing it and using `Option<ConvertedType>` instead. Then all of this
-// handwritten code could go away.
-
+// look into removing it and using `Option<ConvertedType>` instead.
+thrift_enum!(
/// Common types (converted types) used by frameworks when using Parquet.
///
/// This helps map between types in those frameworks to the base types in Parquet.
/// This is only metadata and not needed to read or write the data.
///
/// 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)]
Review Comment:
> this is a breaking API change I think given that `ConvertedType` is 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [thrift-remodel] Use `thrift_enum` macro for `ConvertedType` [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #8680:
URL: https://github.com/apache/arrow-rs/pull/8680#discussion_r2460397853
##########
parquet/src/basic.rs:
##########
@@ -61,156 +61,112 @@ enum Type {
// ----------------------------------------------------------------------
// Mirrors thrift enum `ConvertedType`
-//
-// Cannot use macros because of added field `None`
// TODO(ets): Adding the `NONE` variant to this enum is a bit awkward. We should
-// look into removing it and using `Option<ConvertedType>` instead. Then all of this
-// handwritten code could go away.
-
+// look into removing it and using `Option<ConvertedType>` instead.
+thrift_enum!(
/// Common types (converted types) used by frameworks when using Parquet.
///
/// This helps map between types in those frameworks to the base types in Parquet.
/// This is only metadata and not needed to read or write the data.
///
/// 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)]
Review 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.
https://github.com/apache/arrow-rs/blob/d519bb800340fa1a5e2601ae51cba82be3a7aa4b/parquet/src/parquet_macros.rs#L46
Edit: I guess that fact could be better documented
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [thrift-remodel] Use `thrift_enum` macro for `ConvertedType` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8680:
URL: https://github.com/apache/arrow-rs/pull/8680#discussion_r2460384082
##########
parquet/src/basic.rs:
##########
@@ -61,156 +61,112 @@ enum Type {
// ----------------------------------------------------------------------
// Mirrors thrift enum `ConvertedType`
-//
-// Cannot use macros because of added field `None`
// TODO(ets): Adding the `NONE` variant to this enum is a bit awkward. We should
-// look into removing it and using `Option<ConvertedType>` instead. Then all of this
-// handwritten code could go away.
-
+// look into removing it and using `Option<ConvertedType>` instead.
+thrift_enum!(
/// Common types (converted types) used by frameworks when using Parquet.
///
/// This helps map between types in those frameworks to the base types in Parquet.
/// This is only metadata and not needed to read or write the data.
///
/// 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)]
Review Comment:
> I like the way you think 😄. I think it's long overdue to standardize on logical type and stop using converted type altogether.
💯
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [thrift-remodel] Use `thrift_enum` macro for `ConvertedType` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8680:
URL: https://github.com/apache/arrow-rs/pull/8680
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [thrift-remodel] Use `thrift_enum` macro for `ConvertedType` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8680:
URL: https://github.com/apache/arrow-rs/pull/8680#discussion_r2460381795
##########
parquet/src/basic.rs:
##########
@@ -61,156 +61,112 @@ enum Type {
// ----------------------------------------------------------------------
// Mirrors thrift enum `ConvertedType`
-//
-// Cannot use macros because of added field `None`
// TODO(ets): Adding the `NONE` variant to this enum is a bit awkward. We should
-// look into removing it and using `Option<ConvertedType>` instead. Then all of this
-// handwritten code could go away.
-
+// look into removing it and using `Option<ConvertedType>` instead.
+thrift_enum!(
/// Common types (converted types) used by frameworks when using Parquet.
///
/// This helps map between types in those frameworks to the base types in Parquet.
/// This is only metadata and not needed to read or write the data.
///
/// 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)]
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org