Iceberg deletion vector support (attempt #2)

[ianton-ru]

Changelog category (leave one):

• New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Iceberg deletion vectors support

Documentation entry for user-facing changes

Goal

Add read support for Iceberg v3 Puffin deletion vectors (deletion-vector-v1) so ClickHouse applies DV bitmaps when reading Iceberg tables (local / object storage / cluster), without writing DVs.
Also expose SQL input formats Puffin / PuffinMetadata for inspecting Puffin files, and a process-global Puffin files cache for parsed DV bitmaps.

---

High-level architecture

Manifest (position deletes, content=2)
        │
        ▼
IcebergIterator ──loadDeletionVector──► Puffin footer bind + blob read
        │                                      │
        │                                      ▼
        │                              PuffinFilesCache (optional)
        ▼
IcebergDataObjectInfo.excluded_rows  (roaring bitmap of deleted positions)
        │
        ▼
StorageObjectStorageSource / DeletionVectorTransform
        │
        ├── need_only_count → cardinality via roaring rank (no Filter materialization)
        └── full read → exclude rows (DV before equality deletes)

Shared Puffin parsing / DV deserialize lives under:

Component	Role
PuffinFile	Footer parse (seekable), blob metadata, DV footer bind
PuffinDeletionVectorReader	Envelope peek, CRC, roaring deserialize, size ceilings
PuffinFilesCache	Context-global cache of cloned exclusion bitmaps
IcebergDeletionVector	Iceberg-specific load + validation vs data-file record_count
PuffinBlockInputFormat	SQL Puffin / PuffinMetadata
Iceberg path uses seekable object-storage reads. SQL formats also support a non-seekable fallback (pipes / input_format_allow_seeks = 0).

---

Feature behavior (what users get)

1. Iceberg reads honor live Puffin DVs attached as position-delete manifest entries (content = 2 / deletion vectors).
2. Non-Parquet data files with DVs are rejected (fail closed).
3. Equality deletes still work; DVs are applied before equality filters so file-local row numbers stay correct.
4. Trivial / snapshot COUNT shortcuts fail closed when any live deletes (equality, position files, or DVs) are present — do not trust poisoned snapshot summaries or naive data − deletes arithmetic.
5. Cluster / parallel read fails closed if the cluster protocol cannot carry excluded_rows or delete metadata (no silent drop of deletes).
6. SYSTEM DROP PUFFIN FILES CACHE (spaced form; underscore alias accepted) clears the cache; gated by access control.
7. Settings: use_puffin_files_cache and related server/cache size settings (see Settings / docs).

---

Safety / fail-closed decisions (intentional)

Reviewers should treat these as product decisions, not accidental omissions:

• Absolute ceilings (not FormatSettings knobs): footer payload (16 MiB), DV blob size (2 GiB, Iceberg-aligned), materialized positions, non-seekable buffer size.
• Envelope peek before allocating full DV blob; CRC after bounded read.
• Footer bind: unique blob at (content_offset, content_size) matching referenced_data_file + cardinality.
• Positions must be < data_file.record_count.
• Cache keys include storage identity, path, etag, slice, referenced data file, expected cardinality, and data-file row count.
• Cache returns clones of bitmaps so callers cannot mutate shared cache state.
• Weak / empty etags skip the cache (isEtagUsableAsCacheKey).
• COUNT / need_only_count: prefer roaring cardinality / rank; avoid building a full Filter over all file rows when only a count is needed; skip file-level count cache when excluded_rows is present.
  Explicitly out of scope / deferred (workspace rule): Poco JSON Int64 wrap of 2^63 / 2^63+1 — do not treat as a defect to fix in this PR.

---

Tests (where to look)

Unit / gtest

• Puffin: envelope, bounds, cardinality, footer bind, referenced_data_file, non-seekable buffer limit
• Cache: key (incl. storage identity), clone, weight, metrics (clear-during-load, waiter, hit-after-clear)
• Iceberg: count shortcuts, DV positions, position-delete kind presence, parquet row-deletes guard
• Parquet: need_only_count with buckets, row-group global offsets
• CacheBase / LRU: getOrSetWithOutcome*

Stateless

• Puffin happy path, allow_seeks=0, stdin pipe, error fixtures under tests/queries/0_stateless/data_puffin/
• SYSTEM DROP PUFFIN FILES CACHE parsing / privileges
  Integration
• tests/integration/test_storage_iceberg_with_spark/test_deletion_vectors.py
• MinIO fixtures under data_minio/ (dv_puffin_*), generator generate_iceberg_dv_fixture.py

---

Docs touched

• Iceberg table engine / table function
• Puffin / PuffinMetadata formats
• SYSTEM DROP PUFFIN FILES CACHE

CI/CD Options

Exclude tests:

• Fast test
• Integration Tests
• Stateless tests
• Stateful tests
• Performance tests
• Aarch64 tests
• All with ASAN
• All with TSAN
• All with MSAN
• All with UBSAN
• All with Coverage
• All Regression
• Disable CI Cache

Regression jobs to run:

• Fast suites (mostly <1h)
• Aggregate Functions (2h)
• Alter (1.5h)
• Benchmark (30m)
• ClickHouse Keeper (1h)
• Iceberg (2h)
• LDAP (1h)
• OAuth (5m)
• Parquet (1.5h)
• RBAC (1.5h)
• SSL Server (1h)
• S3 (2h)
• S3 Export (2h)
• Swarms (30m)
• Tiered Storage (2h)

[github-actions]

Workflow [PR], commit [61c3ec7]

[Selfeer]

Aside from my own tests, I'm attaching an audit review for this PR, please check if any of these make sense and need fixing - I've tried to make it as easily readable as possible.

---

Audit Review — PR #2183

PR: Altinity/ClickHouse#2183 — Iceberg deletion vector support (attempt 2)

AI audit note: This review was generated by AI (gpt-5.6-sol). Static reasoning only — nothing was compiled or executed.

---

High severity

1. ALTER TABLE ... DELETE/UPDATE on a v3 table with deletion vectors silently deletes nothing

When ClickHouse runs a mutation on an Iceberg table, it writes the deleted row positions as parquet position-delete files (Mutations.cpp). The Iceberg v3 spec forbids adding position-delete files to tables that use deletion vectors, and requires readers to ignore position-delete files for any data file that has a DV — which is exactly what this PR's reader now correctly does (IcebergIterator.cpp:402 skips parquet position deletes whenever a DV matches the data file).

Put together: on a v3 table written by Spark/Iceberg ≥ 1.8 (where DVs are the default), a ClickHouse ALTER TABLE ... DELETE commits a delete file that every spec-compliant reader — ClickHouse itself, Spark, Trino — must ignore. The ALTER reports success, and the "deleted" rows keep coming back in every subsequent SELECT. No error is raised anywhere.

The only guard on the mutation path today is format_version < 2; there is no check for DVs or v3. Before this PR the problem was unreachable, because reading a DV table failed with "Position deletes are supported only for parquet format" — the PR makes these tables readable without making the write side safe.

Fix: make checkMutationIsPossible (or mutate) throw when the current snapshot contains any live deletion vectors, or when format-version >= 3.

---

Medium severity

2. The non-seekable Puffin buffer limit was written but never wired in — the SQL path still buffers the whole stream

The PR adds appendReadBufferWithAbsoluteSizeLimit and a PUFFIN_NON_SEEKABLE_MAX_BUFFERED_SIZE constant, with a comment explaining they exist so that "a crafted pipe cannot allocate unbounded memory before footer-length validation". But no production code calls them — the only caller is the unit test.

The actual non-seekable path in readPuffinFooter (PuffinBlockInputFormat.cpp:519) still reads the entire stream into memory in a loop, with nothing checked beforehand except the 4-byte PFA1 magic:

std::vector<UInt8> tmp(DEFAULT_BLOCK_SIZE);
while (!buf.eof())
{
    size_t n = buf.read(reinterpret_cast<char *>(tmp.data()), tmp.size());
    result.data.insert(result.data.end(), tmp.data(), tmp.data() + n);
}

So SELECT * FROM url('http://attacker/x', Puffin) (or Puffin from stdin, or with input_format_allow_seeks = 0) can be fed PFA1 followed by endless junk, and memory grows until the query memory limit kicks in — far past the ~2 GiB ceiling the helper was designed to enforce.

Fix: replace the raw loop with a call to appendReadBufferWithAbsoluteSizeLimit(buf, result.data, PUFFIN_NON_SEEKABLE_MAX_BUFFERED_SIZE).

3. The SQL Puffin format uses roaring bitmaps from untrusted files without validating them

The PR's new deletion-vector reader (PuffinDeletionVectorReader.cpp) deserializes roaring bitmaps and then calls roaring_bitmap_internal_validate, with a comment explaining why: readSafe only bounds the read; CRoaring requires internal validation before an untrusted bitmap can be safely used.

The SQL Puffin format has its own older copy of the same helper (readRoaringPortableSafe in PuffinBlockInputFormat.cpp) that skips this validation — and then runs getSizeInBytes, cardinality, and iteration on the unvalidated bitmap. This is the most attacker-exposed surface of the two, since it parses arbitrary user-supplied files via file/url/s3. A crafted file (for example a run container with start + length > 65535, with a correctly recomputed CRC) passes every check and gets consumed with its internal invariants broken; at minimum the output positions can be garbage, duplicated, or unsorted.

Fix: add the same roaring_bitmap_internal_validate call to the format path — or better, delete the duplicate and have both paths share the validated helper.

4. count() via the trivial-count shortcut can silently overflow and return a wrong number

IcebergMetadata::totalRows was carefully designed to fail closed: per-manifest row sums are overflow-guarded, negative counts bail out, and snapshot summaries are treated as untrustworthy hints. But the final loop that adds the per-manifest sums together is a plain result += *manifest_rows; on a UInt64 with no overflow check (IcebergMetadata.cpp:1098).

Each manifest's sum is individually capped at Int64::max, so with three or more manifests declaring huge counts (crafted or corrupt metadata), the total wraps around and count() returns a small wrong number — no exception, no fallback to a real scan, just a warning in the log if the summary happens to disagree.

Fix: accumulate with common::addOverflow and return nullopt (fall back to a real scan) on overflow, matching how every other lane of this function behaves.

---

Low severity

5. Setting puffin_files_cache_size = 0 makes DV reads slower than having no cache at all

There are two ways to "turn off" the Puffin cache, and they behave very differently. use_puffin_files_cache = 0 takes the uncached path, which keeps the filesystem cache enabled. But puffin_files_cache_size = 0 still takes the cached path — where the loader deliberately sets disable_filesystem_cache = true to avoid double-caching (IcebergDeletionVector.cpp:254) — and every insert into the zero-capacity cache is immediately evicted. The result: every DV read re-fetches the footer and blob from remote storage with no caching at any layer, plus an extra HEAD request per read to fetch the etag for a cache key that will never be used.

Fix: treat a zero-capacity cache the same as use_puffin_files_cache = 0 in loadDeletionVector.

6. puffin_files_cache_max_entries says "Zero means disabled" but zero means unlimited

The description of the new server setting (ServerSettings.cpp:566) claims zero disables the cache. In LRUCachePolicy/SLRUCachePolicy, max_count == 0 means no entry-count limit — only puffin_files_cache_size = 0 actually disables it. An operator setting max_entries = 0 to turn the cache off gets a count-unbounded cache instead. (The wording was copied from the parquet metadata cache setting, which has the same error.)

Fix: change the description to "Zero means unlimited."

7. The settings-history entry is dated to a version the change can't ship in

SettingsChangesHistory.cpp records use_puffin_files_cache under 26.6.1.20001.altinityantalya, but the branch is already versioned 26.6.2.20000, so no 26.6.1.x release can contain this setting. system.settings_changes will attribute it to a version that never had it. Harmless in practice only because the old and new defaults are both true.

Fix: bump the entry to the actual shipping version (e.g. 26.6.2.20001.altinityantalya).

8. The new privilege has a useless alias and is missing the useful one

SYSTEM_DROP_PUFFIN_FILES_CACHE in AccessType.h declares the alias "SYSTEM DROP PUFFIN FILES CACHE" — identical to the privilege's own name, so it does nothing (and shows up as a visibly odd self-alias in SHOW PRIVILEGES, unique among all entries). Meanwhile the underscore spelling SYSTEM DROP PUFFIN_FILES_CACHE is accepted by the statement parser but not by GRANT, breaking the convention every sibling cache privilege follows (compare SYSTEM DROP PARQUET_METADATA_CACHE).

Fix: change the alias to "SYSTEM DROP PUFFIN_FILES_CACHE" and regenerate 01271_show_privileges.reference.

9. PuffinFilesRead misses the biggest read on the SQL path

The profile event is described as "Number of Puffin files read (footer or deletion vector blob)", and the Iceberg path counts both. But on the SQL Puffin format path, only the footer parse increments the counter — the blob read (up to 2 GiB, the heaviest IO of the whole operation) is neither counted in PuffinFilesRead nor timed in PuffinFileReadMicroseconds. The same logical read reports different numbers depending on which path performed it.

Fix: add the profile-event scope to readDeletionVectorBlobBytes in PuffinBlockInputFormat.cpp.

10. Parquet files with inconsistent metadata that used to be readable now fail on every read

The new buildRowGroupGlobalOffsets (Parquet/Reader.cpp:352) throws INCORRECT_DATA when the sum of row-group row counts doesn't equal FileMetaData.num_rows — and it runs on every ParquetV3 read, not just the DV/count paths that actually need the consistency guarantee. Some third-party writers produce files with this mismatch; those files were readable before this PR (the old normal path never consulted the top-level num_rows) and now throw on a plain SELECT.

Fix: enforce the equality only where it's load-bearing (prepareNeedOnlyCountRowGroups / DV paths), or downgrade to a warning on the normal read path.

11. The PositionDeleteKindPresence gate is tested but never used

getPositionDeleteKindPresence and the iterator-level getRowsCountInAllFilesExcludingDeleted in ManifestFileIterator.cpp have no production callers — totalRows uses direct .empty() checks instead (which are strictly stronger, so behavior is fine). But gtest_iceberg_position_delete_kind_presence.cpp has test names like CoexistenceFailsClosedGate and "Mimic totalRows" that certify wiring which doesn't exist. A future reader will assume this gate protects production when it doesn't.

Fix: either wire totalRows through these helpers or delete them and rename the tests.

12. Dead duplicate empty-etag branch in loadDeletionVector

PuffinFilesCache::tryCreateKey only returns nullopt when the etag is empty — but loadDeletionVector already handled the empty-etag case and returned earlier. The !cache_key fallback branch (IcebergDeletionVector.cpp:237), which logs "because etag is empty" a second time, is unreachable.

Fix: drop the branch, or make tryCreateKey the single gate and remove the earlier check.

13. One of the new cache gtests is timing-dependent and can flake

In gtest_puffin_files_cache_metrics.cpp (WaiterOfClearDiscardedLoadCountsAsMiss), the waiter thread signals readiness before it actually calls into the cache, and the producer is released after a fixed 50 ms sleep. On a slow/loaded CI machine the producer can finish before the waiter reaches the cache, at which point the waiter runs its own load and the assertions EXPECT_FALSE(waiter_load_called) / EXPECT_EQ(load_calls, 1) fail. Nothing synchronizes on "the waiter is blocked on the token" except the sleep.

Fix: retry the scenario until load_calls == 1, or relax the assertions to what's actually deterministic (hits == 0).

14. Cluster-protocol tests live in the wrong file

The 226 lines of new tests for ClusterFunctionReadTaskResponse fail-closed serialization (old workers must get an exception rather than silently losing deletion vectors, iceberg deletes, or bucket info) are correct and valuable — but they were added to gtest_rendezvous_hashing.cpp, a file about task-distribution hashing. Nobody will find them there.

Fix: move them to a dedicated file, e.g. gtest_cluster_function_read_task.cpp.

15. The fail-closed protocol checks stop one field short (pre-existing gap)

This PR adds three good fail-closed checks to ClusterFunctionReadTaskResponse::serialize: workers on an old protocol version get UNKNOWN_PROTOCOL instead of silently missing excluded rows, iceberg deletes, or bucket info. But right next to them, data_lake_metadata.schema_transform is still silently dropped for workers below protocol version 2 — the exact failure mode (silently wrong results on the worker) the new checks were added to prevent. This gap predates the PR, but the PR establishes the pattern and leaves the hole beside it.

Fix: add a fourth check — throw when the worker protocol can't carry a non-trivial schema_transform.

[ianton-ru]

Test 04401_system_reset_ddl_worker_access is failed, because DB::ASTSystemQuery::Type now has more than 128 elements, and magic_enum is broken. Default value for MAGIC_ENUM_RANGE_MAX is 127.

[ianton-ru]

In upstream fix is in commit 2dd1e2cbfb93798aaa7eb81b6bb4c2f6a77a33f7, cherry-picked here