fix: add space in decimal type serialization for cross-engine compat#2538
Open
SreeramGarlapati wants to merge 1 commit into
Open
fix: add space in decimal type serialization for cross-engine compat#2538SreeramGarlapati wants to merge 1 commit into
SreeramGarlapati wants to merge 1 commit into
Conversation
…pliance The Iceberg spec, Java, and Python all serialize the decimal type as `decimal(P, S)` (with a space after the comma). iceberg-rust was writing `decimal(P,S)` which could cause metadata written by Rust to be rejected or misinterpreted by strict parsers in other implementations. The deserializer already handles both formats via trim(), so this change is backwards-compatible for reading. Closes apache#2534 Co-authored-by: rawataaryan9 <rawataaryan9@users.noreply.github.com>
SreeramGarlapati
force-pushed
the
fix/decimal-type-serialization-spacing
branch
from
945b288 to
3499e4b
Compare
mbutrovich
approved these changes
Jun 1, 2026
Collaborator
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
LGTM, thanks @SreeramGarlapati!
xanderbailey
reviewed
Jun 1, 2026
Contributor
xanderbailey
left a comment
xanderbailey
left a comment
There was a problem hiding this comment.
Interestingly the spec makes it seem like this is legitimate both with and without the space? It would appear that if other consumers are failing here, they're not compliant?
Interested to know what other people in the community think the right approach here is? It's true that this fix will align with other clients but I also feel like perhaps we either need to clarify the spec or improve the other clients to be compliant?
mbutrovich
reviewed
Jun 1, 2026
Collaborator
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
Changing my approval after the better investigation by @xanderbailey, thanks for double-checking!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
iceberg-rust serializes the decimal type as
decimal(P,S)(no space), while the Iceberg spec, Java, and Python all usedecimal(P, S)(space after comma). This mismatch can cause issues when metadata written by iceberg-rust is read by strict parsers in other implementations.What changed
serialize_decimal:decimal({precision},{scale})→decimal({precision}, {scale})Display for PrimitiveType: same fix for the decimal armWhy this is safe
The deserializer already uses
.trim()on both precision and scale components, so it accepts bothdecimal(9,2)anddecimal(9, 2). Existing metadata files with the old format continue to be readable.Closes #2534
Test plan