perf(manifest): upgrade to apache-avro 0.22 and read manifest entries without Value

[mbutrovich]

Which issue does this PR close?

• Closes Upgrade to using apache-avro 0.22 #3063.
• Part of [EPIC] Fast manifest reads for PyIceberg through pyiceberg-core #3262 (Task 1).

What changes are included in this PR?

pyiceberg-core reads manifests 4x to 5x slower than PyIceberg's Cython reader (#3262), and the profile linked there puts about 82% of the Rust parse inside apache-avro 0.21. For each entry, 0.21 decodes the bytes into a tree of Values, cloning every field name into a new String (decode.rs#L265-L285), rebuilds the tree to resolve it against the reader schema (types.rs#L1092-L1152), and then converts it into the serde struct. 0.22's schema-aware deserializer (apache/avro-rs#512) decodes the bytes into the serde struct in one pass.

entries	main (0.21)	this PR
1,000	21.2 ms	3.7 ms
10,000	211.5 ms	36.8 ms
50,000	1058.6 ms	181.1 ms

These are the best of 21 runs of Manifest::parse_avro, in three interleaved rounds of 7 per side, in a release build on an Apple M5 Max. The manifests are V2 manifests that PyIceberg wrote with deflate, 12 columns with full stats, and an identity partition.

Benchmark harness

Generate the manifests with PyIceberg installed:

python gen_manifests.py /tmp/manifests

Then run the harness against main, switch the iceberg dependency to this PR's head commit, and run it again:

cargo run --release -- /tmp/manifests/manifest_1000.avro /tmp/manifests/manifest_10000.avro /tmp/manifests/manifest_50000.avro

Cargo.toml:

[package]
name = "manifest-parse-bench"
version = "0.0.0"
edition = "2024"
publish = false

[dependencies]
# apache/iceberg-rust main, which uses apache-avro 0.21.
iceberg = { git = "https://github.com/apache/iceberg-rust", rev = "ecba0b537a1f78ec8125c1a26f6dfab3f95b509e" }
# apache/iceberg-rust#3354. Replace the line above with this one.
# iceberg = { git = "https://github.com/mbutrovich/iceberg-rust", rev = "91486882b29deb0d97560543e0764aa00a2988fd" }

[workspace]

src/main.rs:

use std::time::Instant;

use iceberg::spec::Manifest;

fn main() {
    for path in std::env::args().skip(1) {
        let bytes = std::fs::read(&path).unwrap();
        let entries = Manifest::parse_avro(&bytes).unwrap().entries().len();
        let mut runs: Vec<f64> = (0..7)
            .map(|_| {
                let start = Instant::now();
                std::hint::black_box(Manifest::parse_avro(std::hint::black_box(&bytes)).unwrap());
                start.elapsed().as_secs_f64() * 1e3
            })
            .collect();
        runs.sort_by(f64::total_cmp);
        println!("{entries:>6} entries  best {:8.2} ms  median {:8.2} ms", runs[0], runs[3]);
    }
}

gen_manifests.py:

"""Write PyIceberg V2 manifests (deflate, 12 columns with full stats, identity partition)."""

import sys

from pyiceberg.io.pyarrow import PyArrowFileIO
from pyiceberg.manifest import DataFile, DataFileContent, FileFormat, ManifestEntry, ManifestEntryStatus, write_manifest
from pyiceberg.partitioning import PartitionField, PartitionSpec
from pyiceberg.schema import Schema
from pyiceberg.transforms import IdentityTransform
from pyiceberg.typedef import Record
from pyiceberg.types import (
    BooleanType,
    DateType,
    DoubleType,
    FloatType,
    IntegerType,
    LongType,
    NestedField,
    StringType,
    TimestampType,
)

SCHEMA = Schema(
    NestedField(1, "id", LongType(), required=False),
    NestedField(2, "category", StringType(), required=False),
    NestedField(3, "v_int", IntegerType(), required=False),
    NestedField(4, "v_long", LongType(), required=False),
    NestedField(5, "v_float", FloatType(), required=False),
    NestedField(6, "v_double", DoubleType(), required=False),
    NestedField(7, "v_str", StringType(), required=False),
    NestedField(8, "v_bool", BooleanType(), required=False),
    NestedField(9, "v_date", DateType(), required=False),
    NestedField(10, "v_ts", TimestampType(), required=False),
    NestedField(11, "v_str2", StringType(), required=False),
    NestedField(12, "v_long2", LongType(), required=False),
)
SPEC = PartitionSpec(PartitionField(source_id=2, field_id=1000, transform=IdentityTransform(), name="category"), spec_id=0)


# Single-value serialization widths per field id. String columns use 16 ASCII bytes.
WIDTHS = {1: 8, 3: 4, 4: 8, 5: 4, 6: 8, 8: 1, 9: 4, 10: 8, 12: 8}


def bound(f: int, v: int) -> bytes:
    if f in WIDTHS:
        return (v % (2 ** (8 * WIDTHS[f] - 1))).to_bytes(WIDTHS[f], "little")
    return f"{v:016d}".encode()


def entry(i: int) -> ManifestEntry:
    ids = range(1, 13)
    data_file = DataFile.from_args(
        content=DataFileContent.DATA,
        file_path=f"s3://bucket/warehouse/db/table/data/category=c{i % 10}/{i:08d}-0-6f1e1a2b-3c4d-5e6f-7a8b-9c0d1e2f3a4b.parquet",
        file_format=FileFormat.PARQUET,
        partition=Record(f"c{i % 10}"),
        record_count=10_000 + i,
        file_size_in_bytes=1_000_000 + i,
        column_sizes={f: 1000 + f + i for f in ids},
        value_counts={f: 10_000 + i for f in ids},
        null_value_counts={f: i % 7 for f in ids},
        nan_value_counts={5: 0, 6: 0},
        lower_bounds={f: bound(f, i * 13 + f) for f in ids},
        upper_bounds={f: bound(f, i * 17 + f + 1000) for f in ids},
        split_offsets=[4],
        sort_order_id=0,
    )
    return ManifestEntry.from_args(
        status=ManifestEntryStatus.ADDED, snapshot_id=1, sequence_number=1, file_sequence_number=1, data_file=data_file
    )


def main(out_dir: str) -> None:
    io = PyArrowFileIO()
    for n in (1_000, 10_000, 50_000):
        path = f"{out_dir}/manifest_{n}.avro"
        with write_manifest(2, SPEC, SCHEMA, io.new_output(path), snapshot_id=1, avro_compression="deflate") as writer:
            for i in range(n):
                writer.add_entry(entry(i))
        print(path)


if __name__ == "__main__":
    main(sys.argv[1])

• Upgrade apache-avro to 0.22 and port to its API. Written manifests don't change.
• Read manifest entries with Reader::into_deser_iter through a new Resolved<T> adapter in avro/deserializer.rs, which applies the schema resolution rules that 0.22's deserializer skips. RawLiteral::project_by_name matches partition fields to the spec by name, as resolution did. The manifest list reader is unchanged, because perf: parse manifest list entries without buffering Avro values #3224 covers it.
• Define each named Avro type once when writing. schema_to_avro_schema repeats fixed_{L} and decimal_{P}_{S} definitions, which the Avro specification doesn't allow. Both apache-avro versions reject a repeated fixed_{L}, so main can't write two fixed[L] partition fields of the same length. 0.22 also rejects a repeated decimal, such as identity and truncate partitions on one decimal column.
• Keep reading manifests that iceberg-rust wrote with 0.21 and repeated decimal definitions. 0.22 rejects their header, so the reader rewrites each identical repeat as a reference. It logs the first such manifest in a process at warn level, with its path, and later ones at debug level.
• Store StandardKeyMetadata's file_length as an Avro long explicitly, because 0.22 serializes a u64 as fixed.
• Ignore test_schema_with_array_map until a release includes the fix for regression bug, logicalType is dropped from custom attributes avro-rs#654. It only affects avro_schema_to_schema, which has no callers outside tests. Manifest writes aren't affected, because the writer builds its Avro schema in code without parsing it, and a test checks that written headers keep logicalType: map.
• The PR adds about 2,150 lines, not counting Cargo.lock and the binary fixtures. Tests, test helpers, and test data are about two thirds of them (about 1,450 lines). The Resolved<T> adapter in avro/deserializer.rs is 280 lines, and most of its code forwards serde visit_* methods.

Behavior changes

• An int field written as long, such as equality_ids from PyIceberg before Manifest: Use int for equality_ids in manifest schema per Iceberg spec (#3840) iceberg-python#3842, still reads, but a value outside the int range is now an error instead of wrapping.
• A manifest with a uuid partition in the spec's Avro form, fixed(16) with logical type uuid as Java writes it, now reads. On main, resolving it against iceberg-rust's string uuid schema turns the value into a string, which RawLiteral rejects. Writing a uuid partition still fails, which Partitions with type UUID fail to create transactions #2913 tracks, and the writer's Avro schema still declares uuid as a string. [EPIC] Fast manifest reads for PyIceberg through pyiceberg-core #3262 lists writing the spec's fixed(16) form as a follow-up.

Fields are still matched by name rather than by field ID. #3262 tracks that.

Are these changes tested?

Yes, with unit tests, fixtures written by main and by PyIceberg, and a comparison against main:

• Tests in spec/manifest/mod.rs read manifests whose writer schema differs from iceberg-rust's in field order, field set, numeric types, unions, record names, and the Avro type of uuid. All of them except test_parse_manifest_with_fixed_uuid_partition also pass on main. That test fails on main with Unable to convert raw literal (string) fail convert to type uuid.
• test_parse_manifest_without_each_field removes each manifest entry and data_file field from the writer schema in turn. The 18 fields that the spec lets a writer omit read as their defaults, and the 7 required fields return an error.
• test_parse_manifest_written_by_pyiceberg reads testdata/manifests/pyiceberg-v2-data.avro, which PyIceberg 0.12.0 wrote, and compares the whole manifest.
• Round-trip tests write and read repeated decimal and fixed partition types, both as written now and with the header rewritten to repeat each definition as 0.21 wrote it. testdata/manifests/repeated-decimal-type-definitions.avro, written by main, also covers the reader fallback, and unit tests cover the header rewrite.
• test_read_data_files_with_repeated_named_type_definitions checks that read_data_files_from_avro reads data files with repeated definitions. 0.22 checks for repeated names only when the reader has no reader schema, and this path passes one.
• Unit tests in avro/deserializer.rs read each Avro type through Resolved into plain and Option targets.
• test_write_manifest_header_marks_int_keyed_maps checks that written headers keep logicalType: map, and test_schema_with_array_map_without_parsing covers the array-map conversion while test_schema_with_array_map is ignored.
• Parsing PyIceberg manifests with 1,000, 10,000, and 50,000 entries gives identical entries on main and on this PR.

AI Disclosure

I wrote this PR with help from Claude but understand and support the core approach, implementation, and test coverage.

[laskoviymishka]

The entry-without-Value read path is the right direction, and the writer-side named-type dedup plus the key-metadata i64 hardening are clean. A few things on the compatibility surface need pinning down before this merges, though.

The biggest one: dropping reader-schema resolution makes the _serde structs the entire compatibility contract for every on-disk manifest. There's no systematic check that each spec-optional/defaulted field is tolerated when a Java, PyIceberg, or older-Rust writer omits it, and the fixtures are one hand-generated file plus synthetic JSON through this crate's own writer — no real foreign-writer manifests. A future edit to DataFileSerde can now silently break old files, with nothing tying the structs back to the manifest schema.

Second, the legacy repeated-definition fallback is a public behavior change, not just a shim. Only try_from_avro_bytes got it, so read_data_files_from_avro — which reads bytes this crate itself wrote — now hard-fails on previously-readable data. That path, plus the warn-on-every-scan and the second full buffer copy, wants one shared legacy-tolerant open with a stated sunset.

Third, the ~5-6x number is the whole justification for the dependency bump, the 280-line adapter, and the shim, and it isn't reproducible from the diff — no bench, no representative fixture. A criterion bench over a realistic manifest (wide bounds + partition columns) would settle it.

Before merge I'd want:

• a benchmark (or linked reproducible numbers) backing the perf claim
• confirmation the writer still emits logicalType: map in the header it writes — avro-rs #654 drops it on parse, so a header-JSON assertion would prove it isn't a silent cross-engine interop regression hiding behind the #[ignore]
• the legacy fallback shared across the reader paths, with the error-masking and warn-storm points (flagged inline) addressed
• at least one real Java/PyIceberg-written manifest in the fixtures, and the contract — new _serde fields must be Option or #[serde(default)] — written down

Separately, the file_length i64 move and the uuid schema change are unrelated to manifest-entry performance and would bisect more cleanly as their own commits. The rest is inline.

[laskoviymishka]

The deserializer's union handling rides on how 0.22's deserialize_any dispatches unions, which isn't a documented public contract — a 0.22.x minor bump could shift it under us. Worth a comment here that the adapter semantics are pinned to this version, and keeping test_parse_manifest_reads_union_writer_fields_as_{required,optional} as the guard for the next bump.

[mbutrovich]

The deserializer's union handling rides on how 0.22's deserialize_any dispatches unions, which isn't a documented public contract

As I read serde's Deserializer contract, deserialize_any on a self-describing format dispatches on the data. 0.22's schema-aware deserialize_any reads a union's branch index and continues with that branch's schema (deser_schema/mod.rs#L228-L272). The new tests in avro/deserializer.rs read union and non-union writer values into plain and Option targets. A 0.22.x release that changes the dispatch fails them, along with the two union tests you named. Would those tests work as the guard instead of a comment in Cargo.toml?

[laskoviymishka]

define_once now emits AvroSchema::Ref for a repeated fixed/decimal, but AvroSchemaToSchema has no Ref arm, so an iceberg→avro→iceberg round-trip on two same-typed decimal or fixed columns hits the unsupported-primitive path. Correctness here also leans on the visitor reaching names in serialization order, which no test pins. A round-trip test with two decimal(10,2) fields — one nested in a map value or list element — would cover both; otherwise resolve Ref in the visitor or document it as write-only.

[mbutrovich]

define_once now emits AvroSchema::Ref for a repeated fixed/decimal, but AvroSchemaToSchema has no Ref arm

avro_schema_to_schema has no callers outside tests. It's marked #[allow(unused)] with a TODO to fix it once something uses it (schema.rs#L598-L602). On main it already returns an error for any parsed schema that refers to a named type by name, because a reference falls through to the error arm of primitive (schema.rs#L538-L563). What do you think about adding Ref support when something starts calling the converter?

Correctness here also leans on the visitor reaching names in serialization order, which no test pins.

test_schema_to_avro_schema_defines_named_types_before_references covers this. It converts a schema with decimal(10, 2) at the top level, in a list element, and in a map value with int keys. It then parses the serialized Avro schema and checks that decimal_10_2 is defined once. apache-avro 0.22 fails to parse a reference that comes before its definition ("Unknown primitive type"), so the parse fails if the visitor order ever differs from the serialization order.

[laskoviymishka]

The new read test accepts the spec's fixed(16) uuid, but the writer still picks UuidSchema::String here — so we read the spec form and don't write it, and a Java/PyIceberg reader resolving our manifest sees a string writer schema against its fixed reader schema. The bytes match 0.21 so it isn't a regression, but the description frames the uuid change as read-only. I'd either emit UuidSchema::Fixed (name uuid_fixed, size 16, same define-once handling) or call the choice out explicitly.

[mbutrovich]

I'd either emit UuidSchema::Fixed (name uuid_fixed, size 16, same define-once handling) or call the choice out explicitly.

Writing the spec's form changes the bytes that iceberg-rust writes, and this PR keeps written manifests unchanged. #3262 lists the writer change as a follow-up that needs a compatibility check, and it overlaps #2913 and #2916. Writing a uuid partition fails today (#2913), and the description's "Behavior changes" section says so. Would you be OK leaving the writer change to that follow-up?

[laskoviymishka]

Writer::new is fallible now, and this expect adds a panic path in library code — the repo convention here is to not panic, and data_file.rs / manifest/writer.rs already propagate the same call with ?. The surrounding fn returns Result, so I'd just ? it with an error map for consistency.

[mbutrovich]

The surrounding fn returns Result, so I'd just ? it with an error map for consistency.

If I'm reading it right, ManifestListWriter::new returns Self, and so do the public v1, v2, and v3 constructors that call it (writer.rs#L58-L157). Using ? would make all three fallible, which breaks the public API. 0.22's Writer::new fails only when it can't resolve the schema's names, and the three manifest list schemas are static. The expect matches the existing one for add_user_metadata a few lines below it. Would you be OK keeping it?

[laskoviymishka]

small thing while we're here — these two use the bare raw_literal_project_by_name_* names where the rest of the file prefixes with test_ (e.g. test_datum_to_decimal_rejects_scale_change).

[mbutrovich]

these two use the bare raw_literal_project_by_name_* names

Renamed both with the test_ prefix.

[mbutrovich]

Thanks for the thorough review @laskoviymishka. I pushed changes for most of the inline comments and replied in each thread, including the ones where I'd like to keep the current behavior.

Before merge I'd want:

• a benchmark (or linked reproducible numbers) backing the perf claim

The harness and the PyIceberg script that generates its manifests are in a collapsed section of the description, with the commands to run them. The harness times Manifest::parse_avro and prints the best and median of 7 runs in a release build. Pointing its iceberg dependency at main or at this PR's head commit reproduces the table in the description. I kept it out of the repo because the repo has no criterion dependency or benches/ directory, and @blackmwk asked to keep benchmark harnesses out of the repo for now because of their maintenance cost (#2558 (comment)). Adding in-repo benchmarks seems like its own discussion.

• confirmation the writer still emits logicalType: map in the header it writes

test_write_manifest_header_marks_int_keyed_maps writes a manifest and finds "logicalType":"map" six times in the header's schema JSON, once each for column_sizes, value_counts, null_value_counts, nan_value_counts, lower_bounds, and upper_bounds. The writer builds its Avro schema in code and serializes it without parsing it, so apache/avro-rs#654 doesn't reach it. Details are in the schema.rs thread.

• at least one real Java/PyIceberg-written manifest in the fixtures

pyiceberg-v2-data.avro is a manifest written by PyIceberg 0.12.0, and test_parse_manifest_written_by_pyiceberg compares every field of both entries. The testdata/manifests/README.md includes the script that generates it. Details are in the mod.rs line 99 thread.

Separately, the file_length i64 move and the uuid schema change are unrelated to manifest-entry performance and would bisect more cleanly as their own commits.

Both come from the version bump rather than the reader change, and only the file_length one can be split out. apache-avro 0.22 serializes a u64 as an 8-byte fixed (serde/ser.rs#L155-L157), where 0.21 serialized it as a long (ser.rs#L156-L162). With the field left as u64, encoding any key metadata that has a file length fails on 0.22 with "Failed to encode key metadata", and test_roundtrip_with_length, test_decode_tolerates_trailing_bytes, and test_load_manifest_decrypts_when_key_metadata_present fail. The type change also works on 0.21. There, the only difference is that a file length above i64::MAX returns DataInvalid instead of Unexpected, because 0.21 already rejects it and already rejects a negative length on decode. Would you like me to move it to a PR that lands on main before this one?

The uuid changes can't be split out. The only non-test uuid changes rename Schema::Uuid to 0.22's Schema::Uuid(UuidSchema), which this PR needs in order to compile. Reading the spec's fixed(16) uuid follows from the new reader no longer resolving the writer schema against iceberg-rust's string uuid schema, and test_parse_manifest_with_fixed_uuid_partition documents that.

[laskoviymishka]

This turned over cleanly — most of round 1 is closed. The map-header assertion landed as test_write_manifest_header_marks_int_keyed_maps, the named_types.rs slice panic is gone (try_from + .get + DataInvalid), the warn-storm is a warn-once behind WARNED_REPEATED_DEFINITIONS, and the compat contract is written down in the _serde module doc with test_parse_manifest_without_each_field behind it. The real PyIceberg 0.12 fixture via test_parse_manifest_written_by_pyiceberg is exactly the foreign-writer coverage I was after.

One correction on my end: I claimed in round 1 that read_data_files_from_avro would hard-fail on repeated named-type definitions. You showed that was wrong, and test_read_data_files_with_repeated_named_type_definitions proves the reader-schema path reads 0.21 output fine — withdrawing that, and agreed we don't need a shared fallback (#7); the test is the guard.

On the rest of your open questions:

• (#1) yes, the union/non-union tests are a fine guard in place of a Cargo.toml comment — they fail on a real deserialize_any semantics change. A one-line note on the dep line is optional.
• (#5) deferring the uuid writer to #3262/#2913 is fine since the written bytes don't change; I'd just drop a // TODO(#2913) at the UuidSchema::String site and note the read-fixed/write-string asymmetry in the description.
• (#11) keep the .expect — ? would make the public v1/v2/v3 constructors fallible, which is the worse trade. I'd add one small test constructing all three writers so a future schema edit fails in CI, and reword the message to state the invariant.
• (#14) keep both fixtures; the real 0.21 binary and the generated-bytes helper cover different things.

What I'd still want before merge: the contract claims "every format version" but the audit test only exercises V2 — V1 reads a different shape and ships untested (inline). The error-masking fix only covers the first reparse; the second AvroReader::new can still swap the original error (inline). The deserializer doc lists what the adapter does but not what it drops, with no differential check against the old resolving reader, so an unintended divergence — a mismatched all-null column, say — wouldn't be caught (inline). And the by-name partition projection has a sharper failure mode than the doc caveat lets on (inline). Separately, the ~5-6x is still unbacked — no bench landed; a criterion bench in crates/iceberg/benches or a link to the harness numbers would let us watch it.

This is close now. The V1 audit is the one I'd really want nailed before merge — it's the whole "serde structs are the compat layer" claim resting on an untested version — along with the three-line error-masking fallback. The partition name-mismatch and the differential test I'd love to see, and the bench is still the one round-1 ask left hanging, but none of those three has to gate the merge.

[laskoviymishka]

This doc lists what the adapter does but not what it drops versus the old resolving reader — aliases, reader-schema defaults, string-to-bytes promotion, name/order matching outside the partition record. That gap bites because a type mismatch the old path rejected up front now only surfaces as a serde error when a value is actually hit, so a mismatched column whose values are all null reads clean. I'd add the "doesn't support" list here, and — the real guard — a differential test running the fixtures plus a few synthetic variants through both the 0.22 with_schema reader and Resolved, asserting equal results, so any divergence is intentional rather than something we eyeball.

[mbutrovich]

a type mismatch the old path rejected up front now only surfaces as a serde error when a value is actually hit, so a mismatched column whose values are all null reads clean.

If I'm reading 0.21 right, the old path didn't check types up front either. Reader::with_schema only compares the writer and reader schemas for equality, and the reader resolves each value as it reads it. A null resolves to the null branch of the reader's union (types.rs#L1020-L1044) whatever the writer's other branch is. So main also reads a mismatched column whose values are all null without an error.

aliases, reader-schema defaults, string-to-bytes promotion, name/order matching outside the partition record

As I read main, the reader schema used few of these.

• It never set aliases (schema.rs#L95-L104).
• Its only defaults were null for optional fields (schema.rs#L88-L94). The Option fields and #[serde(default)] on content cover them, and the two field audits check each one.
• A string written for a bytes field still reads, because serde_bytes::ByteBuf accepts a string.
• Fields outside the partition record match by name in any order through serde. The writer-schema tests in spec/manifest/mod.rs cover reordered fields.

Given that, do you see a manifest that the two readers would read differently? If you do, I'll add it as a fixture test.

[laskoviymishka]

This contract claims every format version, but test_parse_manifest_without_each_field only builds manifest_schema_v2/ManifestEntryV2. V1 reads a different shape — snapshot_id is a plain i64 there, not Option, and the V1-only fields aren't exercised — so a V1 regression ships without this test catching it. I'd either add a V1 variant or narrow the wording to V2/V3; while deciding, worth settling whether a null V1 snapshot_id has to be tolerated, since Java can write that for inheritance.

[mbutrovich]

This contract claims every format version, but test_parse_manifest_without_each_field only builds manifest_schema_v2/ManifestEntryV2.

test_parse_v1_manifest_without_each_field does the same for V1. It starts from manifest_schema_v1 with every field set and removes one field at a time. The 9 fields that v1 marks optional read as their defaults. The 8 required fields, including snapshot_id, fail with an error that names the field. block_size_in_bytes is required in v1 and absent in v2, so the spec's read rules say to ignore it. The test checks that a manifest without it reads. Both audits share the loop in assert_reads_without_each_field.

while deciding, worth settling whether a null V1 snapshot_id has to be tolerated, since Java can write that for inheritance.

The spec marks snapshot_id required in v1, and Java writes it as null in v1 when a table sets compatibility.snapshot-id-inheritance.enabled. main rejects that manifest too. apache-avro 0.21 takes the null out of the writer's union and rejects it as a long. I opened #3371 with a reproduction against main. Java and the spec disagree here, so could we settle it in that issue rather than in this PR?

[laskoviymishka]

The caveat here is accurate, but the consequence is sharper than it reads: partition fields are all optional, so if the writer's names don't line up at all — Java sanitizes ts.day to ts_x2Eday — every field misses and reads as Null with no error, and this is now the sole safeguard since we dropped Avro's own resolution. All-null partition values corrupt pruning and nothing downstream can tell. I'd error when the record has fields but none of them match any spec field, or sanitize field.name the way Java's makeCompatibleName does before the lookup.

[mbutrovich]

if the writer's names don't line up at all

main reads these the same way. I wrote a V2 manifest with an identity partition on the nested column a.b and named the Avro field a_x2Eb, as Java's TypeToSchema does. main and this PR both read the partition value as null. With the spec's field also named a_x2Eb, main reads the value. On main, the reader schema gives each optional partition field a null default (schema.rs#L88-L94), and 0.21 uses that default when the writer has no field with that name.

Java writes each partition field's field-id, so matching by field ID fixes this. It's the first follow-up in #3262, which also covers the read side of #2536. This PR keeps the current name matching, so would it work for you to handle this there?

[mbutrovich]

Thanks @laskoviymishka. I pushed a V1 version of the field audit and the fix for the second error path, and replied in each thread. Some of these apply to main with apache-avro 0.21 as well, so I'd like to keep this PR to the upgrade and track them separately. The details are in each thread.

I'd just drop a // TODO(#2913) at the UuidSchema::String site and note the read-fixed/write-string asymmetry in the description.

I added the TODO and a sentence to "Behavior changes" in the description.

I'd add one small test constructing all three writers so a future schema edit fails in CI, and reword the message to state the invariant.

I reworded the message. test_manifest_list_writer_v1, test_manifest_list_writer_v2, and test_manifest_list_writer_v3 already construct each writer through this expect, so a schema edit that breaks it fails them.

a criterion bench in crates/iceberg/benches or a link to the harness numbers would let us watch it.

The table in the description comes from the harness in its collapsed "Benchmark harness" section, which runs against main and against this PR's head commit.

[laskoviymishka]

This is basically there now. Both round-2 gates closed: test_parse_v1_manifest_without_each_field runs the real Resolved<ManifestEntryV1> path and pins the required set against the 9 v1-optional resets plus the block_size_in_bytes no-op, and the reparse arm now returns the original AmbiguousSchemaDefinition with the retry error as context — ..._reports_original_error holds both strings. That puts the "serde structs are the whole compat contract" worry from round 1 to bed.

Two of my round-2 wants I'll withdraw, because you were right on both. On the differential test, I was picturing 0.21 type-checking up front; it didn't — Reader::with_schema only compared schemas for equality and resolved each value lazily, so the all-null mismatched-column case was never a regression. And on the partition by-name mismatch, main builds its reader schema from the unsanitized spec names too, so a Java-sanitized a_x2Eb field read as a default null there as well — same behavior, not new here; field-id matching (#3262) is the real fix.

What's left is small. The error-return is right, but the warn/swap(true) still fire before the retry parse, so a rewrite that also fails logs a success message and spends the warn-once slot (inline, with the swallowed define_named_types_once Err beside it). And I'd like to confirm whether a null V1 snapshot_id is a shape we need to tolerate — the test pins it required per spec, but Java's wrapFileSchema writes it optional for list inheritance (inline). The ~5-6x bench is still the one round-1 ask hanging, and as last round it doesn't gate.

Fix the warn-ordering and settle the snapshot_id question and I'm glad to see this land.

[laskoviymishka]

The V1 audit landed exactly as I wanted — the required set and the 9 optional-reset cases plus the block_size_in_bytes no-op cover the real path. One thing I want to confirm before we call the contract closed, not a blocker: Java's V1Metadata.wrapFileSchema uses ManifestEntry.SNAPSHOT_ID, which is optional(1, …), so a Java-written V1 manifest can carry ["null","long"] here and snapshot_id: i64 would reject the null. The spec's V1 column lists it required, so pinning it here isn't wrong — have you seen a null V1 snapshot_id in the wild (inheritance writers defer the id)? If it's a shape we need to tolerate, Option<i64> plus a null case covers it; if not, a one-line note that it's required-per-spec / Java-writes-optional-for-list-inheritance is plenty.

[mbutrovich]

have you seen a null V1 snapshot_id in the wild (inheritance writers defer the id)?

I haven't.

If it's a shape we need to tolerate, Option<i64> plus a null case covers it; if not, a one-line note that it's required-per-spec / Java-writes-optional-for-list-inheritance is plenty.

I added the note on ManifestEntryV1::snapshot_id, with a link to #3371. main rejects these manifests too, and #3371 has a reproduction against main. Reading the field as Option<i64> changes which manifests iceberg-rust accepts, so I'd like to decide that in #3371 rather than in this PR.

[laskoviymishka]

Optional follow-on, not blocking: these writer-schema variants all write long/double matching the reader, so the one path still untested is numeric promotion. A sibling that writes int for a long field (or float for double) would pin the promotion the serde visitors now own — that's the piece the old resolving reader used to handle for us. Cheap to add right alongside these.

[mbutrovich]

A sibling that writes int for a long field (or float for double) would pin the promotion the serde visitors now own

test_parse_manifest_promotes_writer_types, just above this one, covers both. It writes int for the long partition field and for record_count and file_size_in_bytes, and float for the double partition field. test_resolved_reads_writer_values covers the same promotions in the adapter.

[mbutrovich]

Thanks @laskoviymishka. I pushed the log-ordering fix and the snapshot_id note, and replied in each thread.