fix(spec): Harden decimal and fixed type string parsing by shoemoney · Pull Request #3256 · apache/iceberg-rust

shoemoney · 2026-09-21T03:45:05Z

Which issue does this PR close?

Closes Schema data type parsing needs hardening #3229.

What changes are included in this PR?

deserialize_decimal (crates/iceberg/src/spec/datatypes.rs:350-363 on main) and
deserialize_fixed (:376-387 on main) used trim_start_matches / trim_end_matches.
Those strip repeated occurrences and never require the prefix to be present, so the
following all parsed successfully before this change:

"decimal(5, 2)))))" -> Decimal { precision: 5, scale: 2 }
"decimal(decimal(5, 2)))" -> Decimal { precision: 5, scale: 2 }
"decimal(5, 2" (no closing paren) -> Decimal { precision: 5, scale: 2 }
"fixed[fixed[16]]]" -> Fixed(16)
"fixed[16" -> Fixed(16)

Precision was also unvalidated, so "decimal(50, 2)" deserialized into a
PrimitiveType::Decimal and round-tripped back out, even though Type::decimal and
Type::decimal_required_bytes enforce precision <= MAX_DECIMAL_PRECISION (38). This is
reachable from every JSON schema parse (impl Deserialize for PrimitiveType), so a
Schema or TableMetadata written through iceberg-rust could carry a decimal type that
iceberg-java and pyiceberg reject; the error only surfaced much later and far away, in
arrow::schema (validate_decimal_precision_and_scale) or avro::schema. scale > precision
was likewise accepted.

The fix parses strictly with strip_prefix / strip_suffix, and applies the invariants the
crate already states elsewhere: 0 < precision <= MAX_DECIMAL_PRECISION and scale <= precision.

Not fixed deliberately: this PR does not introduce a regex dependency as the issue suggests,
since strip_prefix + split_once covers the same grammar with no new dependency, and it does
not touch the lenient starts_with("decimal") dispatch in impl Deserialize for PrimitiveType,
which now simply routes malformed input to a clear error instead of silently accepting it.

Are these changes tested?

Yes, two new unit tests in the existing spec::datatypes test module.

Before the fix (test only, source pristine):

test spec::datatypes::tests::test_reject_malformed_decimal_and_fixed_type_strings ... FAILED

---- spec::datatypes::tests::test_reject_malformed_decimal_and_fixed_type_strings stdout ----
panicked at crates/iceberg/src/spec/datatypes.rs:1346:13:
expected "decimal(50, 2)" to be rejected

test result: FAILED. 14 passed; 1 failed; 0 ignored; 0 measured; 1743 filtered out

After the fix:

$ cargo test -p iceberg --lib spec::
test result: ok. 475 passed; 0 failed; 0 ignored; 0 measured; 1283 filtered out

$ cargo test -p iceberg --lib
test result: ok. 1758 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

rustfmt (nightly-2026-04-16, per rust-toolchain.toml) and cargo clippy -p iceberg --lib --all-features are both clean on the touched file.

AI Disclosure

Written in conjunction with my pair programmer Claude.

This review follow-up was prepared with AI assistance.

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 <jeremy@shoemoney.com>

Copilot

Copilot review overview

🟢 Approval recommended

The changes are small, targeted, and well-tested, and the parsing hardening aligns with existing invariants while reducing downstream schema incompatibilities.

Review effort: Lite
Findings: 2

Open (2)

Fix misleading precision validation error message · New
Make serde round-trip test actually round-trip · New

What changed in this PR

This PR hardens Iceberg primitive type string parsing for decimal(p, s) and fixed[n] during JSON schema deserialization to reject malformed inputs and enforce decimal invariants earlier in the pipeline.

Changes:

Switch decimal and fixed parsing to strict strip_prefix/strip_suffix parsing instead of trim_*_matches.
Enforce decimal constraints at deserialization time (0 < precision <= MAX_DECIMAL_PRECISION, scale <= precision) with clearer errors for malformed inputs.
Add unit tests covering rejected malformed strings and accepted valid strings.

File	Description
crates/iceberg/src/spec/datatypes.rs	Tightens decimal/fixed string parsing and adds regression tests for malformed inputs.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

shoemoney · 2026-09-21T03:59:19Z

+    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}"
+        )));
+    }


Fixed in 7d88ecb. Zero precision now has its own error message; the regression test failed with the old message and passes with the fix.

shoemoney · 2026-09-21T03:59:22Z

+    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);
+    }


Fixed in 7d88ecb. The helper now serializes the expected type and deserializes it again before comparing. All 1,759 iceberg library tests, nightly formatting, and crate Clippy passed.

mkroll-db

It matches mostly the go implementation and the difference in how negative numbers are handled is covered by tests.
Added some smaller suggestions to add more tests.

mkroll-db · 2026-09-23T08:52:09Z

+            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[]""#,


Suggestion for some more tests, in particular testing negative numbers (go has them excluded in the regex):

Suggested change

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[]""#,

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""#,

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[]""#,

Added the missing negative, nested and delimiter cases in efb7ebb. All 1,759 iceberg library tests pass, along with format and strict Clippy checks.

mkroll-db · 2026-09-23T08:55:28Z

+                    precision: 5,
+                    scale: 2,
+                }),
+            ),


Some more 'corner' case tests:

Suggested change

),

),

(

r#""decimal(5, 0)""#,

Type::Primitive(PrimitiveType::Decimal {

precision: 5,

scale: 0,

}),

),

(

r#""decimal(5, 5)""#,

Type::Primitive(PrimitiveType::Decimal {

precision: 5,

scale: 5,

}),

),

Added decimal(5, 0) and decimal(5, 5) round trips in efb7ebb; both are covered by the passing library suite.

dannycjones

LGTM, thanks for picking this up!

Copilot AI lite review requested due to automatic review settings September 21, 2026 03:45

Copilot started reviewing on behalf of shoemoney September 21, 2026 03:45 View session

Copilot AI reviewed Sep 21, 2026

View reviewed changes

fix(spec): Clarify zero precision errors and verify serde roundtrips

7d88ecb

mkroll-db approved these changes Sep 23, 2026

View reviewed changes

test(spec): cover decimal and fixed parsing edge cases

efb7ebb

dannycjones approved these changes Sep 23, 2026

View reviewed changes

Navigation Menu

fix(spec): Harden decimal and fixed type string parsing - #3256

fix(spec): Harden decimal and fixed type string parsing#3256
shoemoney wants to merge 3 commits into
apache:mainfrom
shoemoney:fix/harden-decimal-fixed-type-parsing

shoemoney commented Sep 21, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

shoemoney Sep 21, 2026

Uh oh!

shoemoney Sep 21, 2026

Uh oh!

mkroll-db left a comment

Uh oh!

mkroll-db Sep 23, 2026

Uh oh!

shoemoney Sep 23, 2026

Uh oh!

mkroll-db Sep 23, 2026

Uh oh!

shoemoney Sep 23, 2026

Uh oh!

dannycjones left a comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

-            ),
+            ),
+            (
+                r#""decimal(5, 0)""#,
+                Type::Primitive(PrimitiveType::Decimal {
+                    precision: 5,
+                    scale: 0,
+                }),
+            ),
+            (
+                r#""decimal(5, 5)""#,
+                Type::Primitive(PrimitiveType::Decimal {
+                    precision: 5,
+                    scale: 5,
+                }),
+            ),

Conversation

shoemoney commented Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

AI Disclosure

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

shoemoney Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

shoemoney Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

mkroll-db left a comment

Choose a reason for hiding this comment

Uh oh!

mkroll-db Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

shoemoney Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

mkroll-db Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

shoemoney Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

dannycjones left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shoemoney commented Sep 21, 2026 •

edited

Loading