fix(table/dv): read deletion vectors by content_offset when the file is not a Puffin container - #2071
Open
sevbanbayrak wants to merge 1 commit into
Open
sevbanbayrak wants to merge 1 commit into
sevbanbayrak wants to merge 1 commit into
Conversation
…is not a Puffin container Databricks writes deletion vectors for IcebergCompatV3 (UniForm) tables as a Delta deletion_vector_*.bin file: a one-byte version prefix followed by deletion-vector-v1 blobs, with no Puffin header or footer. The manifest entry is well-formed (file_format=PUFFIN, referenced_data_file, content_offset, content_size_in_bytes point at a valid blob), and the Java reference reader (BaseDeleteLoader.readDV) reads such files because it never consults the Puffin footer: it reads content_size_in_bytes bytes at content_offset. ReadDV/ReadDVs opened the file with puffin.NewReader first and failed with "puffin: invalid header magic" on these tables. - puffin: NewReader wraps the header-magic and too-small errors in a new sentinel, ErrNotPuffinFile, so callers can detect "no Puffin container". - table/dv: when the DV file is not a Puffin container, fall back to reading the blobs directly at content_offset (Java parity). The blob's length, magic and CRC-32 are still verified by DeserializeDV and the decoded cardinality is validated against the manifest record_count; footer-only checks (blob type, referenced-data-file property, cardinality property) are skipped with a slog warning. Real Puffin files keep the existing strict path unchanged. Verified against a Databricks Unity Catalog IcebergCompatV3 table read through the UC Iceberg REST catalog with vended credentials: 968,980 rows after 1,020 deletes + 1,020 updates, matching the SQL result; copy-on-write and non-DV tables unaffected. Fixes apache#2070 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MqCR5xLuYcb7Baw6Xbj4be
This branch has not been deployed
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.
Fixes #2070
Problem
Databricks writes deletion vectors for
IcebergCompatV3(UniForm) tables as a Deltadeletion_vector_<uuid>.binfile: a one-byte version prefix followed bydeletion-vector-v1blobs, with no Puffin header or footer. The manifest entry is well-formed (file_format=PUFFIN,referenced_data_file,content_offset,content_size_in_bytespoint at a valid blob; length, magicD1D33964and CRC-32 check out).ReadDV/ReadDVsopened the file withpuffin.NewReaderand failed withpuffin: invalid header magic.The Java reference reader (
BaseDeleteLoader.readDV) reads these tables because it never consults the Puffin footer: it readscontent_size_in_bytesbytes atcontent_offsetand deserializes the blob.Change
puffin:NewReaderwraps the header-magic and too-small errors in a new sentinelErrNotPuffinFileso callers can detect "no Puffin container" witherrors.Is. Error messages keep their existing text.table/dv: when the DV file is not a Puffin container,ReadDV/ReadDVsfall back to reading the blobs directly atcontent_offset(readBareDVs), matching Java.DeserializeDVstill verifies the blob's length, magic and CRC-32, and the decoded cardinality is validated against the manifestrecord_count. The footer-only checks (blob type,referenced-data-fileproperty, cardinality property) cannot be performed and are skipped with aslogwarning. Real Puffin files keep the existing strict path unchanged.TestReadDVBareBlobWithoutPuffinContainer(single blob, two blobs in one bare file, cardinality mismatch, range beyond file);TestReadDVInvalidPuffinnow asserts the direct-read error.Verification
go test ./puffin/ ./table/dv/,go vet,make lint(0 issues).IcebergCompatV3table (Iceberg format-version 3) read through the UC Iceberg REST catalog with vended credentials, after a 1,020-rowDELETEand a 1,020-rowUPDATEproduced one DV (total-delete-files=1,total-position-deletes=2040):Scan().ToArrowRecordsreturns 968,980 rows with 119,960 / 1,020 rows carrying the updated markers, identical to the SQL result. Copy-on-write and non-DV tables on the same catalog are unaffected.