perf: parse manifest list entries without buffering Avro values - #3224
Open
kacper-li-airspace-intelligence wants to merge 1 commit into
Open
kacper-li-airspace-intelligence wants to merge 1 commit into
kacper-li-airspace-intelligence wants to merge 1 commit into
Conversation
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.
What changes are included in this PR?
ManifestList::parse_with_versioncollects the entire Avro reader into aValue::Arraybefore deserializing it. Each manifest stays in memory as aValue::Record, with owned field names, strings and nested values.from_valuethen borrows that array while allocating the typed records, so both representations coexist.This becomes expensive when a snapshot contains many manifests. For example,
FastAppendAction::existing_manifestloads the current manifest list on each append, even if the new append is small. The temporary allocation cost grows with the existing list, not the size of the append.Convert each Avro record into its final
ManifestFileas it is read, then drop the generic value before reading the next record. V1 keeps its reader-schema projection; V2/V3 keep the writer schema, field aliases and defaults. The input bytes and final typed list still occupy memory. Parsing still returns an error without a partial list, although a conversion error can now precede a later Avro decoding error.Remove the unused whole-list conversion methods and keep the serialization wrappers used by the existing tests behind
cfg(test).Are these changes tested?
The added
manifest_list_memorytest generates 10,000 distinct manifest paths with one partition summary per entry. It checks every parsed entry and measures peak live allocations with a test-only wrapper aroundSystem. On aarch64 macOS, Rust 1.96.0, release mode:9db0b685The 16 MiB regression budget fails on main and passes with the patch. Input generation is excluded from the measurement. Snappy and Zstandard cases also pass; the test additionally checks V1/V2/V3 round trips, empty lists, V1 defaults when read as V2, and rejection of truncated input.
cargo +stable test -p iceberg --release --test manifest_list_memory --locked -- --nocaptureTo reproduce the baseline, keep the added test and restore
crates/iceberg/src/spec/manifest_list/{mod.rs,_serde.rs}from9db0b685, then run the same command.All 1,691 core unit tests pass, including the existing schema-projection, field-alias and encrypted manifest-list tests. Clippy and formatting pass with the repository's pinned nightly toolchain.
AI Disclosure
AI assisted with the implementation, test and description.