iceberg: serialize all data_file fields in manifests#29680
Merged
nvartolomei merged 5 commits intoFeb 25, 2026
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request enhances Iceberg manifest serialization to include all data_file fields from the v2 specification, ensuring full compatibility with manifests produced by Spark and other Iceberg engines. The PR introduces a reusable Avro comparison utility and updates test infrastructure to verify correct roundtrip serialization of all fields.
Changes:
- Added comprehensive serialization support for all data_file fields including lower_bounds, upper_bounds, key_metadata, split_offsets, equality_ids, sort_order_id, and referenced_data_file
- Introduced a reusable avro_comparator test utility with support for schema evolution (subset matching)
- Updated field types from size_t to int64_t to match Iceberg specification
- Enhanced test coverage with Spark-generated manifest data and improved test data generation
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/v/serde/avro/tests/avro_comparator.h | New utility for deep comparison of Avro GenericDatum trees with schema evolution support |
| src/v/serde/avro/tests/avro_comparator_test.cc | Test suite for the avro_comparator utility |
| src/v/serde/avro/tests/parser_test.cc | Refactored to use avro_comparator, removing duplicate comparison logic |
| src/v/iceberg/manifest_entry.h | Updated data_file struct to use optional fields with int64_t values |
| src/v/iceberg/manifest_entry.cc | Updated copy methods to handle new optional fields |
| src/v/iceberg/manifest_entry_values.cc | Implemented serialization/deserialization for all data_file fields |
| src/v/iceberg/manifest_entry_type.cc | Added referenced_data_file field to schema |
| src/v/iceberg/avroschemas/manifest_entry.schema.json | Added referenced_data_file field definition |
| src/v/iceberg/tests/manifest_serialization_test.cc | Enhanced tests to verify null/non-null field handling and Spark compatibility |
| src/v/iceberg/tests/gen_test_iceberg_manifest.py | Updated to use uv with PEP 723 metadata and generate test data with varied field values |
| src/v/iceberg/tests/testdata/* | Updated test data files and README with new generation instructions |
| src/v/serde/avro/tests/BUILD | Added avro_comparator library and test targets |
| src/v/iceberg/tests/BUILD | Added Spark manifest test data dependency |
nvartolomei
force-pushed
the
nv/iceberg-manifest-serde
branch
from
894190a to
d5619d1
Compare
Collaborator
nvartolomei
force-pushed
the
nv/iceberg-manifest-serde
branch
from
d5619d1 to
825bcb2
Compare
Extract the GenericDatum comparison logic from parser_test into a reusable avro_comparator.h with unit tests. - Fix copy-paste bugs in AVRO_ENUM and AVRO_FIXED that compared expected against itself instead of actual. - Check union branch index before comparing inner values; the old AVRO_UNION switch arm was dead code since type() unwraps unions. - Path-based error messages for easier debugging of nested mismatches. - Named test parameters for schema-based roundtrip tests.
Add a GenericDatum-based Avro comparison path for manifest serialization tests. This catches field loss that parse/serialize roundtrips can hide. Wire manifest tests to the shared avro comparator helper and emit mismatch diagnostics from the roundtrip loop for faster debugging.
nvartolomei
force-pushed
the
nv/iceberg-manifest-serde
branch
4 times, most recently
from
6422e5b to
4c00355
Compare
Make count maps (column_sizes, value_counts, null_value_counts, nan_value_counts) optional in data_file to preserve the distinction between null and empty map through Avro roundtrip. Add lower_bounds, upper_bounds, key_metadata, split_offsets, equality_ids, and sort_order_id to data_file and implement their serialization/deserialization. Update test data to cover both null and empty map cases.
Add the referenced_data_file field (field-id 143) to the data_file schema, struct, and serde to match the full Iceberg v2 spec. Add a roundtrip test using a Spark 3.5.5 / Iceberg 1.8.1 generated manifest. Make the avro comparator's extra-null-fields tolerance opt-in via an allow_extra_null_fields flag for schema evolution scenarios where the writer has more fields than the original data.
nvartolomei
force-pushed
the
nv/iceberg-manifest-serde
branch
from
4c00355 to
d455088
Compare
oleiman
reviewed
Feb 24, 2026
oleiman
left a comment
oleiman
left a comment
Member
There was a problem hiding this comment.
looking good on first pass, some nits and a question about a field that got dropped from the TODO
Comment on lines
41
to
42
| size_t record_count; | ||
| size_t file_size_bytes; |
Member
There was a problem hiding this comment.
should these also be int64 as well? I think they will get assigned to one in the snapshot
Contributor
Author
There was a problem hiding this comment.
Yes. Began changing them but then didn't want to get too distracted from the main task. To be done.
Comment on lines
277
to
278
| if (fs.size() < 16) { | ||
| throw std::invalid_argument("Expected more values"); |
Member
There was a problem hiding this comment.
nitpick: is it worth naming this constant and/or sticking it in the exception content? not sure the path this exception takes or whether it's at all actionable 🤷
Comment on lines
+195
to
+196
| for (size_t i = 0; i < expected_map.size(); ++i) { | ||
| if (expected_map[i].first != actual_map[i].first) { |
Member
There was a problem hiding this comment.
micro-nitpick: I think these are populated by iterating unordered maps, so maybe we should compare them in an order independent way.
Contributor
Author
There was a problem hiding this comment.
Added as a separate commit.
Should have been earlier in the commit chain but some of these rebases take too long...
andrwng
previously approved these changes
Feb 25, 2026
oleiman
previously approved these changes
Feb 25, 2026
oleiman
left a comment
oleiman
left a comment
Member
There was a problem hiding this comment.
"missing" field is deprecated
Avro maps are stored as ordered vectors of key-value pairs, but the Avro specification treats them as unordered. Different serializers (e.g. our writer vs Spark/pyiceberg) may emit the same logical map with keys in different order. Add a by_key_unique map matching mode that compares entries by key lookup instead of position, rejecting duplicate keys. This is now the default since most callers compare logically equivalent maps. The positional mode is retained for the parser roundtrip test where the random data generator can produce duplicate map keys and the test verifies exact binary-level fidelity rather than logical equivalence. Comparison options (extra_fields_policy, map_matching_policy) are grouped into a compare_options struct with sensible defaults.
Contributor
Author
|
Change since last review round:
|
oleiman
approved these changes
Feb 25, 2026
Comment on lines
+52
to
+53
| inline std:: | ||
| expected<std::unordered_map<std::string, size_t>, ::testing::AssertionResult> |
andrwng
approved these changes
Feb 25, 2026
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.
Serialize all
data_filefields in Iceberg manifest entries. Previously,optional fields like
sort_order_id,split_offsets,equality_ids,and the column size/count maps were stubbed out as nulls. Iceberg
implementations in the wild often do not handle missing optional fields
well, and third-party metadata rewriters that compute these fields would
have their work discarded on the next manifest rewrite by us.
https://redpandadata.atlassian.net/browse/CORE-13459
Backports Required
Release Notes
Improvements
merge_append_actionmanifest rewriting.