[Variant] Minor: make fields in `VariantDecimal*` private, add examples #7770

alamb · 2025-06-24T18:22:36Z

Which issue does this PR close?

We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax.

part of [EPIC] [Parquet] Implement Variant type support in Parquet #6736

Rationale for this change

I noticed this while reviewing @Weijun-H's PR

feat: [Variant] Add Validation for Variant Deciaml #7738

I didn't know what part of the crate were using the fields directly and thus what part could potentially be creating invalid variants. By making the fields non pub(crate) I think it makes it clear these can not be constructed invalidly

This also gave me a good excuse to write some more examples

What changes are included in this PR?

make fields in VariantDecimal* private
Add doc examples examples

Are these changes tested?

By CI

Are there any user-facing changes?

API change in a non-published crate, so no end user impact yet

friendlymatthew · 2025-06-24T19:12:27Z

Hi, I wonder if we should do the same for ShortString:

arrow-rs/parquet-variant/src/variant.rs

Lines 36 to 41 in 121371c

    
           /// A Variant [`ShortString`] 
        
           /// 
        
           /// This implementation is a zero cost wrapper over `&str` that ensures 
        
           /// the length of the underlying string is a valid Variant short string (63 bytes or less) 
        
           #[derive(Debug, Clone, Copy, PartialEq)] 
        
           pub struct ShortString<'a>(pub(crate) &'a str);

alamb · 2025-06-24T19:13:47Z

Hi, I wonder if we should do the same for ShortString:

Good idea, i will do so

Weijun-H

LGTM! Thanks @alamb

alamb · 2025-06-24T21:13:50Z

Thanks @Weijun-H and @friendlymatthew -- I think this PR will cause some non trivial logical conflicts so I will postpone merging it until we get the initial JSON conversion in

…cimal

alamb · 2025-06-27T11:37:41Z

parquet-variant/src/variant.rs

                VariantPrimitiveType::Decimal4 => {
                    let (integer, scale) = decoder::decode_decimal4(value_data)?;
-                    Variant::Decimal4(VariantDecimal4 { integer, scale })
+                    Variant::Decimal4(VariantDecimal4::try_new(integer, scale)?)


I think technically constructing variants from binary values did did not verify the integer/scale were within range, but now they do after this PR

I think you're right. Good catch!

alamb · 2025-06-27T11:38:34Z

I think this one is now ready to go -- let me know if you would like time to review @scovich or @friendlymatthew or @superserious-dev

I am done with variant for this morning, and will hopefully return this evening

friendlymatthew

LGTM

scovich

Nice change! There's a question about ergonomics and correctness gaps this change uncovers, but maybe they should be addressed as a follow-up PR?

scovich · 2025-06-27T17:11:32Z

parquet-variant/src/variant.rs

                VariantPrimitiveType::Decimal4 => {
                    let (integer, scale) = decoder::decode_decimal4(value_data)?;
-                    Variant::Decimal4(VariantDecimal4 { integer, scale })
+                    Variant::Decimal4(VariantDecimal4::try_new(integer, scale)?)


I think you're right. Good catch!

scovich · 2025-06-27T17:13:14Z

parquet-variant/src/variant.rs

+                if let Ok(converted_integer) = decimal8.integer().try_into() {
+                    Some((converted_integer, decimal8.scale()))
                } else {
                    None
                }


Isn't this just

let converted_integer = decimal8.integer().try_into().ok()?; Some((converted_integer, decimal8.scale()))

or even

decimal8.integer().try_into().ok().map(|i| (i, decimal8.scale()))

(more below)

(but see other comment -- scale also needs to be checked)

scovich · 2025-06-27T17:33:12Z

parquet-variant/src/variant.rs

-                if let Ok(converted_integer) = decimal16.integer.try_into() {
-                    Some((converted_integer, decimal16.scale))
+                if let Ok(converted_integer) = decimal16.integer().try_into() {
+                    Some((converted_integer, decimal16.scale()))


We need to validate scale as well, because scale cannot be larger than precision:
decimal4 max precision is 9, while decimal16 max precision is 38.

scovich · 2025-06-27T17:51:59Z

parquet-variant/src/variant.rs

    pub fn as_decimal_int64(&self) -> Option<(i64, u8)> {
        match *self {
-            Variant::Decimal4(decimal) => Some((decimal.integer.into(), decimal.scale)),
-            Variant::Decimal8(decimal) => Some((decimal.integer, decimal.scale)),
+            Variant::Decimal4(decimal) => Some((decimal.integer().into(), decimal.scale())),
+            Variant::Decimal8(decimal) => Some((decimal.integer(), decimal.scale())),
            Variant::Decimal16(decimal) => {
-                if let Ok(converted_integer) = decimal.integer.try_into() {
-                    Some((converted_integer, decimal.scale))
+                if let Ok(converted_integer) = decimal.integer().try_into() {
+                    Some((converted_integer, decimal.scale()))
                } else {
                    None
                }


Now that VariantDecimalXX types are a thing, shouldn't as_decimalXX methods just return them? Combine that with impl From and TryFrom for the various decimal types and this code may become cleaner/safer?

pub fn as_decimal4(&self) -> Option<VariantDecimal4> { match *self { Variant::Int8(integer) => i32::from(integer).try_into().ok(), Variant::Int16(integer) => i32::from(integer).try_into().ok(), Variant::Int32(integer) => integer.try_into().ok(), Variant::Int64(integer) => i32::try_from(integer)?.try_into().ok(), Variant::Decimal4(decimal) => Some(decimal), Variant::Decimal8(decimal) => decimal.try_into().ok(), Variant::Decimal16(decimal) => decimal.try_into().ok(), _ => None, } } pub fn as_decimal8(&self) -> Option<VariantDecimal8> { match *self { Variant::Int8(integer) => i64::from(integer).try_into().ok(), Variant::Int16(integer) => i64::from(integer).try_into().ok(), Variant::Int32(integer) => i64::from(integer).try_into().ok(), Variant::Int64(integer) => integer.try_into().ok(), Variant::Decimal4(decimal) => Some(decimal.into()), Variant::Decimal8(decimal) => Some(decimal), Variant::Decimal16(decimal) => decimal.try_into().ok(), _ => None, } } pub fn as_decimal16(&self) -> Option<VariantDecimal16> { match *self { Variant::Int8(integer) => i128::from(integer).try_into().ok(), Variant::Int16(integer) => i128::from(integer).try_into().ok(), Variant::Int32(integer) => i128::from(integer).try_into().ok(), Variant::Int64(integer) => i128::from(integer).try_into().ok(), Variant::Decimal4(decimal) => Some(decimal.into()), Variant::Decimal8(decimal) => Some(decimal.into()), Variant::Decimal16(decimal) => Some(decimal), _ => None, } }

Details

// Infallible conversion from a narrower decimal type to a wider one macro_rules! impl_from_decimal_for_decimal { ($from_ty:ty, $for_ty:ty) => { impl From<$from_ty> for $for_ty { fn from(decimal: $from_ty) -> Self { Self { integer: decimal.integer().into(), scale: decimal.scale(), } } } }; } // Fallible conversion from a wider decimal type to a narrower one macro_rules! impl_try_from_decimal_for_decimal { ($from_ty:ty, $for_ty:ty) => { impl TryFrom<$from_ty> for $for_ty { type Error = ArrowError; fn try_from(decimal: $from_ty) -> Result<Self, ArrowError> { if decimal.scale() > Self::MAX_PRECISION { return Err(...); } Ok(Self { integer: decimal.integer().try_into()?, scale: decimal.scale(), }) } } }; } // Fallible conversion from a decimal's underlying integer type macro_rules! impl_try_from_int_for_decimal { ($from_ty:ty, $for_ty:ty) => { impl TryFrom<$from_ty> for $for_ty { type Error = ArrowError; fn try_from(integer: $from_ty) -> Result<Self, ArrowError> { Self::try_new(integer, 0) } } }; } impl_from_decimal_for_decimal!(Decimal4, Decimal8); impl_from_decimal_for_decimal!(Decimal4, Decimal16); impl_from_decimal_for_decimal!(Decimal8, Decimal16); impl_try_from_decimal_for_decimal!(Decimal8, Decimal4); impl_try_from_decimal_for_decimal!(Decimal16, Decimal4); impl_try_from_decimal_for_decimal!(Decimal16, Decimal8); impl_try_from_int_for_decimal!(i32, Decimal4); impl_try_from_int_for_decimal!(i64, Decimal8); impl_try_from_int_for_decimal!(i128, Decimal16);

[Variant] Minor: make fields in VariantDecimal* private, add examples #7770

[Variant] Minor: make fields in VariantDecimal* private, add examples #7770

Uh oh!

Conversation

alamb commented Jun 24, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

friendlymatthew commented Jun 24, 2025

Uh oh!

alamb commented Jun 24, 2025

Uh oh!

Weijun-H left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented Jun 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Jun 27, 2025

Uh oh!

friendlymatthew left a comment

Choose a reason for hiding this comment

Uh oh!

scovich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Jun 27, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[Variant] Minor: make fields in `VariantDecimal*` private, add examples #7770

[Variant] Minor: make fields in `VariantDecimal*` private, add examples #7770