Support for Databricks deletion vectors - #2271
Conversation
Databricks writes the spec-aligned deletion-vector-v1 envelope in a Delta container, so Iceberg reads failed on Puffin magic. Decode that slice from content_offset when the object is not a Puffin file. Co-authored-by: Cursor <cursoragent@cursor.com>
Spark Iceberg DELETE writes Puffin, so convert those objects to the Databricks UniForm layout before the ClickHouse read. Co-authored-by: Cursor <cursoragent@cursor.com>
…ctors Spark Iceberg DELETE writes Puffin; this customer table stores DVs as version-1 `.bin` objects with `content_offset = 1`. Co-authored-by: Cursor <cursoragent@cursor.com>
The root gitignore drops `*.bin`, so the previous commit omitted the object the test actually reads. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8e94ed1bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (isDeletionVectorV1Envelope(envelope, content_size_in_bytes)) | ||
| return IcebergDeletionVectorContainer::SliceOnly; |
There was a problem hiding this comment.
Reject unknown wrappers before selecting the slice-only path
When a Puffin header is damaged—or a .bin object uses an unknown version—this accepts any valid envelope found at the manifest offset as SliceOnly. That skips bindDeletionVectorBlob, including the footer checks for the referenced data file, fields, and cardinality, so malformed input can silently filter rows instead of being rejected. Require either the known Delta 0x01 container layout or an exact bare-envelope layout before taking this path.
AGENTS.md reference: AGENTS.md:L153-L153
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I did not find strong description what can and can't be in non-puffin DV files. So prefer to accept all, until report about specific files that must be declined.
CI triage — @ianton-ruVerdict: 0 of the failures are caused by this PR. They break down as 3 flaky/infra stateless tests (all in non-default storage configs), 2 integration shards killed by the 3-hour xdist session-timeout (with only pre-existing flaky tests failing inside them), and 1 base-image CVE scan. The PR's own feature is healthy: all 36 Head SHA analyzed: Per-failure breakdown
Why the two integration shards are the systemic issue (not the PR)Both Shard 5/8 contains none of this PR's new tests and still timed out — so the timeout is a capacity/sharding problem, not something the added tests introduced. The PR's Iceberg tests that did run were quick ( Suggested next steps
Automated triage from CI reports (praktika |
|
This is from the audit review. I was also wandering about this, but I don't see any unexpected behavior while testing this specific path. For now we agree fully that we will not be filtering out the deletion vector files as valid or invalid? PR #2271 Audit Findings
|
The same as codex find above. I did not find strong description what can and can't be in non-puffin DV files. So prefer to accept all, until report about specific files that must be declined. |
Iceberg v3 deletion vectors in Databricks Delta
|
| Kind | What it pins |
|---|---|
| Layout | A .bin is 0x01 + the byte-identical Iceberg blob; content_offset = 1 |
| Fixtures | Our builder matches ClickHouse’s own gtest blobs ({2,5}, {7,9}, two-slice container) |
| Spark blob + our wrapper | Crafted container around a real writer blob adds only the version byte |
| Converted writer table | In-place Puffin → .bin does not change visible rows |
Installed .bin |
Object overwrite, and a sibling deletion_vector_*.bin the manifest points at |
| Permissive wrappers | Bare envelope at offset 0; unexpected version byte; unknown 4-byte prefix — all accepted |
| Manifest vs bytes | file_format = PARQUET still classified as a DV from content_offset / content_size_in_bytes |
| Fail-closed | Misdeclared offset 5; declared size ≠ envelope length; object that is neither container |
| Golden table | Customer’s own Databricks UniForm table (see below) |
The three ways we get .bin files
We cannot run Databricks. Everything the reader can see reduces to: version-byte container, file_format = PUFFIN on a non-Puffin object, content_offset = 1, and (for the customer table) UniForm’s _iceberg/ metadata. Three complementary routes:
1. Craft the container
build_delta_bin / install_delta_bin: one version byte in front of the blob builder we already use for Puffin.
Used for hostile and edge cases a real writer will never produce — wrong offset, wrong size, no version byte, unknown prefix, truncated object, file_format mismatch. Also the path behind replace_vector_with_positions when a scenario installs a crafted bitmap.
2. Convert a Spark-written Puffin in place
Default for every reused scenario. Spark commits a normal v3 MoR table (real Puffin, real blob). The harness extracts each deletion-vector-v1 blob, writes 0x01 ‖ blob… over the same key, and patches content_offset / content_size_in_bytes / file_size_in_bytes. The blob bytes stay the writer’s; only the wrapper is ours.
This is what makes the existing suite a .bin suite without copying it. After conversion we assert the objects really are .bin — a no-op conversion would leave a Puffin table and the whole feature would pass while proving nothing.
3. Golden Databricks fixture
The customer table from the PR, stored under fixtures/databricks_uniform_v3/. We did not write the .bin, the Parquet, or the Avro. Install only rewrites S3 paths and flattens _iceberg/metadata → metadata/.
This is the only input that is not an expression of our own layout assumption. Databricks also run-length-encoded the roaring bitmap (cookie 12347, 251 bytes for 1000 positions); our writer emits array containers for the same positions, so this is a decode path crafting does not cover.
What we do for the Databricks / UniForm case
The customer shape, in one line: deletion_vector_<uuid>.bin beside the data files, manifest file_format = PUFFIN, content_offset = 1, content_size_in_bytes = 251, gzip-compressed Iceberg metadata, no version-hint.text.
- Install the frozen table and assert that layout (size 252, leading
0x01, notPFA1, offsets, cardinality 1000, run-encoded cookie) so a mangled fixture cannot produce a green read. - Read it through
icebergS3: 9000 live rows, ids1001..10000, the deletedId <= 1000band gone, trivialcount()agrees. - Replay the same file layout on crafted tables: sibling
.bin+ retargetedfile_path(how Databricks leaves aDELETE), andfile_format = PUFFINon a non-Puffin object (how Databricks labels it). - Fail closed on the layout people actually misread:
content_offset = 5(blob starts at the magic instead of the length prefix) — the error must name offset 5, not “out of bounds.” - Run the reused suite on converted Spark tables so access forms, counts, cluster reads, malformed blobs, and writer DML all see
.bincontainers, not only the one golden table.
The golden table is the fidelity anchor (one schema, one snapshot, customer bytes). Conversion plus crafting is the coverage. Together they are the Databricks .bin test.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Support for Databricks deletion vectors
Documentation entry for user-facing changes
Databricks put deletion vector into .bin file instead of puffin files.
CI/CD Options
Exclude tests:
Regression jobs to run: