Skip to content

[VARIANT] Validate precision in VariantDecimalXX structs and add missing tests #7776

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alamb merged 2 commits into from
Jun 25, 2025
Merged

[VARIANT] Validate precision in VariantDecimalXX structs and add missing tests #7776

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
93af2a7
[VARIANT] Move VariantDecimalXX structs to their own mod and add tests
scovich Jun 25, 2025
e122538
Merge remote-tracking branch 'apache/main' into validate-decimal-value
alamb Jun 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 4 additions & 100 deletions parquet-variant/src/variant.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
use std::ops::Deref;
Copy link
Contributor Author

@scovich scovich Jun 25, 2025

Choose a reason for hiding this comment

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

not sure how this ended up here...

Copy link
Contributor

@alamb alamb Jun 25, 2025

Choose a reason for hiding this comment

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

we can blame the IDE perhaps


// Licensed to the Apache Software Foundation (ASF) under one
// or more contributor license agreements. See the NOTICE file
// distributed with this work for additional information
Expand All @@ -16,6 +14,7 @@ use std::ops::Deref;
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.
pub use self::decimal::{VariantDecimal16, VariantDecimal4, VariantDecimal8};
pub use self::list::VariantList;
pub use self::metadata::VariantMetadata;
pub use self::object::VariantObject;
Expand All @@ -27,6 +26,9 @@ use crate::utils::{first_byte_from_slice, slice_from_slice};
use arrow_schema::ArrowError;
use chrono::{DateTime, NaiveDate, NaiveDateTime, Utc};

use std::ops::Deref;

mod decimal;
mod list;
mod metadata;
mod object;
Expand All @@ -40,98 +42,6 @@ const MAX_SHORT_STRING_BYTES: usize = 0x3F;
#[derive(Debug, Clone, Copy, PartialEq)]
pub struct ShortString<'a>(pub(crate) &'a str);

/// Represents a 4-byte decimal value in the Variant format.
Copy link
Contributor Author

@scovich scovich Jun 25, 2025

Choose a reason for hiding this comment

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

Moved to decimal.rs

///
/// This struct stores a decimal number using a 32-bit signed integer for the coefficient
/// and an 8-bit unsigned integer for the scale (number of decimal places). Its precision is limited to 9 digits.
///
/// For valid precision and scale values, see the Variant specification:
/// <https://github.com/apache/parquet-format/blob/87f2c8bf77eefb4c43d0ebaeea1778bd28ac3609/VariantEncoding.md?plain=1#L418-L420>
///
#[derive(Debug, Clone, Copy, PartialEq)]
pub struct VariantDecimal4 {
pub(crate) integer: i32,
pub(crate) scale: u8,
}

impl VariantDecimal4 {
pub fn try_new(integer: i32, scale: u8) -> Result<Self, ArrowError> {
const PRECISION_MAX: u32 = 9;

// Validate that scale doesn't exceed precision
if scale as u32 > PRECISION_MAX {
return Err(ArrowError::InvalidArgumentError(format!(
"Scale {} cannot be greater than precision 9 for 4-byte decimal",
scale
)));
}

Ok(VariantDecimal4 { integer, scale })
}
}

/// Represents an 8-byte decimal value in the Variant format.
///
/// This struct stores a decimal number using a 64-bit signed integer for the coefficient
/// and an 8-bit unsigned integer for the scale (number of decimal places). Its precision is between 10 and 18 digits.
///
/// For valid precision and scale values, see the Variant specification:
///
/// <https://github.com/apache/parquet-format/blob/87f2c8bf77eefb4c43d0ebaeea1778bd28ac3609/VariantEncoding.md?plain=1#L418-L420>
///
#[derive(Debug, Clone, Copy, PartialEq)]
pub struct VariantDecimal8 {
pub(crate) integer: i64,
pub(crate) scale: u8,
}

impl VariantDecimal8 {
pub fn try_new(integer: i64, scale: u8) -> Result<Self, ArrowError> {
const PRECISION_MAX: u32 = 18;

// Validate that scale doesn't exceed precision
if scale as u32 > PRECISION_MAX {
return Err(ArrowError::InvalidArgumentError(format!(
"Scale {} cannot be greater than precision 18 for 8-byte decimal",
scale
)));
}

Ok(VariantDecimal8 { integer, scale })
}
}

/// Represents an 16-byte decimal value in the Variant format.
///
/// This struct stores a decimal number using a 128-bit signed integer for the coefficient
/// and an 8-bit unsigned integer for the scale (number of decimal places). Its precision is between 19 and 38 digits.
///
/// For valid precision and scale values, see the Variant specification:
///
/// <https://github.com/apache/parquet-format/blob/87f2c8bf77eefb4c43d0ebaeea1778bd28ac3609/VariantEncoding.md?plain=1#L418-L420>
///
#[derive(Debug, Clone, Copy, PartialEq)]
pub struct VariantDecimal16 {
pub(crate) integer: i128,
pub(crate) scale: u8,
}

impl VariantDecimal16 {
pub fn try_new(integer: i128, scale: u8) -> Result<Self, ArrowError> {
const PRECISION_MAX: u32 = 38;

// Validate that scale doesn't exceed precision
if scale as u32 > PRECISION_MAX {
return Err(ArrowError::InvalidArgumentError(format!(
"Scale {} cannot be greater than precision 38 for 16-byte decimal",
scale
)));
}

Ok(VariantDecimal16 { integer, scale })
}
}

impl<'a> ShortString<'a> {
/// Attempts to interpret `value` as a variant short string value.
///
Expand Down Expand Up @@ -1137,10 +1047,4 @@ mod tests {
Some((123456789012345678901234567890_i128, 2))
);
}

#[test]
fn test_invalid_variant_decimal_conversion() {
let decimal4 = VariantDecimal4::try_new(123456789_i32, 20);
assert!(decimal4.is_err(), "i32 overflow should fail");
}
}
Loading
Loading