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 (with @glesperance's Spark interop test), 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).
A Spark interoperability test confirming PyIceberg can read Spark-written Puffin DVs (tests/integration/test_puffin_spark_interop.py, by @glesperance).

Are there any user-facing changes?

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

Verify pyiceberg's PuffinFile reader can parse deletion vectors written by Spark. Uses coalesce(1) to force Spark to create DVs instead of COW.

@ebyhr

PuffinFile reads only the serialized vector, skipping a blob's length prefix, deletion-vector magic and CRC-32, so the round-trip tests never exercise that framing. Add coverage for review items agreed on the original PR (apache#2822) that were not yet asserted by any test: - Assert the blob `fields` is [2147483645] (Java MetadataColumns.ROW_POSITION, INT_MAX - 2), required for Java/Spark interoperability (raised by @ebyhr). - Assert the deletion-vector blob framing at the byte level: the length prefix, the deletion-vector magic, and the CRC-32 over magic + vector.

ebyhr · 2026-06-09T23:36:03Z

+        self._blobs = []
+        self._blob_payloads = []
+
+        # 1. Create bitmaps from positions


nit: I would avoid using number prefixes. When we want to add a new operation, we need to adjust the subsequent numbers.

Removed the numbered prefixes in 4ecfd18.

ebyhr · 2026-06-09T23:39:30Z

+        # Calculate the cardinality from the bitmaps
+        cardinality = sum(len(bm) for bm in bitmaps.values())


nit: A comment for a simple single line seems excessive. It's evident when we read the code.

Removed in 4ecfd18.

ebyhr · 2026-06-10T00:00:21Z

+@pytest.mark.integration
+def test_read_spark_written_puffin_dv(spark: SparkSession, session_catalog: RestCatalog) -> None:
+    """Verify pyiceberg can read Puffin DVs written by Spark."""
+    identifier = "default.spark_puffin_format_test"


This PR introduces support for write operations, so we're interested in verifying that Spark can read Puffin files written by PyIceberg. There are no requested changes for now. I suppose this PR is a preparatory change, and we'll need another PR to use it during the write operations.

Exactly right, this PR is preparatory. PyIceberg does not yet have a write path that commits DVs as delete files, so a Spark-reads-PyIceberg interop test is not possible in isolation. As follow-ups, I plan to (1) extend PuffinWriter to support one blob per referenced data file and expose per-blob offset/length for content_offset/content_size_in_bytes, and (2) wire it into the merge-on-read branch of Transaction.delete() for v3 tables (toward #1078), where the Spark-reads-PyIceberg interop test will live.

ebyhr · 2026-06-10T00:20:36Z

+class PuffinWriter:
+    _blobs: list[PuffinBlobMetadata]
+    _blob_payloads: list[bytes]
+    _created_by: str | None


Could you please set the default value for the _created_by field using PyIceberg version {version}? You can obtain the version by using importlib.metadata.version.

Good idea. Done in 4ecfd18, the default is now PyIceberg version {importlib.metadata.version("pyiceberg")}.

ebyhr · 2026-06-10T00:24:52Z

@@ -0,0 +1,93 @@
+# Licensed to the Apache Software Foundation (ASF) under one


This test passes without the changes made in this PR. Could you please extract a PR that adding this test?

Extracted to #3476 and removed from this PR in 4ecfd18.

- Default created-by footer property to 'PyIceberg version {version}' - Move the Spark interop reader test to a separate PR - Remove numbered and self-evident comments - Name the row position field id constant - Validate positions in set_blob (non-negative, non-empty) - Simplify blob framing and finish() assembly

ebyhr · 2026-06-10T04:20:54Z

+
+
+class PuffinWriter:
+    """Writes a Puffin file containing a single deletion-vector-v1 blob."""


This comment looks misleading. This writer doesn't write a file in my understanding.

You are right, it didn't. Addressed in eb81422 together with the suggestion below: PuffinWriter now accepts an OutputFile and finish() writes the file, so the docstring matches the behavior now.

ebyhr · 2026-06-10T04:22:38Z

+    _blob_payloads: list[bytes]
+    _created_by: str
+
+    def __init__(self, created_by: str | None = None) -> None:


What about accepting an OutputFile or something, and writing the content to it? I think this is a better approach than returning bytes. Iceberg Java PuffinWriter also accepts an output file object.

Good idea, done in eb81422. PuffinWriter now takes an OutputFile and finish() writes the content to it and returns the file size, following the Java PuffinWriter shape. One simplification compared to Java: the file is assembled in memory and written in one shot rather than streamed, which should be fine for DVs since they are small. Happy to revisit with a streaming implementation if needed.

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-22T17:12:34Z

+        self._blobs = []
+        self._blob_payloads = []
+        self._created_by = (
+            created_by if created_by is not None else f"PyIceberg version {importlib.metadata.version('pyiceberg')}"


small nit: pyiceberg already exposes __version__ in pyiceberg/__init__.py, so we could do from pyiceberg import __version__ here instead of importlib.metadata.version('pyiceberg') like we do in cli/console.py. Besides matching the existing pattern, importlib.metadata.version raises PackageNotFoundError when pyiceberg isn't pip-installed, which would crash the writer over the cosmetic created-by field.

Thanks, that makes sense — switched to from pyiceberg import __version__ to match the existing pattern in cli/console.py and to avoid the PackageNotFoundError from importlib.metadata.version when pyiceberg isn't pip-installed. Updated the test accordingly.

Still in place after the rework onto #3491 — created-by defaults to PyIceberg version {__version__} via from pyiceberg import __version__.

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?

Switch from importlib.metadata.version('pyiceberg') to the exported __version__ to match the existing pattern in cli/console.py and avoid PackageNotFoundError when pyiceberg is not pip-installed. Co-authored-by: Isaac

Co-authored-by: Isaac

…-revival # Conflicts: # pyiceberg/table/puffin.py # tests/table/test_deletion_vector.py

…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

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.

rambleraptor and others added 7 commits June 9, 2026 14:57

deletion vector write

755793c

test fix

9b10a4f

lint fixes

c90ad38

test: Add Spark interop test for Puffin DV reader

842d6a5

Verify pyiceberg's PuffinFile reader can parse deletion vectors written by Spark. Uses coalesce(1) to force Spark to create DVs instead of COW.

PR comments

9524618

lint

e23a67d

moomindani mentioned this pull request Jun 9, 2026

Write Deletion Vectors #2822

Closed

ebyhr reviewed Jun 10, 2026

View reviewed changes

moomindani mentioned this pull request Jun 10, 2026

Add Spark interop test for reading Puffin deletion vectors #3476

Open

moomindani added 2 commits June 10, 2026 17:52

Accept an OutputFile in PuffinWriter and write the file in finish()

eb81422

Add unit tests for sparse bitmap keys and the Java key range limit

a6d2f31

moomindani mentioned this pull request Jun 10, 2026

[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 added 2 commits June 23, 2026 09:13

Use pyiceberg.__version__ for PuffinWriter created-by default

a979e55

Switch from importlib.metadata.version('pyiceberg') to the exported __version__ to match the existing pattern in cli/console.py and avoid PackageNotFoundError when pyiceberg is not pip-installed. Co-authored-by: Isaac

Apply ruff format to PuffinWriter created-by line

be344a6

Co-authored-by: Isaac

moomindani mentioned this pull request Jun 23, 2026

Extract DeletionVector logic from PuffinFile #3491

Merged

moomindani added 2 commits June 25, 2026 23:19

Merge remote-tracking branch 'upstream/main' into moomindani/dv-write…

16ce42c

…-revival # Conflicts: # pyiceberg/table/puffin.py # tests/table/test_deletion_vector.py

		# Calculate the cardinality from the bitmaps
		cardinality = sum(len(bm) for bm in bitmaps.values())

		@@ -0,0 +1,93 @@
		# Licensed to the Apache Software Foundation (ASF) under one



		class PuffinWriter:
		"""Writes a Puffin file containing a single deletion-vector-v1 blob."""

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


		class PuffinWriter:

Uh oh!

Conversation

moomindani commented Jun 9, 2026

Rationale for this change

Are these changes tested?

Are there any user-facing changes?

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!

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!

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!

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!

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!

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!

moomindani commented Jun 25, 2026

Reworked onto #3491

Notable changes in context

How prior review feedback is addressed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants