Iceberg deletion vector support - #2119
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bd1ca373d
ℹ️ 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".
| const bool effective_need_only_count = need_only_count; | ||
| #endif | ||
|
|
||
| const bool can_use_count_cache = effective_need_only_count && headers_requested |
There was a problem hiding this comment.
_headers guard before using the count cache
When a count-style query requests _headers and a cached row count exists, this predicate enables the ConstChunkGenerator shortcut, so no data GET occurs and the result can contain metadata-probe/HEAD headers instead of the requested GET headers. Conversely, ordinary count queries without _headers can no longer use the cache. Require !headers_requested here so the shortcut is skipped rather than falling back to different headers.
AGENTS.md reference: AGENTS.md:L153-L153
Useful? React with 👍 / 👎.
| @@ -0,0 +1,46 @@ | |||
| --- | |||
There was a problem hiding this comment.
Both this new page and docs/en/interfaces/formats/Puffin/PuffinMetadata.md omit the mandatory sidebar_label and sidebar_position frontmatter fields, leaving the new documentation pages noncompliant with the repository's required metadata. Add both fields to each page's frontmatter.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
Restore `!headers_requested` after a merge-conflict resolution inverted the condition, which disabled the cache for ordinary counts and allowed wrong headers on cache hits. Co-authored-by: Cursor <cursoragent@cursor.com>
Include `sidebar_label`, `sidebar_position`, and input/output flags so the new pages match AGENTS.md and peer format docs. Co-authored-by: Cursor <cursoragent@cursor.com>
Without the guard, Fast test builds that disable Avro fail to compile because IcebergPathFromMetadata is only visible when Avro is enabled. Co-authored-by: Cursor <cursoragent@cursor.com>
Reviewer notes: Altinity PR #2119Title: Iceberg deletion vector support MotivationIceberg v3 replaces classic position-delete files with deletion vectors stored as roaring bitmaps inside Puffin files. Without this, ClickHouse can return rows that engines like Spark already treat as deleted. What landed (architecture)1. SQL formats
|
| Area | Behavior |
|---|---|
| Cluster protocol | Refuse to serialize tasks with non-empty excluded_rows / Iceberg eq+pos deletes / file_bucket_info when worker protocol is too old (would otherwise silently return deleted or duplicated rows). |
| DV vs equality deletes | Apply DeletionVectorTransform before equality FilterTransform so file row numbers stay valid. |
| Count shortcuts | Skip count-from-files cache when DVs / buckets / Iceberg equality+position deletes are present; require explicit total-equality-deletes == 0 for snapshot totalRows shortcut; disable need_only_count when equality deletes exist. |
_headers + count cache |
Count-cache shortcut skipped when _headers is requested (otherwise cache hit returns wrong headers). |
| Data format | Reject DVs on non-Parquet data files. |
| Avro-disabled builds | IcebergDeletionVector.cpp wrapped in #if USE_AVRO (Fast test without Avro). |
Tests
- GTests: Puffin envelope/cardinality/bounds, files cache, DV-before-equality order, Iceberg count shortcuts, data-object clone, Parquet row-delete guards, ParquetV3
need_only_count+ buckets. - Stateless:
04077/04257/04260/04548/04549/04612/04613(Puffin formats);04261–04263(Iceberg DV + cache) with MinIO fixtures undertests/queries/0_stateless/data_minio/dv_puffin_warehouse/anddata_puffin/. - Integration:
tests/integration/test_storage_iceberg_with_spark/test_deletion_vectors.py.
Follow-up commits on top of the main cherry-pick
These are small fixes after the initial Iceberg deletion vector commit / review / CI:
!headers_requestedpolarity — merge-conflict typo had inverted the count-cache guard.- Puffin docs frontmatter —
sidebar_label/sidebar_position(+ input/output flags) for AGENTS.md compliance. USE_AVROguard onIcebergDeletionVector.cpp— fixes Fast test compile without Avro.- Settings history — Antalya version key only for
use_puffin_files_cache.
Out of scope / intentional limits
- No writing Iceberg deletion vectors.
- Non-
deletion-vector-v1Puffin blob types are skipped by the SQLPuffinformat. - Selecting only
referenced_data_fileskips payload I/O (and thus envelope/CRC/materialization checks) by design. - Poco JSON
Int64wrap of values near2^63in footer parsing was reviewed and deferred (not treated as a blocker in this PR).
Suggested review focus
- Manifest →
excluded_rowsattachment and path matching (referenced_data_filevs data file). - Transform order (DV before equality) and cluster fail-closed protocol checks.
- Count /
totalRows/ cache interactions — easy to silently over-count. - Resource bounds in
PuffinDeletionVectorReader(cardinality ceiling, envelope peek before allocate). - Antalya-specific settings history entry vs upstream versioning.
Links
- PR: Iceberg deletion vector support #2119
- Related upstream Puffin reader: Puffin file reader ClickHouse/ClickHouse#109990
- Iceberg Puffin spec: https://iceberg.apache.org/puffin-spec/
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b30a8f3e8
ℹ️ 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".
| for (UInt64 position : deleted_positions) | ||
| bitmap->add(static_cast<size_t>(position)); |
There was a problem hiding this comment.
When a deletion vector contains a position greater than or equal to the referenced data file's manifest record_count, this loop inserts it without validation; the transform never encounters that position and silently returns rows that the manifest claims were deleted. This can also make a full scan disagree with Iceberg's trivial count() calculation, which subtracts the deletion vector's record_count. Pass the data file row count into the loader and reject out-of-range positions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for the note. Agreed that we do not currently bound-check positions against the data file’s manifest record_count (that field lives on IcebergDataObjectInfo / the data-file entry and is not passed into loadDeletionVector).
A few clarifications on impact:
- In-range deletes are unaffected.
DeletionVectorTransformonly filters rows whose file-local indices appear in the bitmap (rb_containsonChunkInfoRowNumbers). Positions past the real file never match a produced row, so they are no-ops for the scan — they do not revive deleted in-range rows. - What “manifest claims deleted” means here. The DV entry’s own
record_countis already checked exactly against the roaring bitmap cardinality inPuffinDeletionVectorReader. The gap is only vs the data file row count. OOB positions mean some declared deletes are ineffective, not that valid deletes are skipped. - Count disagreement with the snapshot
totalRowsshortcut (total_records - total_position_deletes) would require the table metadata/summary itself to be inconsistent with the on-disk DV (corrupt or non-spec writer). For normal Spark/Iceberg-written tables, writers keep positions within the file.
So this is a reasonable fail-closed hardening against bad metadata (and we could pass object_info->info.record_count into the loader and reject OOB positions), but we do not treat it as a correctness bug for well-formed Iceberg v3 deletion vectors. Prefer to leave it out of this PR unless reviewers want the extra check now.
| if (etag.empty()) | ||
| return std::nullopt; | ||
|
|
||
| return PuffinFilesCacheKey{file_path, etag, content_offset, content_size_in_bytes, referenced_data_file}; |
There was a problem hiding this comment.
When two manifest versions reference the same Puffin path, ETag, slice, and data file but declare different record_count values, this key aliases them even though deserialization validates the bitmap against that declaration. After one version populates the cache, the other receives the cached bitmap without running its cardinality check, so malformed or inconsistent metadata is accepted depending on query order. Include the expected cardinality in the key or revalidate it on every cache hit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks. Confirmed: tryCreateKey / PuffinFilesCacheKey currently use (file_path, etag, content_offset, content_size_in_bytes, referenced_data_file) and omit the manifest record_count passed as expected_cardinality.
Why this is weaker than it sounds for query correctness:
- A cache entry is only created after a successful uncached load. That path always runs
readDeletionVectorFromPuffin(..., expected_cardinality), which requires the roaring bitmap cardinality to exactly match the declared value. A mismatched declaration never populates the cache. - Same path + etag + slice ⇒ same blob bytes. The true bitmap (and thus true cardinality) is fixed. The only realistic “two declarations, one key” case is inconsistent metadata for the same physical object — not two different bitmaps aliased together.
- On a later hit with a wrong declaration, we skip re-checking cardinality and return the already-validated bitmap. Scan filtering still matches the on-disk DV; we fail to detect that this manifest entry’s
record_countdisagrees with the blob.
So the finding is a real gap for fail-closed metadata validation (and adding cardinality to the key, or checking bitmap->size() == expected_cardinality on hit, would close it). It is not a silent wrong-bitmap / wrong-answer bug for consistent Iceberg tables.
Prefer to leave the key as-is for this PR unless reviewers want the stricter check now; happy to follow up with cardinality in the key (and a small gtest) if desired.
|
Failed tests looks unrelated to PR. Stateless test Regression tests are useless unless all Antalya features are merged in the branch. Were turned on by default, I have turned them off only right now. |
ianton-ru
changed the title
|
Doesn't it make sense to emphasize differences between this PR and 109990 (unless you are totally skeptical about its chances to be accepted to upstream)? |
|
109990 Is only a Puffin reader. |
|
By the way I want to create PR with this changes in upstream too, but it is possible only after accept of puffin reader. |
2 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Iceberg deletion vector support
Documentation entry for user-facing changes
Iceberg deletion vector support.
Based on uncommited Puffin file reader ClickHouse#109990.
CI/CD Options
Exclude tests:
Regression jobs to run: