Repository navigation
fix(manifest): round-trip decimal partition values - #998
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.
| ArrowDecimalInit(&value, Decimal::kBitWidth, type.precision(), type.scale()); | ||
| ArrowArrayViewGetDecimalUnsafe(view, row_idx, &value); | ||
| int128_t unscaled; | ||
| std::memcpy(&unscaled, value.words, sizeof(unscaled)); |
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.
Share decimal encoding across direct and generic writers, preserve signed values at the schema fixed width, and reject representation overflow. Cover exact wire bytes and decimal file round trips in both backends.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open findings
🧠 Review effort: Lite
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.