Add PuffinWriter for writing deletion vectors by moomindani · Pull Request #3474 · apache/iceberg-python

moomindani · 2026-06-09T23:07:54Z

Part of #2261. Continues #2822.

Rationale for this change

This adds a PuffinWriter for writing Puffin files containing deletion-vector-v1 blobs — the first building block for deletion-vector write support in PyIceberg (tracking issue #2261).

It revives #2822 by @rambleraptor, which was auto-closed by the stale bot rather than on merit. The original work — including all review feedback already addressed there (@ebyhr, @geruh) — is preserved commit-for-commit.

On top of that, this PR adds unit tests for two agreed review items that were not yet asserted by any test:

the blob fields value [2147483645] (Java MetadataColumns.ROW_POSITION, INT_MAX - 2), required for Java/Spark interoperability; and
the deletion-vector blob framing at the byte level (length prefix, DV magic, CRC-32 over magic + vector), which the PuffinFile reader skips, so the round-trip tests did not previously exercise it.

As in the original PR, this is intentionally scoped to the writer + tests so we can agree on the write semantics before wiring it into the delete/manifest writers and the merge-on-read path. Per the original review discussion, the writer expects the caller to provide one merged deletion vector per data file.

Are these changes tested?

Yes: unit tests for round-trip write/read, the single-blob (1:1) behavior, the DV field id, byte-level blob framing, and empty files (tests/table/test_puffin.py).

Are there any user-facing changes?

No. PuffinWriter is a new internal building block and is not yet wired into any public write path.

ebyhr · 2026-06-13T00:32:04Z

        return {path: _bitmaps_to_chunked_array(bitmaps) for path, bitmaps in self._deletion_vectors.items()}
+
+
+class PuffinWriter:


This name looks too generic. We could consider renaming it to DeletionVectorWriter or a similar name. I've opened #3491 to extract DV-specific logic from puffin.py.

+1 to @ebyhr's point

Once #3491 lands I think it would be worth rebasing this implementation on top of it. WDYT?

Sounds good to me. I'll wait for #3491 to land and then rebase this on top of it, renaming the writer to DeletionVectorWriter as part of that. Thanks both!

Following up here: after the discussion in #3491 we decided to keep PuffinWriter as a generic, format-level writer and put the DV serialization on DeletionVector (to_blob()), rather than renaming. I've reworked this PR onto #3491 accordingly — details in the summary comment above.

sungwy

Thanks for working on this @moomindani - I added some initial review comments

sungwy · 2026-06-22T19:45:35Z

        return {path: _bitmaps_to_chunked_array(bitmaps) for path, bitmaps in self._deletion_vectors.items()}
+
+
+class PuffinWriter:


+1 to @ebyhr's point

Once #3491 lands I think it would be worth rebasing this implementation on top of it. WDYT?

moomindani · 2026-06-25T14:28:36Z

Reworked onto #3491

Now that #3491 has merged, I've rebased this onto it (via a merge of main plus a rework commit, so the earlier history is preserved). The write path is now split along the same format/domain boundary #3491 established on the read side:

DeletionVector (domain) gains the write-side counterparts of the read path: from_positions(), _serialize_bitmap() (counterpart of _deserialize_bitmap()), and to_blob() (counterpart of to_vector()). It reuses MAX_JAVA_SIGNED / PROPERTY_REFERENCED_DATA_FILE from Extract DeletionVector logic from PuffinFile #3491.
PuffinWriter (format) stays in puffin.py as a generic, blob-agnostic writer that assembles a Puffin file from PuffinBlob payloads — mirroring PuffinFile on the read side and scaling to future blob types (e.g. apache-datasketches-theta-v1).

Notable changes in context

Naming: I'd earlier said I'd rename to DeletionVectorWriter, but per the follow-up discussion in Extract DeletionVector logic from PuffinFile #3491 we settled on keeping PuffinWriter generic and putting the DV serialization on DeletionVector instead. That's what's implemented here.
Interface: PuffinWriter is now a context manager (consistent with ManifestWriter); the file size is available via len(output_file) after the with block, replacing the earlier finish() -> int.
created-by: defaults to PyIceberg version {__version__} via from pyiceberg import __version__.

How prior review feedback is addressed

Feedback	Source	Status
`created_by` default = `PyIceberg version {version}`	@ebyhr	Done
Use `from pyiceberg import __version__` (not `importlib.metadata`)	@sungwy	Done
Accept an `OutputFile` and write to it (Java `PuffinWriter` shape)	@ebyhr	Done
Fix misleading docstring ("writer doesn't write a file")	@ebyhr	Done
Avoid numbered prefixes / excessive single-line comments	@ebyhr	Done
Extract the Spark interop test to its own PR	@ebyhr	Done (#3476)
DV serialization on `DeletionVector` (counterpart to `to_vector`)	#3491	Done (`to_blob`)
Keep a generic, format-level `PuffinWriter` in `puffin.py`	#3491	Done
Reuse `MAX_JAVA_SIGNED` / `PROPERTY_REFERENCED_DATA_FILE`	#3491	Done
Lock in DV field id and blob framing with tests	review	Done (`test_to_blob_payload_layout`)
Tests for sparse bitmap keys and the Java key-range limit	review	Done

Wiring this into the merge-on-read branch of Transaction.delete() for v3 tables (toward #1078), where the Spark-reads-PyIceberg interop test will live, remains a follow-up.

ebyhr · 2026-06-29T00:50:03Z

+
+            puffin_bytes = out.getvalue()
+
+        with self._output_file.create(overwrite=True) as output_stream:


Why is overwrite=True hardcoded here? For idempotency? If a caller accidentally passes an existing file path, this silently replaces it.

Yes — it's for idempotency. This matches the other write-once writers in PyIceberg: the data file writer (pyarrow.py) and the manifest writer (avro/file.py) both hardcode overwrite=True. Puffin DV files are written to fresh, UUID-based locations (like data/manifest files), so a collision with a different file isn't expected; the realistic case where the path already exists is a retry writing the same file, and overwrite=True keeps that idempotent instead of failing. Only ToOutputFile.table_metadata parameterizes overwrite, since metadata writes can legitimately target an existing path.

That said, if you can think of a case where silently overwriting would be a problem, I'm happy to make it a constructor argument.

ebyhr · 2026-06-30T04:13:00Z

Could you rebase on main to resolve conflicts?

…inWriter Following the design agreed in apache#3491, split the write path into: - DeletionVector serialization (domain): from_positions(), _serialize_bitmap() and to_blob() as the write-side counterparts of _deserialize_bitmap() and to_vector(), reusing MAX_JAVA_SIGNED / PROPERTY_REFERENCED_DATA_FILE. - A generic, blob-agnostic PuffinWriter (format) that assembles a Puffin file from PuffinBlob payloads, mirroring PuffinFile on the read side and scaling to future blob types (e.g. apache-datasketches-theta-v1). PuffinWriter is a context manager (consistent with ManifestWriter); the file size is available via len(output_file) after exit. The created-by default uses pyiceberg.__version__. Co-authored-by: Isaac

… error - DeletionVector._serialize_bitmap now run-optimizes each bitmap before serializing, matching Java's BitmapPositionDeleteIndex (a contiguous 100k-position DV drops from ~16 KB to ~25 bytes; still round-trips). - from_positions rejects positions above MAX_POSITION, mirroring Java's RoaringPositionBitmap.MAX_POSITION, to avoid an OOM in the gap-fill. - PuffinWriter.__exit__ short-circuits when the with-body raised, so no half-populated file is written. Co-authored-by: Isaac

Restructure test_puffin_writer_does_not_write_on_exception to use try/except instead of nesting an unconditional raise inside pytest.raises, which mypy's warn-unreachable flagged as unreachable. Co-authored-by: Isaac

moomindani · 2026-06-30T06:24:55Z

Done — rebased onto the latest main and resolved the conflicts. The branch now sits on top of main with the deletion-vector framing reconciled against the recent reader changes (_file_bytes absolute offsets and _extract_vector_payload).

moomindani · 2026-07-08T05:49:03Z

@ebyhr @sungwy This has been rebased onto the latest main and CI is green. Could you take another look when you have a chance?

rambleraptor · 2026-07-08T19:50:14Z

Can you change the PR description? We're not using the Spark test anymore (waiting until a later PR), so we shouldn't mention that the test exists for future PR readers.

rambleraptor · 2026-07-08T19:55:50Z

+        with io.BytesIO() as out:
+            out.write(len(non_empty).to_bytes(8, "little"))
+            for key, bitmap in non_empty:
+                if key > MAX_JAVA_SIGNED:


I think we've got an off-by-one error here.

Here's Java

Java fails if key > (2^31 - 1) - 1
We fail if key > (2^31 ) - 1

This is a weird edge case and definitely an issue in Java, but probably not worth fixing over there.

You're right, thanks — Java's readKey rejects key > Integer.MAX_VALUE - 1 while this check allowed one more key, so we could write a DV that Java cannot read. Fixed the bound to match Java. Since the key is the list index in _serialize_bitmap, the boundary isn't practically constructible in a test (it would need a 2^31-element list); the existing MAX_POSITION validation in from_positions keeps user input below the bound, so this check stays as defense in depth.

rambleraptor

Broadly this looks great. I saw a quick off-by-one error, but that's fine as-is. I do want to verify that we meant to get rid of the Spark test. I know that was a blocker in my original PR and I want to make sure that we get that test in at some point.

moomindani · 2026-07-09T00:20:33Z

Thanks for the review! Updated the description to remove the Spark interop test references.

Java's RoaringPositionBitmap.readKey rejects keys above Integer.MAX_VALUE - 1, while the serializer allowed one more key, so PyIceberg could write a deletion vector Java cannot read.

moomindani mentioned this pull request Jun 9, 2026

Write Deletion Vectors #2822

Closed

ebyhr reviewed Jun 10, 2026

View reviewed changes

Comment thread pyiceberg/table/puffin.py Outdated

Comment thread pyiceberg/table/puffin.py Outdated

Comment thread tests/integration/test_puffin_spark_interop.py Outdated

Comment thread pyiceberg/table/puffin.py Outdated

Comment thread tests/integration/test_puffin_spark_interop.py Outdated

ebyhr reviewed Jun 10, 2026

View reviewed changes

Comment thread pyiceberg/table/puffin.py Outdated

Comment thread pyiceberg/table/puffin.py Outdated

This was referenced Jun 10, 2026

Add Spark interop test for reading Puffin deletion vectors #3476

Open

[Feat] Support Merge-on-Read mode for Deletes #1078

Open

ebyhr reviewed Jun 21, 2026

View reviewed changes

sungwy reviewed Jun 22, 2026

View reviewed changes

moomindani mentioned this pull request Jun 23, 2026

Extract DeletionVector logic from PuffinFile #3491

Merged

sungwy reviewed Jun 26, 2026

View reviewed changes

Comment thread pyiceberg/table/puffin.py

Comment thread pyiceberg/table/deletion_vector.py

Comment thread pyiceberg/table/deletion_vector.py Outdated

ebyhr reviewed Jun 29, 2026

View reviewed changes

moomindani added 3 commits June 30, 2026 13:57

Fix mypy unreachable-statement error in PuffinWriter exception test

5fdd51f

Restructure test_puffin_writer_does_not_write_on_exception to use try/except instead of nesting an unconditional raise inside pytest.raises, which mypy's warn-unreachable flagged as unreachable. Co-authored-by: Isaac

moomindani force-pushed the moomindani/dv-write-revival branch from b26e7ed to 5fdd51f Compare June 30, 2026 05:03

This was referenced Jul 8, 2026

Update PyIceberg V3 lineage and column default values to partial Neuw84/iceberg-matrix#16

Merged

Support writing V3 tables #1551

Open

rambleraptor reviewed Jul 8, 2026

View reviewed changes

moomindani force-pushed the moomindani/dv-write-revival branch from a4b4c67 to 5fdd51f Compare July 9, 2026 00:01

Match Java's key bound when serializing deletion vector bitmaps

695c971

Java's RoaringPositionBitmap.readKey rejects keys above Integer.MAX_VALUE - 1, while the serializer allowed one more key, so PyIceberg could write a deletion vector Java cannot read.

		return {path: _bitmaps_to_chunked_array(bitmaps) for path, bitmaps in self._deletion_vectors.items()}


		class PuffinWriter:


		puffin_bytes = out.getvalue()

		with self._output_file.create(overwrite=True) as output_stream:

Uh oh!

Conversation

moomindani commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sungwy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moomindani commented Jun 25, 2026

Reworked onto #3491

Notable changes in context

How prior review feedback is addressed

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebyhr commented Jun 30, 2026

Uh oh!

moomindani commented Jun 30, 2026

Uh oh!

moomindani commented Jul 8, 2026

Uh oh!

rambleraptor commented Jul 8, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rambleraptor left a comment

Choose a reason for hiding this comment

Uh oh!

moomindani commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

moomindani commented Jun 9, 2026 •

edited

Loading

moomindani commented Jul 9, 2026 •

edited

Loading