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

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

Summary

Expose Iceberg V3 deletion vector content range fields on DataFile
Read Puffin deletion vectors from manifest-described content ranges when content_offset/content_size_in_bytes are present
Validate deletion vector blobs for length, magic number, CRC, and cardinality while preserving existing whole-file Puffin reads

Testing

.venv/bin/python -m pytest -q tests/table/test_puffin.py tests/io/test_pyarrow.py::test_read_deletion_vector_blob_from_content_range tests/io/test_pyarrow.py::test_read_deletes

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.

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

# Conflicts: # pyiceberg/table/deletion_vector.py

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

Support range-based reads for deletion vectors

5b6d113

# Conflicts: # pyiceberg/table/deletion_vector.py

KaiqiJinWow force-pushed the fix-dv-content-range-read branch from 4786782 to 5b6d113 Compare July 16, 2026 05:47

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

Uh oh!

Conversation

KaiqiJinWow commented Jun 11, 2026

Summary

Testing

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!

amogh-jahagirdar Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amogh-jahagirdar Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

amogh-jahagirdar Jul 15, 2026 •

edited

Loading