From 9ffc13847880a86b0fee4d2c1bf42170eb7ecd1d Mon Sep 17 00:00:00 2001 From: Jeremy Schoemaker Date: Sun, 20 Sep 2026 22:44:37 -0500 Subject: [PATCH 1/5] 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 --- crates/iceberg/src/spec/datatypes.rs | 99 ++++++++++++++++++++++++---- 1 file changed, 85 insertions(+), 14 deletions(-) diff --git a/crates/iceberg/src/spec/datatypes.rs b/crates/iceberg/src/spec/datatypes.rs index 8c4029f7b3..71d9cb79e5 100644 --- a/crates/iceberg/src/spec/datatypes.rs +++ b/crates/iceberg/src/spec/datatypes.rs @@ -350,16 +350,30 @@ impl Serialize for PrimitiveType { fn deserialize_decimal<'de, D>(deserializer: D) -> std::result::Result 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: u32 = precision.trim().parse().map_err(|_| malformed())?; + let scale: u32 = scale.trim().parse().map_err(|_| malformed())?; + + if precision == 0 || precision > MAX_DECIMAL_PRECISION { + return Err(D::Error::custom(format!( + "Decimals with precision larger than {MAX_DECIMAL_PRECISION} are not supported: {precision}" + ))); + } + if scale > precision { + return Err(D::Error::custom(format!( + "Decimal scale {scale} must not be larger than precision {precision}" + ))); + } - 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( @@ -375,15 +389,13 @@ where fn deserialize_fixed<'de, D>(deserializer: D) -> std::result::Result where D: Deserializer<'de> { - let fixed = String::deserialize(deserializer)? - .trim_start_matches(r"fixed[") - .trim_end_matches(']') - .to_owned(); + let s = String::deserialize(deserializer)?; - fixed - .parse() + s.strip_prefix("fixed[") + .and_then(|inner| inner.strip_suffix(']')) + .and_then(|length| length.trim().parse().ok()) .map(PrimitiveType::Fixed) - .map_err(D::Error::custom) + .ok_or_else(|| D::Error::custom(format!("Invalid fixed type: {s}"))) } fn serialize_fixed(value: &u64, serializer: S) -> std::result::Result @@ -1326,6 +1338,65 @@ 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(5, 8)""#, + r#""decimal(decimal(5, 2)))""#, + r#""decimal(5, 2""#, + r#""decimal(5, 2)))))""#, + r#""decimal(5, 2, 3)""#, + r#""decimal(-5, 2)""#, + r#""decimal()""#, + r#""fixed[fixed[16]]]""#, + r#""fixed[16""#, + r#""fixed[16]]]""#, + r#""fixed[]""#, + ] { + assert!( + serde_json::from_str::(invalid).is_err(), + "expected {invalid} to be rejected" + ); + } + } + + #[test] + fn test_accept_valid_decimal_and_fixed_type_strings() { + for (json, expected) in [ + ( + r#""decimal(9, 2)""#, + Type::Primitive(PrimitiveType::Decimal { + precision: 9, + scale: 2, + }), + ), + ( + r#""decimal(38, 10)""#, + Type::Primitive(PrimitiveType::Decimal { + precision: 38, + scale: 10, + }), + ), + ( + r#""decimal(5,2)""#, + Type::Primitive(PrimitiveType::Decimal { + precision: 5, + scale: 2, + }), + ), + (r#""fixed[16]""#, Type::Primitive(PrimitiveType::Fixed(16))), + ] { + check_type_serde_roundtrip_value(json, expected); + } + } + + fn check_type_serde_roundtrip_value(json: &str, expected_type: Type) { + let parsed: Type = serde_json::from_str(json).unwrap(); + assert_eq!(parsed, expected_type); + } + #[test] fn test_primitive_type_compatible() { let pairs = vec![ From 7d88ecb67fef0116f4240b2b99223327564194bf Mon Sep 17 00:00:00 2001 From: Jeremy Schoemaker Date: Sun, 20 Sep 2026 22:58:36 -0500 Subject: [PATCH 2/5] fix(spec): Clarify zero precision errors and verify serde roundtrips --- crates/iceberg/src/spec/datatypes.rs | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/crates/iceberg/src/spec/datatypes.rs b/crates/iceberg/src/spec/datatypes.rs index 71d9cb79e5..a33538a9d3 100644 --- a/crates/iceberg/src/spec/datatypes.rs +++ b/crates/iceberg/src/spec/datatypes.rs @@ -362,7 +362,12 @@ where D: Deserializer<'de> { let precision: u32 = precision.trim().parse().map_err(|_| malformed())?; let scale: u32 = scale.trim().parse().map_err(|_| malformed())?; - if precision == 0 || precision > MAX_DECIMAL_PRECISION { + if precision == 0 { + return Err(D::Error::custom( + "Decimal precision must be greater than zero", + )); + } + if precision > MAX_DECIMAL_PRECISION { return Err(D::Error::custom(format!( "Decimals with precision larger than {MAX_DECIMAL_PRECISION} are not supported: {precision}" ))); @@ -1362,6 +1367,17 @@ mod tests { } } + #[test] + fn test_decimal_zero_precision_error() { + let error = serde_json::from_str::(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, expected) in [ @@ -1395,6 +1411,9 @@ mod tests { fn check_type_serde_roundtrip_value(json: &str, expected_type: Type) { let parsed: Type = serde_json::from_str(json).unwrap(); assert_eq!(parsed, expected_type); + let serialized = serde_json::to_string(&expected_type).unwrap(); + let reparsed: Type = serde_json::from_str(&serialized).unwrap(); + assert_eq!(reparsed, expected_type); } #[test] From efb7ebbdd25bb1774c82b433ad324e3c29fa5563 Mon Sep 17 00:00:00 2001 From: Jeremy Schoemaker Date: Wed, 23 Sep 2026 04:42:44 -0500 Subject: [PATCH 3/5] test(spec): cover decimal and fixed parsing edge cases --- crates/iceberg/src/spec/datatypes.rs | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/crates/iceberg/src/spec/datatypes.rs b/crates/iceberg/src/spec/datatypes.rs index a33538a9d3..c888f60c03 100644 --- a/crates/iceberg/src/spec/datatypes.rs +++ b/crates/iceberg/src/spec/datatypes.rs @@ -1350,14 +1350,23 @@ mod tests { r#""decimal(0, 0)""#, r#""decimal(5, 8)""#, 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!( @@ -1402,6 +1411,20 @@ mod tests { scale: 2, }), ), + ( + r#""decimal(5, 0)""#, + Type::Primitive(PrimitiveType::Decimal { + precision: 5, + scale: 0, + }), + ), + ( + r#""decimal(5, 5)""#, + Type::Primitive(PrimitiveType::Decimal { + precision: 5, + scale: 5, + }), + ), (r#""fixed[16]""#, Type::Primitive(PrimitiveType::Fixed(16))), ] { check_type_serde_roundtrip_value(json, expected); From 93a06c0aab3d0fd2bb04491c08381c6d7b08566b Mon Sep 17 00:00:00 2001 From: Jeremy Schoemaker Date: Mon, 28 Sep 2026 16:46:55 -0500 Subject: [PATCH 4/5] fix(spec): align decimal serde validation with constructor --- crates/iceberg/src/spec/datatypes.rs | 79 ++++++++++++++++++++-------- 1 file changed, 56 insertions(+), 23 deletions(-) diff --git a/crates/iceberg/src/spec/datatypes.rs b/crates/iceberg/src/spec/datatypes.rs index c888f60c03..af63275273 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 { - 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,6 +344,15 @@ impl Serialize for PrimitiveType { } } +fn validate_decimal_precision(precision: u32) -> Result<()> { + 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 where D: Deserializer<'de> { let s = String::deserialize(deserializer)?; @@ -359,24 +365,16 @@ where D: Deserializer<'de> { .split_once(',') .ok_or_else(|| D::Error::custom(format!("Decimal requires precision and scale: {s}")))?; - let precision: u32 = precision.trim().parse().map_err(|_| malformed())?; - let scale: u32 = scale.trim().parse().map_err(|_| malformed())?; - - if precision == 0 { - return Err(D::Error::custom( - "Decimal precision must be greater than zero", - )); - } - if precision > MAX_DECIMAL_PRECISION { - return Err(D::Error::custom(format!( - "Decimals with precision larger than {MAX_DECIMAL_PRECISION} are not supported: {precision}" - ))); - } - if scale > precision { - return Err(D::Error::custom(format!( - "Decimal scale {scale} must not be larger than precision {precision}" - ))); + let (precision, scale) = (precision.trim(), scale.trim()); + if ![precision, scale] + .iter() + .all(|token| token.bytes().all(|byte| byte.is_ascii_digit())) + { + return Err(malformed()); } + let precision: u32 = precision.parse().map_err(|_| malformed())?; + let scale: u32 = scale.parse().map_err(|_| malformed())?; + validate_decimal_precision(precision).map_err(D::Error::custom)?; Ok(PrimitiveType::Decimal { precision, scale }) } @@ -398,7 +396,9 @@ where D: Deserializer<'de> { s.strip_prefix("fixed[") .and_then(|inner| inner.strip_suffix(']')) - .and_then(|length| length.trim().parse().ok()) + .map(str::trim) + .filter(|length| length.bytes().all(|byte| byte.is_ascii_digit())) + .and_then(|length| length.parse().ok()) .map(PrimitiveType::Fixed) .ok_or_else(|| D::Error::custom(format!("Invalid fixed type: {s}"))) } @@ -1348,7 +1348,6 @@ mod tests { for invalid in [ r#""decimal(50, 2)""#, r#""decimal(0, 0)""#, - r#""decimal(5, 8)""#, r#""decimal(decimal(5, 2)))""#, r#""decimal(decimal(5, 2)""#, r#""decimal(5, 2""#, @@ -1376,6 +1375,40 @@ mod tests { } } + #[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::(invalid).is_err(), "{invalid}"); + assert!( + serde_json::from_str::(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::(&json).is_err()); + assert!(serde_json::from_str::(&json).is_err()); + } + } + #[test] fn test_decimal_zero_precision_error() { let error = serde_json::from_str::(r#""decimal(0, 0)""#).unwrap_err(); From 0c6d0d5fd13057ae8b86b10d1f479f1c1e8093ff Mon Sep 17 00:00:00 2001 From: Jeremy Schoemaker Date: Thu, 1 Oct 2026 03:44:53 -0500 Subject: [PATCH 5/5] fix(spec): preserve type parse errors and canonical serde coverage --- crates/iceberg/src/spec/datatypes.rs | 78 +++++++++++++++++++++++----- 1 file changed, 65 insertions(+), 13 deletions(-) diff --git a/crates/iceberg/src/spec/datatypes.rs b/crates/iceberg/src/spec/datatypes.rs index af63275273..bfee520236 100644 --- a/crates/iceberg/src/spec/datatypes.rs +++ b/crates/iceberg/src/spec/datatypes.rs @@ -345,6 +345,7 @@ 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, @@ -366,14 +367,14 @@ where D: Deserializer<'de> { .ok_or_else(|| D::Error::custom(format!("Decimal requires precision and scale: {s}")))?; let (precision, scale) = (precision.trim(), scale.trim()); - if ![precision, scale] + if [precision, scale] .iter() - .all(|token| token.bytes().all(|byte| byte.is_ascii_digit())) + .any(|token| token.is_empty() || !token.bytes().all(|byte| byte.is_ascii_digit())) { return Err(malformed()); } - let precision: u32 = precision.parse().map_err(|_| malformed())?; - let scale: u32 = scale.parse().map_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, scale }) @@ -394,13 +395,17 @@ fn deserialize_fixed<'de, D>(deserializer: D) -> std::result::Result { let s = String::deserialize(deserializer)?; - s.strip_prefix("fixed[") + let length = s + .strip_prefix("fixed[") .and_then(|inner| inner.strip_suffix(']')) .map(str::trim) - .filter(|length| length.bytes().all(|byte| byte.is_ascii_digit())) - .and_then(|length| length.parse().ok()) + .filter(|length| !length.is_empty() && length.bytes().all(|byte| byte.is_ascii_digit())) + .ok_or_else(|| D::Error::custom(format!("Invalid fixed type: {s}")))?; + + length + .parse() .map(PrimitiveType::Fixed) - .ok_or_else(|| D::Error::custom(format!("Invalid fixed type: {s}"))) + .map_err(D::Error::custom) } fn serialize_fixed(value: &u64, serializer: S) -> std::result::Result @@ -1409,6 +1414,41 @@ mod tests { } } + #[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::(invalid).is_err(), "{invalid}"); + assert!( + serde_json::from_str::(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::(invalid).is_err(), "{invalid}"); + let error = serde_json::from_str::(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::(r#""decimal(0, 0)""#).unwrap_err(); @@ -1422,8 +1462,9 @@ mod tests { #[test] fn test_accept_valid_decimal_and_fixed_type_strings() { - for (json, expected) in [ + for (json, canonical, expected) in [ ( + r#""decimal(9, 2)""#, r#""decimal(9, 2)""#, Type::Primitive(PrimitiveType::Decimal { precision: 9, @@ -1431,6 +1472,7 @@ mod tests { }), ), ( + r#""decimal(38, 10)""#, r#""decimal(38, 10)""#, Type::Primitive(PrimitiveType::Decimal { precision: 38, @@ -1439,12 +1481,14 @@ mod tests { ), ( 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, @@ -1452,22 +1496,30 @@ mod tests { }), ), ( + r#""decimal(5, 5)""#, r#""decimal(5, 5)""#, Type::Primitive(PrimitiveType::Decimal { precision: 5, scale: 5, }), ), - (r#""fixed[16]""#, Type::Primitive(PrimitiveType::Fixed(16))), + ( + r#""fixed[16]""#, + r#""fixed[16]""#, + Type::Primitive(PrimitiveType::Fixed(16)), + ), ] { - check_type_serde_roundtrip_value(json, expected); + check_type_serde_roundtrip_value(json, canonical, expected); } } - fn check_type_serde_roundtrip_value(json: &str, expected_type: Type) { + 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(&expected_type).unwrap(); + 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); }