Repository navigation
Conversation
Co-authored-by: Codex <codex@openai.com>
There was a problem hiding this comment.
🟢 Approval recommended
The focused implementation correctly handles decimal128 values and includes comprehensive regression coverage.
0 open findings
What changed in this PR
Adds decimal partition-value support to manifest serialization and deserialization.
Changes:
- Adds a typed nanoarrow decimal128 append helper.
- Decodes decimal128 partition values with their declared precision and scale.
- Tests positive, negative, zero, null, and >64-bit values across manifest versions 1–3.
| File | Description |
|---|---|
src/iceberg/arrow_row_builder.cc |
Implements decimal128 appending. |
src/iceberg/arrow_row_builder_internal.h |
Declares the decimal helper. |
src/iceberg/manifest/manifest_adapter.cc |
Uses typed decimal serialization. |
src/iceberg/manifest/manifest_reader.cc |
Decodes decimal128 partition values. |
src/iceberg/test/manifest_reader_test.cc |
Adds cross-version round-trip coverage. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
zhjwpku
approved these changes
Oct 11, 2026
| ArrowDecimalInit(&value, Decimal::kBitWidth, type.precision(), type.scale()); | ||
| ArrowArrayViewGetDecimalUnsafe(view, row_idx, &value); | ||
| int128_t unscaled; | ||
| std::memcpy(&unscaled, value.words, sizeof(unscaled)); |
Collaborator
There was a problem hiding this comment.
Does it work on big-endian machines? I'm not familiar with arrow's endianness, but I think we should double check.
Member
Author
This branch has not been deployed
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
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.
Writing a manifest with non-null decimal partition values fails with nanoarrow
EINVAL, and reading decimal partitions fails withUnsupported type decimal128 for partition values. Use a typed decimal128 append helper in the manifest adapter and decode decimal128 values with the partition field's precision and scale in the reader.Add a manifest round-trip regression for format versions 1, 2, and 3 covering positive, negative, zero, null, and values larger than 64 bits. These shared fixes are extracted from #876 so decimal support can be reviewed independently of the metadata tables.
Validation:
manifest_testsuite passes: 210 passed, 18 skipped.git diff --checkpass.