Repository navigation
fix: write and read decimal partition values in manifests - #9
Merged
Merged
Conversation
pkonrad1229
marked this pull request as ready for review
October 2, 2026 14:32
pkonrad1229
force-pushed
the
fix/decimal-partition-values
branch
from
October 2, 2026 14:43
d016d02 to
2bca716
Compare
Author
|
Force-pushed: switched the new test's case table to designated initializers per clang-tidy. |
pkonrad1229
force-pushed
the
fix/decimal-partition-values
branch
from
October 2, 2026 15:33
2bca716 to
cb5484b
Compare
bharathv
approved these changes
Oct 2, 2026
The partition struct child for a decimal field is a nanoarrow DECIMAL128 array, which ArrowArrayAppendBytes rejects with EINVAL, so any manifest entry carrying a decimal partition value failed to write. Append it as an ArrowDecimal instead. Both avro encoders copied all 16 bytes of a decimal128 into a fixed sized for the precision (5 bytes for decimal(10,2)), corrupting the record for any precision below 38. Emit only the fixed's width, big-endian. The manifest reader had no case for decimal partition values, so a manifest carrying one could not be read back. Parse them into decimal literals.
Cover precisions 1 through 38 at every point where the Avro fixed width changes, with max, min, zero and one-ulp values, on both the direct encoder and GenericDatum write paths. Also assert the physical fixed size per precision.
pkonrad1229
force-pushed
the
fix/decimal-partition-values
branch
from
October 5, 2026 06:37
cb5484b to
1152e1f
Compare
Author
|
force-push: rebased to get the CI fix from master. No code changes in this PRs commits |
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
2 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.
Manifests with a decimal partition value could not be written, and if written, were corrupt and unreadable. Three independent bugs on the write → encode → read path:
EINVAL(manifest/manifest_adapter.cc). The partition struct's decimal child is a nanoarrowDECIMAL128array, but the value was appended withArrowArrayAppendBytes, which that array rejects. Now built as anArrowDecimaland appended withArrowArrayAppendDecimal.avro/avro_direct_encoder.cc,avro/avro_data_util.cc). Both encoders copied all 16 bytes of the Arrow decimal128 into an Avrofixedsized for the precision (e.g. 5 bytes fordecimal(10,2)). Avrofixedcarries no length, so the reader takes N bytes and every following field is decoded from the wrong offset. Now emit only the fixed's width (low-order bytes of the little-endian value), reversed to big-endian.Unsupported type(manifest/manifest_reader.cc).ParsePartitionValueshad noDECIMAL128case. Added; it parses intoLiteral::Decimalwith the field's precision and scale.Tests
manifest_writer_versions_test:TestV2WriteDecimalPartitionValueswrites and reads back a manifest partitioned ondecimal(10,2)anddecimal(38,10)(incl. the 38-digit minimum).avro_test:WriteDecimalTypesround-trips precisions 1, 2, 3, 9, 10, 18, 19, 28, 38 — every boundary where the fixed width changes — with max, min, zero and one-ulp values, on both the direct-encoder and GenericDatum paths, and asserts the physical fixed size per precision. Fails on both paths with fix 2 reverted.avro_data_test: existing decimal case now also asserts the encoded fixed is 5 bytes.