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 #3229.

What changes are included in this PR?

Require exactly one pair of delimiters in decimal and fixed type strings. The previous repeated trimming accepted malformed inputs such as decimal(5, 2))))), decimal(decimal(5, 2))), and fixed[fixed[16]]], as well as missing closing delimiters.

Require a nonempty ASCII-digit token after trimming, rejecting empty tokens and a leading + in decimal precision, decimal scale, or fixed length. Integer overflow retains the original ParseIntError diagnostic.

Share the existing 1 <= precision <= 38 validation between Type::decimal and decimal JSON deserialization. Preserve the constructor's accepted scale values, including Type::decimal(5, 8), so constructor-produced values round-trip through JSON. The earlier version of this PR incorrectly imposed scale <= precision only in deserialization. Arrow conversion has a stricter scale check, but that is not a shared constructor or metadata invariant; this PR leaves the existing constructor acceptance unchanged.

No regex dependency or public API is added.

Are these changes tested?

The two new review regression tests failed on the previous PR head: leading-plus inputs were accepted and the constructor-created (5, 8) decimal failed to round-trip. Both now pass, with independent checks through Type and PrimitiveType. Existing malformed-input and zero-precision diagnostic tests remain.

The latest review follow-up adds empty-token rejection coverage and explicit canonical JSON serialization assertions through both types. Its overflow diagnostic regression failed on the previous head and passes with the restored parse errors. The shared precision validator documents the unchanged scale policy.

On the pinned nightly-2026-04-16 toolchain:

cargo test -p iceberg --lib --all-features: 1,763 passed.
cargo fmt --all -- --check: passed.
cargo clippy -p iceberg --all-targets --all-features -- -D warnings: passed.

AI Disclosure

Written in conjunction with my pair programmer Claude. Review follow-ups were prepared with AI assistance, including Codex.

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.

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.

dannycjones

LGTM, thanks for picking this up!

mkroll-db · 2026-09-24T02:06:32Z

Thanks a lot.

mbutrovich

Thanks @shoemoney for picking this up! This also closes a gap I ran into while reviewing #3241. A manifest whose stored schema declares decimal(39, 10) parses today, and the error only shows up later when a bound is converted back to bytes.

mbutrovich · 2026-09-28T19:43:54Z

+    let precision: u32 = precision.trim().parse().map_err(|_| malformed())?;
+    let scale: u32 = scale.trim().parse().map_err(|_| malformed())?;


Should we also reject a leading + here? u32::from_str accepts it, so on the head commit "decimal(+5, +2)" parses to Decimal { precision: 5, scale: 2 } and "fixed[+16]" (line 401) parses to Fixed(16). The type-string patterns in Java (Types.java#L66-L73), iceberg-go (types.go#L36-L37), and PyIceberg (types.py#L60) only allow digits there. If we reject it, could both strings go into test_reject_malformed_decimal_and_fixed_type_strings?

Fixed in 93a06c0. The parser now checks that each trimmed numeric token contains ASCII digits before parsing. Added rejection cases for decimal(+5, 2), decimal(5, +2), decimal(+5, +2), and fixed[+16] through both Type and PrimitiveType.

The new test failed on the previous head and passes with the fix. All 1,761 library tests, the pinned formatting check, and strict all-target/all-feature Clippy passed.

mbutrovich · 2026-09-28T19:44:16Z

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


Type::decimal (datatypes.rs#L193-L199) checks precision but not scale. That means Type::decimal(5, 8) succeeds and serializes as "decimal(5, 8)", which this function now rejects. On the head commit, serde_json::from_str::<Type>(&serde_json::to_string(&Type::decimal(5, 8).unwrap()).unwrap()) returns data did not match any variant of untagged enum SerdeType. So a schema built through the public API can end up in table metadata that iceberg-rust can't read back.

What do you think about moving these checks into one private function that both Type::decimal and deserialize_decimal call? The two construction paths couldn't disagree, and the precision message wouldn't be written twice. A test that round-trips a Type::decimal result through JSON would cover it.

The description says the crate already requires scale <= precision elsewhere. Could you point me to where? I couldn't find it outside this PR. The implementations don't agree on the rule either. The spec only requires precision to be 38 or less (spec.md#L273), and Java's DecimalType checks only that (Types.java#L525-L533). PyIceberg (types.py#L331-L347), iceberg-go (types.go#L680-L694), and arrow-rs (types.rs#L1412-L1442) all reject precision 0 and a scale larger than the precision. With the stricter rule, iceberg-rust would fail to load table metadata that Java accepts, the same way PyIceberg does today. Whichever rule we pick, it would help to note the choice in the description.

You're right: the scale <= precision statement in the description was too broad. arrow/schema.rs applies Arrow's stricter validation, but Type::decimal and the metadata parser did not share that restriction.

In 93a06c0, the constructor and deserializer share a private precision validator. I removed this PR's added scale restriction to preserve existing constructor behavior, including (5, 8), and corrected the description. Constructor-built roundtrip tests cover (1, 0), (5, 5), (5, 8), and (38, 38), plus rejection of precision 0 and 39. The (5, 8) roundtrip failed on the previous head and now passes.

All 1,761 library tests, the pinned formatting check, and strict all-target/all-feature Clippy passed. Tightening the public constructor's scale policy can be considered separately.

laskoviymishka

The strip_prefix / strip_suffix change looks right. It catches the #3241 case during parsing instead of later when the value is serialized, and the reject cases line up with the primitive-type fixtures in apache/iceberg-verification#9.

A couple of small things:

the digit check is true for an empty string, so decimal(, 2) and fixed[] only fail at .parse(). Behavior is fine, but !is_empty() plus tests would make this clearer.
parse overflow now gets turned into a generic error. I’d keep the original ParseIntError.

One thing worth documenting: (5, 8) makes scale > precision explicitly valid here. The spec doesn’t really pin that case, so this matches Java, but iceberg-go and PyIceberg reject it. I’d add a comment so the choice isn’t accidental.

I'll keep this PR open for couple days before merge.

laskoviymishka · 2026-10-01T08:14:54Z

+    let (precision, scale) = (precision.trim(), scale.trim());
+    if ![precision, scale]
+        .iter()
+        .all(|token| token.bytes().all(|byte| byte.is_ascii_digit()))


[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:

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.

Added explicit nonempty-token checks in 0c6d0d5. The rejection tests now cover decimal(, 2), decimal(5,), whitespace-only tokens and fixed[], through both Type and PrimitiveType. All 1,763 library tests, formatting and strict all-target/all-feature Clippy passed on the pinned nightly.

laskoviymishka · 2026-10-01T08:14:54Z

+    {
+        return Err(malformed());
+    }
+    let precision: u32 = precision.parse().map_err(|_| malformed())?;


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().

Restored map_err(D::Error::custom) for decimal precision, decimal scale and fixed length in 0c6d0d5, so overflow retains the ParseIntError diagnostic. The new regression failed on the previous head and now passes for overflowing u32 precision/scale and u64 fixed length.

laskoviymishka · 2026-10-01T08:14:54Z

+
+    #[test]
+    fn test_decimal_constructor_json_roundtrip() {
+        for (precision, scale) in [(1, 0), (5, 5), (5, 8), (38, 38)] {


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.

Added the comment beside validate_decimal_precision in 0c6d0d5. Scale remains deliberately unconstrained for Java parity; the existing (5, 8) constructor/JSON roundtrip still passes.

laskoviymishka · 2026-10-01T08:14:54Z

+        }
+    }
+
+    fn check_type_serde_roundtrip_value(json: &str, expected_type: Type) {


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).

Strengthened the test in 0c6d0d5 to compare serialization of the parsed value with explicit canonical JSON, through both Type and PrimitiveType. The compact decimal(5,2) input must serialize as decimal(5, 2), and the canonical output is reparsed as well.

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

Comment thread crates/iceberg/src/spec/datatypes.rs Outdated

Comment thread crates/iceberg/src/spec/datatypes.rs Outdated

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

7d88ecb

mkroll-db approved these changes Sep 23, 2026

View reviewed changes

Comment thread crates/iceberg/src/spec/datatypes.rs

Comment thread crates/iceberg/src/spec/datatypes.rs

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

efb7ebb

dannycjones approved these changes Sep 23, 2026

View reviewed changes

mbutrovich reviewed Sep 28, 2026

View reviewed changes

fix(spec): align decimal serde validation with constructor

93a06c0

laskoviymishka approved these changes Oct 1, 2026

View reviewed changes

fix(spec): preserve type parse errors and canonical serde coverage

0c6d0d5

laskoviymishka added this pull request to the merge queue Oct 2, 2026

Merged via the queue into apache:main with commit fc682d7 Oct 2, 2026
25 checks passed

		let precision: u32 = precision.trim().parse().map_err(\|_\| malformed())?;
		let scale: u32 = scale.trim().parse().map_err(\|_\| malformed())?;

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!

Uh oh!

Uh oh!

mkroll-db left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dannycjones left a comment

Choose a reason for hiding this comment

Uh oh!

mkroll-db commented Sep 24, 2026

Uh oh!

mbutrovich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

shoemoney commented Sep 21, 2026 •

edited

Loading