You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "laskoviymishka (via GitHub)" <gi...@apache.org> on 2026/10/01 08:17:38 UTC
Re: [PR] fix(spec): Harden decimal and fixed type string parsing [iceberg-rust]
laskoviymishka commented on code in PR #3256:
URL: https://github.com/apache/iceberg-rust/pull/3256#discussion_r4153248498
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -347,19 +344,39 @@ 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<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}")))?;
- Ok(PrimitiveType::Decimal {
- precision: precision.trim().parse().map_err(D::Error::custom)?,
- scale: scale.trim().parse().map_err(D::Error::custom)?,
- })
+ let (precision, scale) = (precision.trim(), scale.trim());
+ if ![precision, scale]
+ .iter()
+ .all(|token| token.bytes().all(|byte| byte.is_ascii_digit()))
Review Comment:
`[precision, scale].iter().all(|t| t.bytes().all(is_ascii_digit))` is vacuously true on an empty token, so `decimal(, 2)` and `decimal(5,)` slip past this guard and only get rejected downstream by `.parse()` — same for `fixed[]` via the `.filter(...)` in `deserialize_fixed`. The observable behavior is already correct (and matches the `decimal()` / `fixed[]` reject cases in apache/iceberg-verification#9), so this is clarity rather than a bug — but the guard isn't what's doing the work. I'd fold in the emptiness check and flip the negation to `any` while we're here:
```rust
if [precision, scale]
.iter()
.any(|token| token.is_empty() || !token.bytes().all(|b| b.is_ascii_digit()))
{
return Err(malformed());
}
```
Worth adding `decimal(, 2)` / `decimal(5,)` to the rejection test so the guard path is what's exercised, not the parse fallback.
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -347,19 +344,39 @@ 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<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}")))?;
- Ok(PrimitiveType::Decimal {
- precision: precision.trim().parse().map_err(D::Error::custom)?,
- scale: scale.trim().parse().map_err(D::Error::custom)?,
- })
+ 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())?;
Review Comment:
Once a token clears the digit filter, the only thing `.parse::<u32>()` can still reject is overflow (e.g. `decimal(9999999999, 2)`), and `map_err(|_| malformed())` collapses that into a generic `Invalid decimal type` — the old code forwarded the real `ParseIntError`. Minor, but I'd keep `map_err(D::Error::custom)` on the parse so overflow still says what it is. Same story with `deserialize_fixed`'s `.parse().ok()`.
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -1326,6 +1343,135 @@ 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_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, 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#""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);
+ }
+ }
+
+ fn check_type_serde_roundtrip_value(json: &str, expected_type: Type) {
Review Comment:
This checks `parse(json) == expected` and `parse(serialize(expected)) == expected`, but never that `serialize(parse(json))` matches the input — so `decimal(5,2)` (no space) doesn't actually round-trip through the string form. That canonical write direction is exactly what the conformance suite makes normative: apache/iceberg-verification#9 pins `decimal(9,2)` re-serializing to the spaced `decimal(9, 2)` byte-for-byte. So asserting `to_string(parse(json))` equals the canonical form here isn't just naming hygiene — it's the check that proves the write direction. I'd either reuse the existing `check_type_serde` for the canonical cases or add that assertion (and drop `roundtrip` from the name if you keep it weak).
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -1326,6 +1343,135 @@ 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)] {
Review Comment:
The `(5, 8)` case pins `scale > precision` as supported, and the deserializer has no `scale <= precision` guard. Worth knowing this is a genuinely unpinned corner: the cross-client conformance suite explicitly leaves `scale > precision` "out on purpose" (apache/iceberg-verification#9, `table-spec/types/README.md`) because the spec neither permits nor forbids it. So accepting it isn't non-conformant — but a Rust- or Java-written `decimal(5, 8)` can still fail to load in iceberg-go (`validateDecimalPrecisionScale`) and PyIceberg (`check_scale`), and no conformance run will catch that.
Since this test makes the choice documented behavior, I'd add a one-line comment on `validate_decimal_precision` noting scale is deliberately unconstrained for Java parity (and that the spec leaves it open). I would not add a `scale <= precision` check — that'd diverge from both Java and the suite's deliberate non-decision. Just want the acceptance to be on purpose.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org