Support range-based reads for deletion vectors by KaiqiJinWow · Pull Request #3478 · apache/iceberg-python

KaiqiJinWow · 2026-06-11T21:27:03Z

Summary

Builds on #3690, which enables reading V3 deletion-vector content-range fields from manifests.

Read deletion-vector blobs using the manifest-provided content_offset and content_size_in_bytes, without requiring the physical file to be a complete Puffin file.
Preserve the existing whole-Puffin read path when all content-reference fields are absent. Partial references go through validation.
Validate DV blob length, magic number, CRC, and cardinality.
Keep the existing set[DataFile] API and DataFile equality unchanged. Match DVs to their referenced data files before constructing task sets, and deduplicate across tasks internally by (file_path, content_offset, content_size_in_bytes).
Preserve DV reference fields during REST task conversion, deriving an omitted DV target from the task when a content range is present.

Testing

Automated

Tests for DV blob deserialization and validation, range-based reads, partial content references, and whole-Puffin fallback.
Checked-in externally generated .bin fixtures containing a single DV and six DVs sharing one physical file.
Packed-fixture tests cover local delete-file indexing and REST task conversion, full and subset scans, and duplicate references. They verify both the deleted positions and the exact surviving Parquet rows.

Manual Validation

Verified the expected deleted positions using an externally generated .bin DV file and its Iceberg V3 metadata.
Exercised metadata JSON and Avro manifest reading through StaticTable.scan() using the packed fixture with locally assembled V3 metadata and Parquet files. Full scans, filtered scans, and point lookups returned the expected results with both PyArrowFileIO and FsspecFileIO, including to_arrow(), count(), and batch reads. This additional check is not part of CI.

amogh-jahagirdar

Thanks @KaiqiJinWow, main comment is that I think we should introduce a new deletion_vector module which exposes a read_deletion_vector API and hides all the I/O, deserialization, validation. Looks like currently that's all kinda spread out over different classes.

Also just for transparency on what's driving this change to others, currently Databricks Runtime produces deletion vectors that are Iceberg spec compliant DV blobs but they are not neccessarily written in literal Puffin files (they're written in .bin files as a single blob) . The current PyIceberg implementation has strict checks that the DVs must be in literal Puffin files but that's not strictly neccessary. As long as the blob is spec compliant I think there's a reasonable argument that we can consume them regardless of what kind of literal file the blob is stored in. For context, the Java implementation also just works off a similar principle of just reading a spec compliant blob from a range.

rambleraptor · 2026-07-06T18:10:23Z

I was asked for a review on this PR. @KaiqiJinWow is this WIP or is it ready for review? If you can get the integration test passing, I'd love to take a look.

rambleraptor

I've got some questions around APIs mostly.

KaiqiJinWow · 2026-07-09T03:35:31Z

Hi @rambleraptor @amogh-jahagirdar @ebyhr, thanks for your reviews! I updated this PR to address the review feedback. The latest revision keeps the content-range DV path strict, preserves whole-Puffin reads, and cleans up the deletion_vector API surface.

Could you take another look when you get a chance? Thanks!

amogh-jahagirdar · 2026-07-15T22:24:28Z

+    if has_deletion_vector_content_reference(data_file):
+        return [_read_deletion_vector(io, data_file)]
+
+    with io.new_input(data_file.file_path).open() as fi:
+        return deletion_vectors_from_puffin_file(PuffinFile(fi.read()))


I don't think we need both branches. In both cases we are reading a single DV in a given byte range. Whether it's in a puffin or not should be inconsequential.

See #3478 (review) for more details.

amogh-jahagirdar · 2026-07-15T22:31:10Z

+    if content_offset is None:
+        raise ValueError(f"Invalid deletion vector, content offset is missing: {data_file.file_path}")
+    if content_size_in_bytes is None:
+        raise ValueError(f"Invalid deletion vector, content size is missing: {data_file.file_path}")
+    if content_offset < 0:
+        raise ValueError(f"Invalid deletion vector, content offset cannot be negative: {content_offset}")


I am fine with having a more defensive implementation (the spec requires writers to produce the offset/size/refereenced file for DVs anyways) but just mentioning i think we only need to do these checks once and in one place only rather than in multiple places.

amogh-jahagirdar · 2026-07-15T22:34:19Z

+        if cardinality != record_count:
+            raise ValueError(f"Invalid cardinality: {cardinality}, expected {record_count}")


I think this is fine, again as we expect these two values to be the same but just remember implementations can choose to be a bit more relaxed (or vice versa more strict) than the actual spec. Is it worth failing the read of the DV if there's a mismatch? On one hand it indicates something incorrect in the metadata, on the other hand, we could be blocking a read of the data unnecessarily (because it wouldn't affect correctness of the result anyways). So in this case I'd probably bias to the latter of not doing this check. But I'll leave it up to you cc @kevinjqliu @rambleraptor in case you folks have opinions here.

Java does the check, so we should probably keep it just to match the implementations. This is the kind of thing that I imagine iceberg-verification will be checking at some point and we don't want to have to add the check back in to help keep that repository green.

That being said, I always bias towards removing checks on user data, since we can't always assume the writer did a valid job. It's such a waste to not read a valid DV because of a mismatch.

amogh-jahagirdar

iceberg-python/pyiceberg/manifest.py

Line 557 in 2c75523

def __eq__(self, other: Any) -> bool:

@KaiqiJinWow I think we need to double check the equals implementation. The code prior to this change only uses path to dedupe even for delete files/DVs, which are collected into a set. This used to work for the case where multiple DVs exist in a Puffin prior to this change just based off luck because we would read the whole puffin file in the end anyways. But in a world where we just do the range based reads which are more generic we cannot rely on this because we're not reading the whole puffin

kevinjqliu · 2026-07-27T04:23:43Z

The DeleteFileSet approach makes sense, but I suggest making its identity explicit with a DeleteFileKey rather than using an anonymous tuple.

@dataclass(frozen=True, slots=True)
class DeleteFileKey:
    file_path: str
    content_offset: int | None
    content_size_in_bytes: int | None

    @classmethod
    def from_file(cls, delete_file: DataFile) -> "DeleteFileKey":
        return cls(
            file_path=delete_file.file_path,
            content_offset=delete_file.content_offset,
            content_size_in_bytes=delete_file.content_size_in_bytes,
        )

DeleteFileSet could then be backed by:

self._files: dict[DeleteFileKey, DataFile]

For example:

def add(self, delete_file: DataFile) -> None:
    key = DeleteFileKey.from_file(delete_file)
    self._files.setdefault(key, delete_file)

def discard(self, delete_file: DataFile) -> None:
    key = DeleteFileKey.from_file(delete_file)
    self._files.pop(key, None)

This makes the intended identity clearer:

A traditional delete file is identified by DeleteFileKey(file_path, None, None).
A DV is identified by its physical range: DeleteFileKey(file_path, offset, size).
Multiple DVs can share the same Puffin or binary file without being incorrectly deduplicated.
Adding the same DV range more than once still behaves like a normal set.

Using a named, immutable key also avoids relying on tuple ordering and keeps this specialized identity separate from the existing path-based DataFile.__eq__ behavior.

github-actions · 2026-08-27T01:35:21Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that's incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

KaiqiJinWow · 2026-09-08T18:52:23Z

Hi @amogh-jahagirdar @rambleraptor @ebyhr @kevinjqliu, #3690 is now merged, and this PR is rebased with CI green. I’ve addressed the previous feedback, including the range aware DeleteFileSet identity and centralized DV range reading and validation, while preserving the whole Puffin fallback.

Could you please take another look? Thanks!

rambleraptor

Can you add an integration test for this with Spark? I'd really love to see that we can successfully read a deletion vector written by an outside source.

Even copying in a fixture made by somebody else would be great.

rambleraptor · 2026-09-10T20:48:01Z

    ) -> None:
        self.file = data_file
-        self.delete_files = delete_files or set()
+        self.delete_files = DeleteFileSet(delete_files if delete_files is not None else [])


Doesn't look like you need the default, since DeleteFileSet already sets a default.

rambleraptor · 2026-09-10T20:56:19Z

+        if cardinality != record_count:
+            raise ValueError(f"Invalid cardinality: {cardinality}, expected {record_count}")


Java does the check, so we should probably keep it just to match the implementations. This is the kind of thing that I imagine iceberg-verification will be checking at some point and we don't want to have to add the check back in to help keep that repository green.

That being said, I always bias towards removing checks on user data, since we can't always assume the writer did a valid job. It's such a waste to not read a valid DV because of a mismatch.

