perf(avro): buffer records into size-bounded blocks - #4008
Open
mbutrovich wants to merge 1 commit into
Open
mbutrovich wants to merge 1 commit into
mbutrovich wants to merge 1 commit into
Conversation
9 tasks
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.
Partially addresses #4007
Rationale for this change
ManifestWriter.add_entrycallsAvroOutputFile.write_blockonce per entry (manifest.py#L1213-L1238), andwrite_blockwrites exactly one Avro block for the list it is given. A manifest with 10,000 entries is therefore 10,000 blocks, each with its own deflate stream and 16-byte sync marker. Every stream starts cold, so compression barely applies, and every reader pays per-block setup to read a single record.This change buffers encoded records in
AvroOutputFileand flushes a block once the buffer reaches a sync interval. The default is 64,000 bytes, which is what Avro Java'sDataFileWriteruses (DataFileConstants.DEFAULT_SYNC_INTERVAL,4000 * SYNC_SIZE) and what Iceberg Java writes with, since it never callssetSyncInterval. The Iceberg spec says nothing about block sizes, so the value is a writer choice rather than a requirement.Measured on 10,000 entries with 12 columns and full column stats, reading through
ManifestFile.fetch_manifest_entry, best of 7 runs:The default was chosen by sweeping the interval on the same data with deflate, rather than by copying Java:
Both curves flatten at 64,000, and larger intervals read slightly slower. With the
nullcodec the file size is flat from 4,000 upward, so the size win comes from giving the compressor a larger window rather than from fewer sync markers.Are these changes tested?
Yes, in
tests/avro/test_file.pyandtests/utils/test_manifest.py.fetch_manifest_entryreturns all entries in order across block boundaries. Covered for V1 and V2, with bothnullanddeflate.write_manifest_listwith 2,000 manifests, which now spans multiple blocks because that writer passes its whole list in one call.ValueError.sync_interval=1.tell()grows after a record that is still buffered.Coverage of
pyiceberg/avro/file.pyis 97%, and every added line is executed. The uncovered lines are pre-existing.Are there any user-facing changes?
Manifests and manifest lists that PyIceberg writes now contain fewer, larger blocks. The file format does not change, so any Avro reader reads them, and files written by earlier versions still read the same way. Manifests get substantially smaller when a compression codec is set.
AvroOutputFilegains an optionalsync_intervalargument. No existing signature or behavior changes, and no table property is added, matching Java, which does not expose the setting.tell()now includes buffered bytes so that it stays monotonic per entry, which keepsManifestWriter.tellmeaningful for callers that use it to decide when to roll over to a new manifest.AI disclosure
Developed with the help of Claude Code, but I understand and support these changes.