This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-3256-19603861d545f8edbc5016e45cb1bacb0579cbf8 in repository https://gitbox.apache.org/repos/asf/iceberg-rust.git
commit fc682d7a050cdc05e2b90ccf04192034dc8f1a1c Author: Jeremy Schoemaker <[email protected]> AuthorDate: Fri Oct 2 09:57:27 2026 +0000 fix(spec): Harden decimal and fixed type string parsing (#3256) * fix(spec): Harden decimal and fixed type string parsing deserialize_decimal and deserialize_fixed used trim_start_matches and trim_end_matches, which strip repeated occurrences and never require the prefix, so "decimal(decimal(5, 2)))))" and "fixed[16" both parsed successfully. Precision was also unbounded, so "decimal(50, 2)" was accepted into Schema and TableMetadata even though the crate's own MAX_DECIMAL_PRECISION is 38, and the failure only surfaced much later in Arrow or Avro conversion. Parse strictly with strip_prefix and strip_suffix, and reject precision outside 1..=38 and scale greater than precision. Signed-off-by: Jeremy Schoemaker <[email protected]> * fix(spec): Clarify zero precision errors and verify serde roundtrips * test(spec): cover decimal and fixed parsing edge cases * fix(spec): align decimal serde validation with constructor * fix(spec): preserve type parse errors and canonical serde coverage --------- Signed-off-by: Jeremy Schoemaker <[email protected]> --- crates/iceberg/src/spec/datatypes.rs | 230 ++++++++++++++++++++++++++++++++--- 1 file changed, 214 insertions(+), 16 deletions(-) diff --git a/crates/iceberg/src/spec/datatypes.rs b/crates/iceberg/src/spec/datatypes.rs index 8c4029f7b..bfee52023 100644 --- a/crates/iceberg/src/spec/datatypes.rs +++ b/crates/iceberg/src/spec/datatypes.rs @@ -191,10 +191,7 @@ impl Type { /// Creates decimal type. #[inline(always)] pub fn decimal(precision: u32, scale: u32) -> Result<Self> { - ensure_data_valid!( - precision > 0 && precision <= MAX_DECIMAL_PRECISION, - "Decimals with precision larger than {MAX_DECIMAL_PRECISION} are not supported: {precision}", - ); + validate_decimal_precision(precision)?; Ok(Type::Primitive(PrimitiveType::Decimal { precision, scale })) } @@ -347,19 +344,40 @@ impl Serialize for PrimitiveType { } } +fn validate_decimal_precision(precision: u32) -> Result<()> { + // Leave scale unconstrained for Java parity; the spec leaves scale > precision open. + ensure_data_valid!(precision > 0, "Decimal precision must be greater than zero",); + ensure_data_valid!( + precision <= MAX_DECIMAL_PRECISION, + "Decimals with precision larger than {MAX_DECIMAL_PRECISION} are not supported: {precision}", + ); + Ok(()) +} + fn deserialize_decimal<'de, D>(deserializer: D) -> std::result::Result<PrimitiveType, D::Error> where D: Deserializer<'de> { let s = String::deserialize(deserializer)?; + let malformed = || D::Error::custom(format!("Invalid decimal type: {s}")); + let (precision, scale) = s - .trim_start_matches(r"decimal(") - .trim_end_matches(')') + .strip_prefix("decimal(") + .and_then(|inner| inner.strip_suffix(')')) + .ok_or_else(malformed)? .split_once(',') - .ok_or_else(|| D::Error::custom("Decimal requires precision and scale: {s}"))?; + .ok_or_else(|| D::Error::custom(format!("Decimal requires precision and scale: {s}")))?; + + let (precision, scale) = (precision.trim(), scale.trim()); + if [precision, scale] + .iter() + .any(|token| token.is_empty() || !token.bytes().all(|byte| byte.is_ascii_digit())) + { + return Err(malformed()); + } + let precision: u32 = precision.parse().map_err(D::Error::custom)?; + let scale: u32 = scale.parse().map_err(D::Error::custom)?; + validate_decimal_precision(precision).map_err(D::Error::custom)?; - Ok(PrimitiveType::Decimal { - precision: precision.trim().parse().map_err(D::Error::custom)?, - scale: scale.trim().parse().map_err(D::Error::custom)?, - }) + Ok(PrimitiveType::Decimal { precision, scale }) } fn serialize_decimal<S>( @@ -375,12 +393,16 @@ where fn deserialize_fixed<'de, D>(deserializer: D) -> std::result::Result<PrimitiveType, D::Error> where D: Deserializer<'de> { - let fixed = String::deserialize(deserializer)? - .trim_start_matches(r"fixed[") - .trim_end_matches(']') - .to_owned(); + let s = String::deserialize(deserializer)?; + + let length = s + .strip_prefix("fixed[") + .and_then(|inner| inner.strip_suffix(']')) + .map(str::trim) + .filter(|length| !length.is_empty() && length.bytes().all(|byte| byte.is_ascii_digit())) + .ok_or_else(|| D::Error::custom(format!("Invalid fixed type: {s}")))?; - fixed + length .parse() .map(PrimitiveType::Fixed) .map_err(D::Error::custom) @@ -1326,6 +1348,182 @@ mod tests { assert_eq!(16, Type::decimal_required_bytes(38).unwrap()); } + #[test] + fn test_reject_malformed_decimal_and_fixed_type_strings() { + for invalid in [ + r#""decimal(50, 2)""#, + r#""decimal(0, 0)""#, + r#""decimal(decimal(5, 2)))""#, + r#""decimal(decimal(5, 2)""#, + r#""decimal(5, 2""#, + r#""decimal(5, 2)))))""#, + r#""decimal(5, 2, 3)""#, + r#""decimal(-5, 2)""#, + r#""decimal(5, -2)""#, + r#""decimal(-5, -2)""#, + r#""decimal(-2, -5)""#, + r#""decimal((5, 2))""#, + r#""decimal[5, 2]""#, + r#""decimal()""#, + r#""fixed[fixed[16]]]""#, + r#""fixed[16""#, + r#""fixed[16]]]""#, + r#""fixed[[16]]""#, + r#""fixed[[16]""#, + r#""fixed(16)""#, + r#""fixed[]""#, + ] { + assert!( + serde_json::from_str::<Type>(invalid).is_err(), + "expected {invalid} to be rejected" + ); + } + } + + #[test] + fn test_reject_leading_plus_in_type_strings() { + for invalid in [ + r#""decimal(+5, 2)""#, + r#""decimal(5, +2)""#, + r#""decimal(+5, +2)""#, + r#""fixed[+16]""#, + ] { + assert!(serde_json::from_str::<Type>(invalid).is_err(), "{invalid}"); + assert!( + serde_json::from_str::<PrimitiveType>(invalid).is_err(), + "{invalid}" + ); + } + } + + #[test] + fn test_decimal_constructor_json_roundtrip() { + for (precision, scale) in [(1, 0), (5, 5), (5, 8), (38, 38)] { + let decimal = Type::decimal(precision, scale).unwrap(); + let serialized = serde_json::to_string(&decimal).unwrap(); + let reparsed: Type = serde_json::from_str(&serialized).unwrap(); + assert_eq!(reparsed, decimal); + let primitive: PrimitiveType = serde_json::from_str(&serialized).unwrap(); + assert_eq!(Type::Primitive(primitive), decimal); + } + for precision in [0, 39] { + assert!(Type::decimal(precision, 0).is_err()); + let json = format!(r#""decimal({precision}, 0)""#); + assert!(serde_json::from_str::<Type>(&json).is_err()); + assert!(serde_json::from_str::<PrimitiveType>(&json).is_err()); + } + } + + #[test] + fn test_empty_decimal_and_fixed_tokens() { + for invalid in [ + r#""decimal(, 2)""#, + r#""decimal(5,)""#, + r#""decimal( , )""#, + r#""fixed[]""#, + r#""fixed[ ]""#, + ] { + assert!(serde_json::from_str::<Type>(invalid).is_err(), "{invalid}"); + assert!( + serde_json::from_str::<PrimitiveType>(invalid).is_err(), + "{invalid}" + ); + } + } + + #[test] + fn test_decimal_and_fixed_overflow_errors() { + for invalid in [ + r#""decimal(9999999999, 2)""#, + r#""decimal(5, 9999999999)""#, + r#""fixed[18446744073709551616]""#, + ] { + assert!(serde_json::from_str::<Type>(invalid).is_err(), "{invalid}"); + let error = serde_json::from_str::<PrimitiveType>(invalid).unwrap_err(); + assert!( + error + .to_string() + .contains("number too large to fit in target type"), + "unexpected error for {invalid}: {error}" + ); + } + } + + #[test] + fn test_decimal_zero_precision_error() { + let error = serde_json::from_str::<PrimitiveType>(r#""decimal(0, 0)""#).unwrap_err(); + assert!( + error + .to_string() + .contains("Decimal precision must be greater than zero"), + "unexpected error: {error}" + ); + } + + #[test] + fn test_accept_valid_decimal_and_fixed_type_strings() { + for (json, canonical, expected) in [ + ( + r#""decimal(9, 2)""#, + r#""decimal(9, 2)""#, + Type::Primitive(PrimitiveType::Decimal { + precision: 9, + scale: 2, + }), + ), + ( + r#""decimal(38, 10)""#, + r#""decimal(38, 10)""#, + Type::Primitive(PrimitiveType::Decimal { + precision: 38, + scale: 10, + }), + ), + ( + r#""decimal(5,2)""#, + r#""decimal(5, 2)""#, + Type::Primitive(PrimitiveType::Decimal { + precision: 5, + scale: 2, + }), + ), + ( + r#""decimal(5, 0)""#, + r#""decimal(5, 0)""#, + Type::Primitive(PrimitiveType::Decimal { + precision: 5, + scale: 0, + }), + ), + ( + r#""decimal(5, 5)""#, + r#""decimal(5, 5)""#, + Type::Primitive(PrimitiveType::Decimal { + precision: 5, + scale: 5, + }), + ), + ( + r#""fixed[16]""#, + r#""fixed[16]""#, + Type::Primitive(PrimitiveType::Fixed(16)), + ), + ] { + check_type_serde_roundtrip_value(json, canonical, expected); + } + } + + fn check_type_serde_roundtrip_value(json: &str, canonical: &str, expected_type: Type) { + let parsed: Type = serde_json::from_str(json).unwrap(); + assert_eq!(parsed, expected_type); + let serialized = serde_json::to_string(&parsed).unwrap(); + assert_eq!(serialized, canonical); + let primitive: PrimitiveType = serde_json::from_str(json).unwrap(); + assert_eq!(serde_json::to_string(&primitive).unwrap(), canonical); + let reparsed: Type = serde_json::from_str(&serialized).unwrap(); + assert_eq!(reparsed, expected_type); + } + #[test] fn test_primitive_type_compatible() { let pairs = vec