Extract DeletionVector logic from PuffinFile

[ebyhr]

Rationale for this change

PuffinFile handles two tasks: format parsing (magic bytes, footer, blobs) and deletion vector domain logic (bitmap deserialization and PyArrow conversion).
This will become problematic when we introduce support for the NDV apache-datasketches-theta-v1 blob in the future.

Are these changes tested?

Yes

Are there any user-facing changes?

Yes - PuffinFile class user needs to call DeletionVector.

[sungwy]

Thanks for working on this PR @ebyhr and I agree this is a good cleanup.

I added some comments

[ebyhr]

Thanks for your review! Addressed comments.

[sungwy]

Thanks for this contribution @ebyhr . I've approved it, and I'll leave it open for a day for any additional reviews before we merge.

[moomindani]

Thanks for splitting this out — the read path looks behavior-preserving and the separation is clean.

For the write side, we'd like to coordinate before building on top of this: once it lands we're planning to rebase #3474 (the DV writer, currently PuffinWriter → to be renamed DeletionVectorWriter) on top of it, moving the writer along with its _serialize_bitmaps helper and the DV-specific constants into deletion_vector.py and reusing MAX_JAVA_SIGNED / PROPERTY_REFERENCED_DATA_FILE from here.

Given that, would it make sense to decide now whether the write/serialize path should live on DeletionVector itself (mirroring from_puffin_file / to_vector with a serialize counterpart) or as a separate DeletionVectorWriter class?

Our preference would be to mirror the read-side split — keep the DV bitmap serialization on DeletionVector (as the counterpart to from_puffin_file / to_vector) and leave the generic Puffin-file assembly as a format-level writer in puffin.py, so the write path scales to the future NDV blob the same way the read path now does. That said, we're happy either way.

[sungwy]

Hi @ebyhr - I thought about this a bit more and I think we'd benefit from revisiting the class design.

A single PuffinFile can hold multiple DVs, and that's read correctly here. But the DeletionVector class ends up encapsulating a map of deletion vectors (keyed by referenced-data-file), which I find a little counterintuitive given the name.

Would it make sense to have DeletionVector map 1-1 to a single DV blob instead? That'd line up with Java, where one DV = one data file's PositionDeleteIndex, loaded via the entry's content_offset/content_size. The from_puffin_file function could then be moved to be a standalone function and return a list of DeletionVectors. WDYT?

[ebyhr]

It makes sense to me. My initial motivation was to create a helper class similar to DVUtil in Iceberg Java.

I've updated this PR. Please let me know if the last change doesn't align with your intention.

[sungwy]

Thanks for splitting this out — the read path looks behavior-preserving and the separation is clean.

For the write side, we'd like to coordinate before building on top of this: once it lands we're planning to rebase #3474 (the DV writer, currently PuffinWriter → to be renamed DeletionVectorWriter) on top of it, moving the writer along with its _serialize_bitmaps helper and the DV-specific constants into deletion_vector.py and reusing MAX_JAVA_SIGNED / PROPERTY_REFERENCED_DATA_FILE from here.

Given that, would it make sense to decide now whether the write/serialize path should live on DeletionVector itself (mirroring from_puffin_file / to_vector with a serialize counterpart) or as a separate DeletionVectorWriter class?

Our preference would be to mirror the read-side split — keep the DV bitmap serialization on DeletionVector (as the counterpart to from_puffin_file / to_vector) and leave the generic Puffin-file assembly as a format-level writer in puffin.py, so the write path scales to the future NDV blob the same way the read path now does. That said, we're happy either way.

Hi @moomindani - thanks for flagging this, and for thinking about how the two paths line up. I'm leaning the same way you are: keep the DV bitmap serialization on DeletionVector as the counterpart to from_puffin_file / to_vector, and leave the generic Puffin assembly (agreed PuffinWriter sounds like a good name) as a format-level writer in puffin.py. That keeps the write path scaling to the future NDV blob the same way the read path now does. Happy to look at #3474 once this lands. Does that work for you?

[moomindani]

Works for me, thanks @sungwy. The 1-1 DeletionVector shape lines up nicely with the writer side too — set_blob maps to a single DV, so the serialize path can sit on DeletionVector as the counterpart to to_vector. I'll build on this once it lands.

[sungwy]

merged. Thanks a lot @ebyhr , and thanks @moomindani for the review!