KaiqiJinWow · 2026-09-30T16:59:43Z

Can you add an integration test for this with Spark? I'd really love to see that we can successfully read a deletion vector written by an outside source.

Even copying in a fixture made by somebody else would be great.

Thanks @rambleraptor! I added two externally generated .bin fixtures in e71c8d1: one containing a single DV and another containing six DVs sharing the same physical file.

The tests verify the expected deleted row positions using the manifest-provided content ranges. The packed fixture test also checks that all six DVs survive deduplication and are associated with the correct data files.

Could you take another look when you have a chance? Thanks!

Fokko

Left some comments, but this looks great to me 👍

Fokko · 2026-10-07T07:12:35Z

+        )
+
+
+class DeleteFileSet(MutableSet[DataFile]):


I'm wondering if we could make it extend Set, rather than MutableSet. We don't used discard and update does an add operation. I think having this as immutable, that it makes it easier to reason about the code and flow.

+1

I don't love the idea of us creating our own set class if possible (that may have unexpected semantics under the hood)

Could we make this a frozenset?

Fokko · 2026-10-07T07:15:38Z

+
+def has_deletion_vector_content_reference(dv: "DataFile") -> bool:
+    """Return whether a deletion vector is described by manifest content-range metadata."""
+    return dv.content_offset is not None or dv.content_size_in_bytes is not None or dv.referenced_data_file is not None


I think these should be and, rather than or, since we require them all downstream in _read_deletion_vector

Hi Fokko, the or is intentional: if any content-reference field is present, we take the range-read path and validate that all required fields are set later. The whole-Puffin fallback is only used when all three fields are absent. Changing this to and would let partial metadata bypass validation. I’ll clarify the docstring and add a test for that case.

Fokko · 2026-10-07T07:17:01Z

+        other_keys: set[DeleteFileKey] = set()
+        other_count = 0
+        for delete_file in other:
+            if not isinstance(delete_file, DataFile):
+                return False
+            other_keys.add(DeleteFileKey.from_file(delete_file))
+            other_count += 1
+
+        return len(other_keys) == other_count and set(self._files) == other_keys


This part feels odd to me, why do we want to compare this to an iterable?

Fokko · 2026-10-07T07:25:10Z

+from pyiceberg.manifest import DataFile
+
+
+@dataclass(frozen=True, slots=True)


Love the slots=True. We define slots by hand throughout the codebase, but that's not needed anymore with Python 3.10

Created an issue for it: #4086

rambleraptor

Broadly, this looks great. Most of my questions are around the DeleteFileSet API

rambleraptor · 2026-10-07T21:23:19Z


    file: DataFile
-    delete_files: set[DataFile]
+    delete_files: DeleteFileSet


This is going to change the API surface, since DeleteFileSet is a MutableSet, not a regular set.

Sets have a whole mess of built-in methods and we don't want to replicate all of them.

rambleraptor · 2026-10-07T21:23:45Z

+        )
+
+
+class DeleteFileSet(MutableSet[DataFile]):


Could we make this a frozenset?

Restore native scan task sets by routing deletion vectors to their referenced data files. Preserve REST range references and cover local and REST scans of packed DV fixtures.

KaiqiJinWow · 2026-10-08T22:50:15Z

Thanks @rambleraptor @Fokko @amogh-jahagirdar @kevinjqliu for the feedback! I’ve pushed 5c05f03.

In this commit, I removed DeleteFileSet and kept the existing set API and DataFile equality. Each DV is now matched to its data file during planning. When loading deletes across tasks, we deduplicate by path, offset, and size, so different DV ranges in the same file are kept.

I also fixed REST planning to preserve the DV metadata and expanded the packed .bin tests to check the final scan results.

Could you take another look at the matching and deduplication changes? Thanks!

KaiqiJinWow changed the title ~~Support range-based reads for deletion vectors~~ [WIP]Support range-based reads for deletion vectors Jun 11, 2026

KaiqiJinWow force-pushed the fix-dv-content-range-read branch 3 times, most recently from 859efdc to 118c561 Compare June 11, 2026 23:07

ebyhr reviewed Jun 12, 2026

View reviewed changes

Comment thread pyiceberg/io/pyarrow.py Outdated

amogh-jahagirdar reviewed Jun 14, 2026

View reviewed changes

Comment thread pyiceberg/table/puffin.py Outdated

