Iceberg deletion vector support (attempt #2) - #2183
Conversation
totalRows was aggregating optional column value_counts, which can disagree with row counts for nested fields; fail closed on negative or overflowing record_count instead. Co-authored-by: Cursor <cursoragent@cursor.com>
…deletion-vectors-read-2
…arser. Related: Altinity#2179 Related: Altinity#2183 Co-authored-by: Cursor <cursoragent@cursor.com>
Footer reads lost their profile event when the Iceberg path moved onto the shared `Puffin` format reader, which halved `PuffinFilesRead` in `04263_iceberg_puffin_files_cache`. Co-authored-by: Cursor <cursoragent@cursor.com>
|
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
High severity1.
|
ClickHouse mutations write parquet position-delete files that readers must ignore for data files with a DV, so fail closed instead. Only DELETE manifests are scanned. Co-authored-by: Cursor <cursoragent@cursor.com>
Wire appendReadBufferWithAbsoluteSizeLimit into the SQL footer fallback so crafted pipes cannot grow memory past the DV+footer ceiling. Co-authored-by: Cursor <cursoragent@cursor.com>
Match the Iceberg deletion-vector reader: call roaring_bitmap_internal_validate so untrusted files cannot be used with broken CRoaring invariants. Co-authored-by: Cursor <cursoragent@cursor.com>
Guard totalRows accumulation with addOverflow so a wrapped UInt64 sum falls back to a real scan instead of returning a wrong count. Co-authored-by: Cursor <cursoragent@cursor.com>
puffin_files_cache_size=0 still entered the cache miss path and disabled filesystem cache; skip that path when the LRU accepts no entries. Co-authored-by: Cursor <cursoragent@cursor.com>
LRU/SLRU treat max_count=0 as no entry-count limit, not disabled. Co-authored-by: Cursor <cursoragent@cursor.com>
Use the underscore form as the GRANT alias instead of a spaced self-alias, matching parquet/iceberg metadata cache privileges. Co-authored-by: Cursor <cursoragent@cursor.com>
The Iceberg path already accounted for blob I/O; the SQL Puffin format path only counted footer reads. Co-authored-by: Cursor <cursoragent@cursor.com>
Offsets follow the row-group layout; rejecting a stale file-level count broke otherwise readable ParquetV3 files on every read path. Co-authored-by: Cursor <cursoragent@cursor.com>
The helper classifies DV vs parquet deletes for callers such as mutation rejection; totalRows fail-closes on any live position deletes. Update comments and gtest names that implied totalRows wiring. Co-authored-by: Cursor <cursoragent@cursor.com>
Empty etag is already handled before tryCreateKey; treat a later nullopt as LOGICAL_ERROR instead of repeating the uncached path. Co-authored-by: Cursor <cursoragent@cursor.com>
Wait for insert-token refcount >= 2 instead of sleeping 50ms before clear, which raced when the producer finished before the waiter joined. Co-authored-by: Cursor <cursoragent@cursor.com>
They do not belong in the rendezvous hashing gtest; keep them next to ClusterFunctionReadTask under Interpreters/tests. Co-authored-by: Cursor <cursoragent@cursor.com>
Protocol < 2 omitted data-lake schema evolution silently; reject the task instead, matching excluded_rows / Iceberg deletes / bucket checks. Co-authored-by: Cursor <cursoragent@cursor.com>
…lds. Align the `Puffin` format docs with `validateDeletionVectorV1Fields` after Spark file-scoped DVs were allowed. Co-authored-by: Cursor <cursoragent@cursor.com>
Reuse requireDeletionVectorV1Properties from the SQL Puffin format and a single checkDeletionVectorBlobReadLimits helper so bind/read paths stop duplicating validation. Co-authored-by: Cursor <cursoragent@cursor.com>
It was only used by the dropped Iceberg mutation DV scan; drop the API, iterator wrapper, dead include, and gtest. Co-authored-by: Cursor <cursoragent@cursor.com>
With snapshot-summary totalRows restored, optimize_count_from_files=0 alone still answered from Iceberg metadata; also set optimize_trivial_count_query=0 so the test forces Parquet decoding. Co-authored-by: Cursor <cursoragent@cursor.com>
Iceberg does not chain another row-number consumer after a DV on this path, so skip Filter/rb_range materialization and restore the cheap rb_range_cardinality + cloneResized fast path. Co-authored-by: Cursor <cursoragent@cursor.com>
…deletion-vectors-read-2 Resolve conflicts by keeping deletion-vector read paths and count fail-closed behavior, while adopting multistorage path resolution from antalya-26.6. Co-authored-by: Cursor <cursoragent@cursor.com>
DV tables often report total-equality-deletes=0 while still having position deletes; the summary path can under-count when a DV supersedes parquet position deletes, so fall through to the real scan / need_only_count path. Co-authored-by: Cursor <cursoragent@cursor.com>
Manifest column byte sizes and cross-manifest totals now reject negative or overflowing sums instead of wrapping, matching the totalRows contract. Co-authored-by: Cursor <cursoragent@cursor.com>
Footer memo no longer gets a second full copy of puffin_files_cache_size; its weight is charged into PuffinFilesCacheBytes/Files and trimmed when DV entries grow so resident usage cannot reach ~2x the configured maximum. Co-authored-by: Cursor <cursoragent@cursor.com>
|
I've run into these three issues in my tests after the latest changes:
The first one seems to be directly related to deletion vectors. The other two are not entirely deletion-vector-related, but they were triggered by the latest changes in this PR. |
A tiny non-zero puffin_files_cache_size previously took the cached path, disabled filesystem cache, paid an etag HEAD, then immediately evicted. Fall back to the uncached loader when the minimum entry weight exceeds the budget. Co-authored-by: Cursor <cursoragent@cursor.com>
Snapshot summary can keep total-position-deletes after deletes are already applied into rewritten data files; add regression coverage that the COUNT shortcut stays closed and count() matches the scan. Co-authored-by: Cursor <cursoragent@cursor.com>
Register a scheme-less absolute path and a flat delete file so ClickHouse remaps onto object storage after upload instead of LocalObjectStorage. Co-authored-by: Cursor <cursoragent@cursor.com>
Assert the snapshot-summary shortcut stays off when DVs or equality deletes are live, and that count() still matches a full scan. Co-authored-by: Cursor <cursoragent@cursor.com>
Poisoned or incrementally incorrect total-records must not become SELECT count(); keep summary only for a mismatch warning. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@Selfeer All three issues about wrong |
mkmkme
left a comment
There was a problem hiding this comment.
I didn't have time for a proper human review. But multiple rounds of review with AI ended up in the place where AI no longer has any complaints about the code. Approving this one. But will need to read it later myself.
PR #2183 CI Triage — Iceberg deletion vector support (attempt #2)
Bottom lineNo CI failure is a functional regression caused by this PR's code. The PR's own feature tests (stateless Summary
Detailed analysis1.
|
|
Latest regression run for delete vectors is fully green now after last changes, will upload the logs a bit later - the PR seems good to be merged. |
…deletion-vectors-read-2
CI triage for
|
| Check | Classification | PR-caused? |
|---|---|---|
| DCO | Missing Signed-off-by on commits |
Housekeeping (not a code defect) |
| RegressionTestsRelease / Common (settings) | clickhouse-regression snapshot drift |
No — trigger is a base-branch setting |
| CAS (aggregate_functions, alter, atomic_insert, engines, lightweight_delete, selects) ×6 | Pre-existing / infra | No |
| TieredStorageCAS / tiered_storage_cas | Pre-existing / infra | No |
| GrypeScanServer (-alpine) | Base-image CVE | No — infra |
1. DCO — housekeeping (not a code defect)
Every commit in this PR (origin/antalya-26.6..c113b0c) lacks a Signed-off-by trailer, so the DCO app red-flags the whole set (including the cherry-picked c04e775e from Sema Checherinda).
Fix: the author signs off the branch (e.g. git rebase --signoff origin/antalya-26.6 then update the PR head), or a maintainer overrides DCO for this internal PR. I can't do this myself — it needs a force-update of the PR head branch, which is outside my permissions.
2. RegressionTestsRelease / Common (settings) — snapshot drift, not this PR's code
Failing scenarios: /settings and /settings/default values. The decisive log line from the report:
SnapshotNotFoundError
filename=.../settings/tests/snapshots/default_values.py.default values>=26.6_antalya.snapshot
name=export_merge_tree_part_schema_mismatch_mode
actual_value = '{"default":"strict"}'
The setting that trips it is export_merge_tree_part_schema_mismatch_mode, which is not in this PR (git grep of the PR diff: 0 hits). It came into antalya-26.6 via the allow_non_matching_schema_export_partition_by_position merge (#2229/#2111) and hasn't been added to the regression snapshot yet. The base-branch run of this suite passed only because that snapshot check runs alphabetically and this setting post-dates the last base run.
Fix (in the separate Altinity/clickhouse-regression repo, not here): add the missing entries to settings/tests/snapshots/default_values.py.default values>=26.6_antalya.snapshot. Note that once export_merge_tree_part_schema_mismatch_mode is added, this PR's own new settings will hit the same check next and must be added in the same snapshot:
puffin_files_cache_policy, puffin_files_cache_size, puffin_files_cache_max_entries, puffin_files_cache_size_ratio (server settings) and use_puffin_files_cache (session setting). No source change is needed in this PR for this — the settings themselves are declared correctly with SettingsChangesHistory handled.
3. CAS ×6 + TieredStorageCAS — pre-existing / infra
All seven fail with the identical shape (35 retries: 7 ok, 2 failed, 26 retried) across unrelated modules (aggregate_functions, alter, atomic_insert, engines, lightweight_delete, selects). That uniform signature across modules this PR doesn't touch is characteristic of a shared environment problem, not code. Confirmed pre-existing: the same suites fail on the base branch antalya-26.6 (64c64b0, the latest MasterCI run) — there under RegressionTestsAarch64 / CAS (…) and TieredStorageCAS.
Next step: safe to ignore for this PR / re-run. Not a blocker introduced here. (Happy to dig into a specific CAS module's failing step if you want it chased down separately.)
4. GrypeScanServer (-alpine) — base-image CVE, infra
1 high/critical vulnerability in the alpine server image only; the non-alpine server image and the keeper image both scan clean (0). This is a base-image CVE, unrelated to C++ changes, and it also fails on the base branch antalya-26.6.
Next step: infra/base-image bump, tracked outside this PR.
Health check
The PR's actual subject matter is in good shape: the two Iceberg regression suites and all functional/fuzzer checks are green on c113b0c, and the diff's new settings + SYSTEM DROP PUFFIN_FILES_CACHE access type + SettingsChangesHistory entries are internally consistent. The only item that genuinely needs attention before merge is DCO sign-off; the settings regression needs a snapshot update in clickhouse-regression (for the base-branch setting and this PR's puffin settings), and the CAS/tiered-storage/Grype-alpine reds are pre-existing on antalya-26.6.
I did not push anything — no source change to this PR is warranted by these failures. Let me know if you'd like me to open a PR against Altinity/clickhouse-regression to refresh the settings snapshot.
🤖 automated triage by @blau-ai — evidence: PR check-runs on c113b0c, base-branch run 64c64b0, and the settings regression report.
Changelog category (leave one):
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/PuffinMetadatafor inspecting Puffin files, and a process-global Puffin files cache for parsed DV bitmaps.High-level architecture
Shared Puffin parsing / DV deserialize lives under:
PuffinFilePuffinDeletionVectorReaderPuffinFilesCacheIcebergDeletionVectorrecord_countPuffinBlockInputFormatPuffin/PuffinMetadatainput_format_allow_seeks = 0).Feature behavior (what users get)
content = 2/ deletion vectors).data − deletesarithmetic.excluded_rowsor delete metadata (no silent drop of deletes).SYSTEM DROP PUFFIN FILES CACHE(spaced form; underscore alias accepted) clears the cache; gated by access control.use_puffin_files_cacheand related server/cache size settings (seeSettings/ docs).Safety / fail-closed decisions (intentional)
Reviewers should treat these as product decisions, not accidental omissions:
(content_offset, content_size)matchingreferenced_data_file+ cardinality.< data_file.record_count.isEtagUsableAsCacheKey).excluded_rowsis present.Explicitly out of scope / deferred (workspace rule): Poco JSON
Int64wrap of2^63/2^63+1— do not treat as a defect to fix in this PR.Tests (where to look)
Unit / gtest
CacheBase/ LRU:getOrSetWithOutcome*Stateless
allow_seeks=0, stdin pipe, error fixtures undertests/queries/0_stateless/data_puffin/SYSTEM DROP PUFFIN FILES CACHEparsing / privilegesIntegration
tests/integration/test_storage_iceberg_with_spark/test_deletion_vectors.pydata_minio/(dv_puffin_*), generatorgenerate_iceberg_dv_fixture.pyDocs touched
Puffin/PuffinMetadataformatsSYSTEM DROP PUFFIN FILES CACHECI/CD Options
Exclude tests:
Regression jobs to run: