Skip to content

Conversation

@vpapp
Copy link
Contributor

@vpapp vpapp commented Aug 22, 2025

Goal of this PR

When encoding fixed.decimal, there are 2 possible errors which manifest when attempting to decode:

  1. the number of digits exceed the precision specified in the schema -- this module decodes these properly because the precision is ignored, but the official org.apache.avro Java decoder errors with DECIMAL_PRECISION_EXCEEDS_MAX_PRECISION

  2. the number of bytes used for encoding exceeds the fixed size specified in the schema -- this shifts subsequent bytes, so the rest of the data cannot be decoded to proper values. The official org.apache.avro Java decoder errors for various malformed data as it tries to decode the wrong bytes.

This PR fixes both of these failure modes by checking the actual precision of the value being encoded for fixed.decimal and bytes.decimal against the precision in the schema, and also the number of bytes being written for fixed.decimal.

I also fixed the max precision calculation in schema_parse.go to match the one in the Avro specification. The original formula was close, but off by one in some cases (e.g. size=3 should allow precision=7 but only allows 6, size=5 should allow 12 but only allows 11, etc).

And I had to update some unit tests because 1734/5 is 346.80, which needs precision=5 to encode if scale=2.

How did I test it?

I added unit tests that expect encoding failure for these cases, which fail without the fixes and pass with the fixes.

@vpapp vpapp force-pushed the decimal-encoding-fix branch from d83b9eb to 91f58e6 Compare August 22, 2025 22:14
Copy link
Member

@nrwiersma nrwiersma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉 Thanks.

@nrwiersma nrwiersma merged commit fa1080c into hamba:main Aug 27, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants