Write deletion vectors as valid Puffin files (add file magic + footer)#1094
Closed
raghav-reglobe wants to merge 1 commit into
Closed
Write deletion vectors as valid Puffin files (add file magic + footer)#1094raghav-reglobe wants to merge 1 commit into
raghav-reglobe wants to merge 1 commit into
Conversation
WriteDeletionVectorFile wrote only the deletion-vector-v1 blob to the file and labelled it file_format=puffin, with content_offset=0 and file_size == blob size. That is not a valid Puffin file: the Puffin spec requires the layout `Magic Blob... Footer`, with the 4-byte magic (0x50 0x46 0x41 0x31, "PFA1") at the start and a Footer (Magic, FooterPayload JSON, FooterPayloadSize, Flags, Magic) at the end. The bare blob only works because every reader locates the vector via the manifest's content_offset/content_size_in_bytes and never parses the Puffin container; a spec-compliant Puffin reader (or any tool that enumerates blobs via the footer) cannot read these files. Add IcebergDeletionVectorData::ToPuffinFile, which wraps the blob in a proper container: leading magic + blob (at offset 4) + footer whose FileMetadata describes the single deletion-vector-v1 blob. Per the spec, snapshot-id and sequence-number are -1, and the blob carries the referenced-data-file and cardinality properties; the blob is uncompressed. content_offset is now 4. Reads are unchanged: they remain driven by the manifest's content_offset, so existing DV files written with content_offset=0 keep reading correctly (no migration). Footer framing (magic, little-endian FooterPayloadSize, flags) verified byte-identical to an Iceberg-Java-generated Puffin file; the blob is unchanged. Adds a test asserting a written deletion-vector file begins and ends with the Puffin magic. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
IcebergDelete::WriteDeletionVectorFilewrites a deletion vector by serializing thedeletion-vector-v1blob and writing only that blob to the file, while labelling itfile_format = puffinwithcontent_offset = 0andfile_size_bytes == blob size.That is not a valid Puffin file. The Puffin spec
requires the layout
Magic Blob₁ … Blobₙ Footer, with the 4-byte magic0x50 0x46 0x41 0x31(PFA1) at the start of the file and a Footer(
Magic,FooterPayloadJSON,FooterPayloadSize,Flags,Magic) at the end.The file produced today has neither the leading magic nor the footer — it is a bare blob.
It works in practice only because every reader (duckdb-iceberg included) locates the
vector via the manifest's
content_offset/content_size_in_bytesand never parses thePuffin container. A spec-compliant Puffin reader — or any tool that enumerates blobs via
the footer, validates the file standalone, or lacks the manifest offsets — cannot read
these files.
Fix
Add
IcebergDeletionVectorData::ToPuffinFile, which wraps the blob in a proper container:The
FooterPayloadis aFileMetadatadescribing the singledeletion-vector-v1blob.Per the spec, for a deletion vector
snapshot-idandsequence-numberare-1, the blobcarries the required
referenced-data-fileandcardinalityproperties, and it isuncompressed (no
compression-codec, flags = 0).content_offsetis now4(the blob begins right after the leading magic). The read pathis unchanged — it is driven by the manifest's
content_offset, so existing deletion-vectorfiles written with
content_offset = 0keep reading correctly. No migration needed.Validation
FooterPayloadSize, flags)is byte-identical to an Iceberg-Java-generated Puffin file
(
empty-puffin-uncompressed.bin):PFA1 | … | <size LE> | 0x00000000 | PFA1. The blobmetadata field order matches Iceberg-Java
(
type, fields, snapshot-id, sequence-number, offset, length, properties).deletion-vector-v1blob is unchanged (ToBlob), so it remains byte-identical tothe Iceberg reference vector.
Testing
test/.../delete/test_deletion_vector_is_valid_puffin.test: after a v3DELETE, resolvesthe deletion-vector file via
iceberg_metadata(content = 'POSITION_DELETES') and assertsthe file begins and ends with
PFA1— i.e. it is a valid Puffin container, not a bareblob. Skipped on Nessie (v3 fixture limitation, like the other v3 delete tests).