Comment thread pyiceberg/io/pyarrow.py Outdated

Comment thread pyiceberg/io/pyarrow.py Outdated

Comment thread pyiceberg/io/pyarrow.py Outdated

amogh-jahagirdar requested review from kevinjqliu and rambleraptor June 14, 2026 03:38

KaiqiJinWow force-pushed the fix-dv-content-range-read branch 3 times, most recently from 46bdf9f to b7c2ef4 Compare July 6, 2026 17:40

KaiqiJinWow changed the title ~~[WIP]Support range-based reads for deletion vectors~~ Support range-based reads for deletion vectors Jul 6, 2026

KaiqiJinWow requested a review from amogh-jahagirdar July 6, 2026 18:18

KaiqiJinWow force-pushed the fix-dv-content-range-read branch from b7c2ef4 to fa81f14 Compare July 6, 2026 22:14

rambleraptor reviewed Jul 6, 2026

View reviewed changes

Comment thread pyiceberg/table/deletion_vector.py Outdated

Comment thread pyiceberg/table/deletion_vector.py Outdated

Comment thread pyiceberg/table/deletion_vector.py Outdated

KaiqiJinWow force-pushed the fix-dv-content-range-read branch 2 times, most recently from d92432d to 4786782 Compare July 8, 2026 22:52

KaiqiJinWow requested review from ebyhr and rambleraptor July 10, 2026 23:39

amogh-jahagirdar reviewed Jul 15, 2026

View reviewed changes

amogh-jahagirdar requested changes Jul 15, 2026

View reviewed changes

KaiqiJinWow force-pushed the fix-dv-content-range-read branch 4 times, most recently from f4b6a20 to 7de9394 Compare July 21, 2026 22:16

KaiqiJinWow mentioned this pull request Jul 21, 2026

Read manifests with V3 projection #3690

Merged

KaiqiJinWow requested a review from amogh-jahagirdar July 24, 2026 16:25

github-actions Bot added the stale label Aug 27, 2026

KaiqiJinWow force-pushed the fix-dv-content-range-read branch from 7de9394 to d71d94c Compare August 27, 2026 17:41

github-actions Bot removed the stale label Aug 28, 2026

Support range-based reads for deletion vectors

0e99544

KaiqiJinWow force-pushed the fix-dv-content-range-read branch from d71d94c to 0e99544 Compare September 1, 2026 17:35

rambleraptor reviewed Sep 10, 2026

View reviewed changes

jhamman mentioned this pull request Sep 27, 2026

V3 support earth-mover/iceberg-python#1

Open

Test external single and packed deletion vector fixtures

e71c8d1

KaiqiJinWow requested a review from rambleraptor September 30, 2026 16:56

anoopj mentioned this pull request Oct 7, 2026

Read deletion vectors by content offset without parsing the Puffin footer #4085

Draft

Fokko reviewed Oct 7, 2026

View reviewed changes

Make delete file sets read-only and clarify DV validation

e8d6fe9

rambleraptor reviewed Oct 7, 2026

View reviewed changes

Keep DV range identity internal to scan aggregation

5c05f03

Restore native scan task sets by routing deletion vectors to their referenced data files. Preserve REST range references and cover local and REST scans of packed DV fixtures.

KaiqiJinWow requested review from Fokko and rambleraptor October 8, 2026 22:49

		if cardinality != record_count:
		raise ValueError(f"Invalid cardinality: {cardinality}, expected {record_count}")

		from pyiceberg.manifest import DataFile


		@dataclass(frozen=True, slots=True)

Conversation

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

Uh oh!

Summary

Testing

Automated

Manual Validation

Uh oh!

Uh oh!

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rambleraptor commented Jul 6, 2026

Uh oh!

rambleraptor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KaiqiJinWow commented Jul 9, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amogh-jahagirdar Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

Uh oh!

kevinjqliu commented Jul 27, 2026

Uh oh!

github-actions Bot commented Aug 27, 2026

Uh oh!

KaiqiJinWow commented Sep 8, 2026

Uh oh!

rambleraptor 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!

KaiqiJinWow commented Sep 30, 2026

Uh oh!

Fokko 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!

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!

rambleraptor left a comment

KaiqiJinWow commented Jun 11, 2026 •

edited

Loading

amogh-jahagirdar Jul 15, 2026 •

edited

Loading