feat(transaction): with_snapshot_id() across actions, MergeAppend auto-compaction - #8
Draft
srinath-prabhu wants to merge 141 commits into
Draft
srinath-prabhu wants to merge 141 commits into
srinath-prabhu wants to merge 141 commits into
Conversation
Implements manifest merging matching Java SDK's MergingSnapshotProducer: - After each commit, checks manifest count vs min-count-to-merge (default 100) - Groups small manifests (<target-size-bytes, default 8MB) by partition spec - Bin-packs and merges them into target-sized manifests - Entries change status Added → Existing in merged manifests - Replaces both try_merge_into_existing (crude) and Tessellate manifest rewriting (external) Controlled by existing table properties: commit.manifest-merge.enabled = true commit.manifest.min-count-to-merge = 100 (default) commit.manifest.target-size-bytes = 8388608 (8MB default) For our scenario (6 manifests/commit, 30s interval): Merge triggers every ~16 commits (8 min) Merges ~100 small manifests into ~1 target-sized manifest Amortized cost: ~9ms/commit (one S3 batch read + one write) Manifest count: bounded at ~100 instead of growing unbounded
…ate_unique_snapshot_id
Lets callers pre-allocate the new snapshot's id before commit so other
parts of the same transaction can reference it. Concrete need: when a
`FastAppendAction` runs together with `update_statistics().set_statistics(...)`
in one transaction, the `StatisticsFile` entries key the per-snapshot
map on `snapshot_id`. Without this API the caller has no way to know
the action's snapshot_id pre-commit, so every entry gets registered
under `snapshot_id=0` and `metadata.statistics_for_snapshot(current)`
returns `None`.
Pattern:
use iceberg::transaction::generate_unique_snapshot_id;
let snapshot_id = generate_unique_snapshot_id(&table);
// ... attach snapshot_id to each StatisticsFile ...
let action = tx.fast_append()
.with_snapshot_id(snapshot_id)
.add_data_files(files);
Mirrors the same pre-allocation pattern RewriteManifestsAction already
uses via the internal `generate_unique_snapshot_id_static`; this just
exposes it through a clean public API + plumbs the override through
FastAppendAction → SnapshotProducer.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Same shape as the FastAppendAction builder from 4a9a5b1. Lets a compaction commit (laminar's merge-on-write) pre-allocate the new snapshot's id so the same transaction can register carry-forward `StatisticsFile` entries under that snapshot — closing the per-snapshot Puffin stats gap where compaction snapshots become the catalog's `current-snapshot-id` without any stats entry, which would otherwise force every reader to walk the parent chain or fall back to a SQL scan. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Same shape as the FastAppendAction (4a9a5b1) and ReplaceDataFilesAction (421ce93) builders. Lets laminar's manifest-compaction maintenance loop pre-allocate the new snapshot's id so the same caller can register a carry-forward StatisticsFile entry under that snapshot — closing the per-snapshot Puffin stats gap on the third commit path. Without this, every manifest compaction (runs every 600s when manifest count > 100) produces a new current-snapshot-id with no statistics-files entry. The executor's `metadata.statistics_for_snapshot(current)` returns None, the label_values fast path falls back to a parquet scan, and stays in the fallback until the chain rolls forward to a FastAppend that re-attaches stats. Both commit paths inside this file (the two-phase `execute()` for concurrent-write resilience, and the simpler `TransactionAction::commit()` fallback) consume the override via `unwrap_or_else`, so the random id generation is preserved when no override is set. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Implements RootManifest types and Parquet serialization for the v4 root manifest that replaces the manifest list layer. Supports three entry types in one Parquet file: manifest references, inline data files, and inline delete files. Includes MDV bitmap column for manifest delete vectors and a to_manifest_list() shim for backward compatibility with existing scan code. 7 tests covering round-trip, projection, MDV bitmap, and shim.
Adds V4 = 4 to FormatVersion enum. All existing match arms treat V4 identically to V3 for now — the V4-specific root manifest commit path will be added in a subsequent commit. This ensures V4 tables can be created and operate through the existing V3 code paths as a baseline.
SnapshotProducer.commit() now dispatches to commit_v4() for V4 tables. The V4 path writes a single root manifest Parquet file containing: - Inline data/delete file entries for new files - Carried-forward manifest references from the previous snapshot - Handles V3→V4 upgrade by converting manifest list entries to refs This reduces commit overhead from 3+ S3 PUTs (manifest + manifest list + metadata.json) to 1 PUT (root manifest) + 1 CAS (metadata pointer). 1076 tests passing, backward compatible with V1-V3 tables.
Adds ManifestDeleteVector struct using roaring bitmaps for soft-deleting entries in child manifests without rewriting them. V4 commit path now handles removed_data_files by: - Removing matching inline entries directly - Building MDV bitmaps for affected child manifest references - Merging with existing MDVs on subsequent compactions This eliminates manifest rewrite amplification during compaction — instead of rewriting N manifest files, only the root manifest is updated with MDV bitmaps marking deleted row indices.
Adds RebalanceRootManifestAction that flushes accumulated inline entries into child manifest files and compacts MDV-heavy manifests: - Inline entries grouped by content type and written as Parquet manifests - Manifest refs with MDV deleted fraction > threshold are rewritten without deleted entries, clearing the MDV - Configurable thresholds: inline_threshold (1000), mdv_compaction (0.3) - No-op when rebalance not needed (inline count below threshold) Available via Transaction::rebalance_root_manifest(). 5 unit tests.
Adds 2 new tests: - mdv_comprehensive: empty, mark, is_deleted, serialize/deserialize round-trip, merge, deleted_fraction, idempotent insert - remove_inline_files: verifies inline entries are removed by path while manifest refs are preserved Total v4-related tests: 14 (7 root manifest + 5 rebalance + 2 new)
Change 1: Cache-forward root manifest entries. commit_v4() accepts pre-cached entries via with_cached_root_entries(), skipping S3 read. ActionCommit returns final entries for the caller to cache. First commit reads from S3, every subsequent commit is zero S3 reads. Change 5: File-to-manifest index for MDV. SnapshotProducer accepts an optional HashMap<file_path, manifest_path> so the MDV block only scans manifests that contain removed files (typically 1-3) instead of all manifests (potentially hundreds). Change 6: Summary updated after MDV. Tracks total_mdv_deleted_files and total_mdv_deleted_rows during the MDV block and inserts them into the snapshot summary so total-data-files reflects logical deletions.
Change 2: Root manifest now uses two row groups instead of a 38-column discriminator schema. Row group 0 = manifest refs (17 cols), row group 1 = inline entries (21 cols). ~45% smaller files, no null column overhead, reader can skip irrelevant row group. Change 3: Adaptive inline→child flush inside commit_v4(). When inline count exceeds root-manifest.inline-threshold (default 500), inline entries are flushed to a child Parquet manifest and replaced with a single manifest ref — all within the same commit. Zero separate table maintenance needed for append-only streaming workloads.
Change 4: to_manifest_list() now computes FieldSummary (min/max partition bounds) from inline data entries using PartitionFieldStats. This enables the query planner to prune inline entries by partition value — for observability tables partitioned by (tenant, hour), a query for the last hour skips entries from other hours. Made PartitionFieldStats pub(super) so root_manifest.rs can reuse the same partition bounds logic that ManifestWriter uses.
- Merge small manifest refs after adaptive flush to keep ref count bounded - Fix cache validation: discard cache on first commit (no current snapshot) - Add row lineage (first_row_id + added_rows) to V4 snapshots - Add V4 fast_append integration test (1 passing, 1 ignored pending FileIO fix)
- Native V4 scan path: inline entries injected directly into scan pipeline as ManifestEntryContext, bypassing ManifestFile::load_manifest - V4-aware duplicate validation checks both manifest refs and inlines - MDV handling on read path: bitmaps threaded through ManifestFileContext, applied during manifest entry streaming to skip soft-deleted rows - Cache validation uses explicit (snapshot_id, entries) tuple instead of deriving from inline entry inspection (TOCTOU fix) - added_rows counts only current-snapshot entries, not carried-forward - RebalanceRootManifestAction sets row_range to prevent serialization crash - read_root_manifest accepts Bytes (zero-copy via Arc clone) - manifest_entries_to_record_batch generic over Borrow<ManifestEntry>, eliminating deep clone of all inline entries on every commit - Object cache weigher uses estimated_size() accounting for heap allocations - Default inline threshold lowered from 500 to 100 - TableMetadataV4 struct with VersionNumber<4> for correct serialization - Removed dead to_manifest_list() shim and unused rebalance variables
…riendly) Adds `Table::effective_format_version()` and a custom table property (`e6.actual-format-version`) that lets a table declare V2/V3 to the catalog wire format while internally taking the V4 commit + rebalance paths. Motivation: catalogs that pre-date V4 (e.g. Lakekeeper at commit bb70173) reject `format-version=4` at CREATE TABLE time: crates/lakekeeper/src/server/tables/create_table.rs:440-444 match v.as_str() { "v1"|"1" => V1, "v2"|"2" => V2, "v3"|"3" => V3, _ => Err("InvalidFormatVersion") } But everything below that gate is opaque to such catalogs: - manifest_list is stored as a plain `String` path -- Lakekeeper never opens the file (grep across its source: zero references to `manifest_list`, `load_manifest`, or `ManifestList`). - Snapshot expiration / drop_table only touch metadata.json; no manifest file walks. - No `deny_unknown_fields` anywhere in lakekeeper's iceberg-rust fork -- unknown table properties round-trip cleanly. - SnapshotV3 already accepts every field our V4 commit produces (manifest_list, summary, schema_id, row_range), so V4 snapshots serialise as valid V3 JSON. So V4's "wire format" against Lakekeeper is identical to V3 plus a single opt-in property; the actual V4 mechanics (Parquet root manifest, MDV bitmaps, single-file commits) live in object storage where the catalog never reaches. The change is intentionally narrow: only the two BEHAVIOUR dispatch sites switch over to `effective_format_version()`: - `SnapshotProducer::commit()` (snapshot.rs:914) -- the "if V4 use commit_v4 else use the manifest-list path" branch. - `RebalanceRootManifestAction::commit()` (rebalance_root_manifest.rs:149) -- the feature gate. All other `metadata().format_version()` call sites stay as-is because they decide MANIFEST-FILE shape (V3 manifest entries are correct for both V3 and V4 tables -- the existing `V3 | V4 => build_v3_data()` match arms remain accurate). Properties: - `e6.actual-format-version` -- only value `"4"` is honoured. Any other value (including `"3"`, `"v4"`, `" 4"`, `""`) falls back to the declared version; never silently downgrades. - When the declared version is already V4, the property is redundant -- legacy tests / file-system catalog use cases continue to work unchanged. Constraints callers must respect: - Never emit `TableUpdate::UpgradeFormatVersion { V4 }` against a V3-declaring catalog; Lakekeeper's enum stops at V3 and it would be rejected. - The opt-in is invisible to non-e6 readers (Trino, Spark via upstream iceberg lib). Those will see `format-version=3`, try to read `manifest_list` as Avro, and fail. Tables using this property MUST only be served by readers that honour `effective_format_version()`. 4 new table-level unit tests cover the precedence rules + bogus value fallback. Full lib suite: 1092 passed, 0 failed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The previous opt-in patch (8f6a35f) wired write-side dispatch through `Table::effective_format_version()` but left the read-side `Snapshot::load_manifest_list` still gating on the catalog-declared `TableMetadata::format_version()`. The net result: a table that declares V3 to a strict catalog (e.g. Lakekeeper pre-V4) and carries `e6.actual-format-version=4` would WRITE a Parquet root manifest on commit (V4 path) and then FAIL on the next scan trying to parse it as Avro (V3 path). Refactor: - Move the precedence rule onto `TableMetadata::effective_format_version` -- the single source of truth. `Table::effective_format_version` becomes a thin delegate. - Move the V4 opt-in constants (`E6_ACTUAL_FORMAT_VERSION_KEY`, `E6_ACTUAL_FORMAT_VERSION_V4_VALUE`) next to the impl in `crate::spec::table_metadata`. - Re-export `E6_ACTUAL_FORMAT_VERSION_KEY` from `crate::table` for the historical import path. - Update `Snapshot::load_manifest_list` to dispatch on `effective_format_version()` -- this is the actual bug fix. Why on TableMetadata vs Table: read paths that hold only a `&TableMetadata` (no `&Table`), like `Snapshot::load_manifest_list`, need to query the effective version without going through the Table wrapper. Putting the impl on TableMetadata makes it accessible to every dispatch site that already holds metadata, which is the natural granularity for format-version decisions. The 4 table-level tests still pass unchanged (they assert the same precedence rule via the Table delegate). Full lib suite: 1092 passed, 0 failed. Existing V4 commit + rebalance unit tests untouched. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…rquet path The V4 inline-overflow code path in commit_v4 templated the child manifest path with `.parquet` extension but invoked the Avro writer (`write_manifest_file()`), producing files named `.parquet` that contain Avro magic bytes (`Obj\x01`). Any downstream reader dispatched by file extension -- iceberg-rust's own Parquet manifest reader (read_parquet_manifest), tessellate's live-set scan, the executor's scan path, the V4 root-manifest load path -- opens them as Parquet and fails with: DataInvalid => Failed to open parquet manifest: Parquet error: Invalid Parquet file. Corrupt footer Reproduced live in the sri-olly stack: 019e9909-5e08-...-m0.parquet (139 KiB) head bytes: 4f62 6a01 → "Obj\x01" (Avro magic, not "PAR1") `RebalanceRootManifestAction` already takes the Parquet path (`write_manifest_file_parquet()`) for both Phase A and Phase B child-manifest writes. The fix is to line commit_v4 up with that convention -- V4 child manifests are unconditionally Parquet, same as V4 root manifests. The .parquet path stays correct. Both data-entry flush (snapshot.rs:1256) and delete-entry flush (:1283) sites updated. 1092 lib tests pass unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…e-entry path
TableScan::plan_files spawned two equivalent consumer tasks for the
manifest-entry channels (delete and data), but the delete one was
written as `spawn(...).await` while the data one was fire-and-forget
`spawn(...)`. Same shape downstream, opposite ownership upstream.
With V4 root manifests carrying inline entries (no child manifests),
plan_files spawns a third task at lines ~381 that sends ALL inline
deletes then ALL inline data on tx clones it OWNS for the duration of
its body. With bounded channels sized to
`concurrency_limit_manifest_files` (executor uses 4), the inline
spawn's `inline_data_tx.send().await` blocks once the data channel
fills past 4 entries -- it's waiting for a consumer.
The data-process spawn would be that consumer, but plan_files is
still stuck on the .await on the delete-process spawn at the line
under change. The delete-process spawn is waiting for the delete
channel to close. The delete channel closes only when
inline_delete_tx drops. inline_delete_tx is held by the inline
spawn, which is blocked waiting for the data consumer. Four-way
deadlock; plan_files hangs forever with no progress and no error.
Live-confirmed on sri-olly's observability.logs table immediately
after a clean reset:
V4 root manifest: 68 inline file entries, 0 child manifests
scan.with_concurrency_limit(4)
-> plan_files start log fires
-> 10+ minutes elapse, no progress, no error
-> executor's discover_files: manifest list stats reports
manifest_count=0 added_files_total=0 (entries() correctly
enumerates only child manifests; inline are a separate set)
Fix: drop the synchronous .await on the delete-process spawn.
Both consumers now run concurrently, matching the data-process
spawn's existing pattern. The inline spawn can drain its data
channel, finish its sends, drop both tx handles, and the delete
channel closes naturally -- terminating the delete consumer on its
own without blocking plan_files.
The .await was likely a copy-paste leftover from an earlier
sequential refactor -- the comment block on the surrounding spawn
already says "in parallel" and the data sibling is correct. Adds a
multi-paragraph code comment to call out the deadlock so future
edits don't reintroduce it.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…deadlock past 32 entries Sibling deadlock to e84ed18 (which removed the outer .await on the delete-process spawn). Same root pattern: `inline_delete_tx` is held across the inline-data send loop and prevents the delete channel from closing, which prevents the DeleteFileIndex from transitioning to Populated, which leaves every `process_data_manifest_entry`'s `into_file_scan_task().await -> DeleteFileIndex::get_deletes_for_data_file` call stuck on `notifier.notified().await` forever. The threshold is silent and exact: deadlock when inline_data_contexts.len() > concurrency_limit_manifest_files + concurrency_limit_manifest_entries Live-reproduced on sri-olly's V4 attribute_index_logs (concurrency=16): 1-hour probe (~23 inline entries, under 32): 115ms total load_index_ms=14 plan_ms=32 collect_ms=0 3-hour probe (66 inline entries, over 32): hangs to the executor's 10s INDEX_PROBE_TIMEOUT ceiling 100% of the time. Every attempt falls back to no-pruning, Grafana times out client-side. Fix is structural, not a knob. The inline spawn now drops inline_delete_tx the moment it finishes sending inline DELETE entries (which is the empty Vec on every append-only table -- the dominant case for V4-on-low-volume tables like attribute_index_*). With the tx gone, the delete channel closes immediately, the DeleteFileIndex populates with an empty set, and every `get_deletes_for_data_file` call returns Vec::new instead of blocking. The inline-data loop then proceeds with the data consumer unblocked. Mixed case (some inline delete entries + many inline data entries): order-preserving — deletes still send first, just don't outlive their loop. The new drop is between the loops, not before the delete loop. Inline deletes are extremely rare in practice (V4 inline-entries are the small-volume tail of fresh commits, which on append-only tables carry no deletes), so this path stays a no-op for the workloads it matters for. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
External tooling that copies parquet row groups verbatim (no decode/ re-encode) needs to construct iceberg::spec::DataFile entries purely from the output file's parquet footer. Tessellate's planned streaming row-group concat path is the immediate consumer — it eliminates the ~1 GB-per-file Arrow decode that today's compact_batch hits, allowing cycles to complete inside their k8s deadline. The function body is unchanged; only the visibility moves from pub(crate) to pub. The doc comment now references the consumer. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Per spec, multiple StatisticsFile entries may share the same snapshot_id as long as their statistics_path differs. The prior storage `HashMap<i64, StatisticsFile>` collapsed every second-and- later set_statistics() call for the same snapshot to last-write-wins (the index_statistics .rev() trick made it first-wins, same problem), silently dropping N-1 of every N entries before they ever reached the catalog. The b63983e action-layer fix changed action storage to Vec but TableMetadata still collapsed — the fix was incomplete and got reverted on main (cc16218) without diagnosis. This corrects it where the collapse actually lived. Storage: `HashMap<(i64, String), StatisticsFile>` keyed on the full spec key. Set N times with same snapshot_id but distinct paths → keeps all N. Set N times with the SAME (snapshot_id, path) → still upserts (correct: that's a true update). API: - `statistics_for_snapshot(snapshot_id) -> Option<&StatisticsFile>` kept for back-compat: returns the first match in HashMap iteration order (non-deterministic). Downstream code that only ever expected one entry continues to compile and read SOMETHING. - New `all_statistics_for_snapshot(snapshot_id) -> Vec<&StatisticsFile>` returns the full per-spec set. Callers (laminar's per-output merge sidecars, tessellate's future per-partition puffin path) should migrate to this. Builder: - `set_statistics`: inserts on composite key — append-not-overwrite for distinct paths, true update for same path. - `remove_statistics(snapshot_id)`: still snapshot-id-keyed (mirrors catalog protocol: RemoveStatistics is keyed by snapshot_id alone). Drops every entry matching the id via `retain`. Emits exactly one TableUpdate::RemoveStatistics iff anything was removed. Wire JSON unchanged: iceberg spec already represents `statistics` as a list. Old/new readers interop on JSON; only internal Rust storage shape changed. Tests: existing test_statistics (single-entry case) updated for new key shape; new test_set_multiple_statistics_same_snapshot_different_paths locks in the three-distinct-paths regression that motivated the fix. All 1092 iceberg lib tests pass. This is Phase 1 of B2 (multi-stats per snapshot end-to-end). Phase 2: Lakekeeper PG migration to add statistics_path to the table_statistics PK. Phase 3: Lakekeeper code rebuild. Phase 4: downstream rebuilds. Phase 5: tessellate refactor to write per- partition puffin during compact_batch instead of accumulating blobs in memory (closes the OOM root cause). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…lumbing
opendal 0.57 ships a native `credential_provider_chain` for S3 that
covers IRSA, EKS Pod Identity, EC2 instance metadata, env vars, and
shared-credentials files with auto-refresh — the same responsibility
our hand-rolled `CustomAwsCredentialLoader` / `customized_credential_load`
extension was filling under opendal 0.55. The chain is now built-in, so
this iceberg-rust hook is dead weight and is removed.
Changes in this commit:
* Workspace `opendal = "0.55.0"` → `"0.57.0"`. The `Scheme` enum was
removed in 0.57 (services moved to per-crate `opendal-service-*`
modules with type-erased Configurator dispatch). `Storage::build`
used `Scheme::S3`/`Scheme::Azdls`/... to dispatch construction;
replace with a normalized scheme-string match on `scheme_str`.
Aliases (`s3`/`s3a`, `abfs[s]`/`wasb[s]`, `gs`/`gcs`) are folded
into the normalizer.
* Delete `CustomAwsCredentialLoader` (the iceberg-rust wrapper that
re-exposed `reqsign::AwsCredentialLoad`), the `Storage::S3
{ customized_credential_load }` field, the `s3_config_build`
parameter, and the `with_file_io_extension`-fed `extensions.get`
lookup. Downstream consumers (executor) must drop their
`FileIOBuilder::with_file_io_extension(CustomAwsCredentialLoader)`
call and rely on opendal-native auth.
* Drop the optional `reqsign` direct dep on the `iceberg` crate
(`storage-s3 = ["opendal/services-s3", "reqsign"]` → just
`["opendal/services-s3"]`). reqsign was only here to materialize
`AwsCredential`/`AwsCredentialLoad` for the custom loader; with
that loader gone, opendal carries reqsign internally and we don't
surface it.
Side-effects:
* Storage-azdls (used by laminar/executor/tessellate on AKS) now
compiles cleanly. The reqsign 0.16.5 federated-token `expires_on`
parse panic (`parse 1782137016 into rfc3339 failed`) seen on
the first ADLS write was an opendal-0.55-internal bug; opendal
0.57's azdls signer doesn't go through that path.
* The `opendal::services::S3Config::allow_anonymous` field is
deprecated in 0.57 (`skip_signature` is the replacement). Left
in place for now — there's a separate cleanup pass for the
`is_truthy` props bridge.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
opendal-service-azdls 0.57 builds a credential chain whose StaticEnv only carries the explicitly-set adls.* config keys (client_id, tenant_id, authority_host) — AZURE_FEDERATED_TOKEN_FILE is dropped on the floor, so reqsign's WorkloadIdentityCredentialProvider returns None on AKS and ADLS writes 403 AuthorizationPermissionMismatch (the IMDS fallback picks up the node-VM identity, wrong principal). Rather than fork opendal, run the federated → AAD exchange ourselves and wrap the operator's HttpClient via the public update_http_client hook on AccessorInfo. The wrapper caches the token until expiry minus 120s and only injects Authorization when the request doesn't already carry one, so static SAS / shared-key paths are untouched. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
reqsign's DefaultCredentialProvider chain runs IMDS before our wrap sees the request — on AKS that mints a token for the *node-VM* identity (wrong principal) and sets Authorization. The previous "if absent" check left that token in place, so writes kept failing 403 AuthorizationPermissionMismatch. Replace unconditionally instead; within the WI gate it's always the right thing to do. Also log at storage construction so we can see the WI path activate. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ReplaceDataFiles carries a fixed delete-file list built at action creation time. When Transaction::commit retries on OCC rejection, it reloads the table but replays the original delete list against the new base. If another writer already replaced those files, the retry succeeds with stale references, causing both merged files to coexist in the live manifest (duplicate data). Add a disable_retry flag to Transaction, set automatically when ReplaceDataFilesAction is applied. The retry predicate checks this flag and skips retry, returning the OCC error to the caller. The caller (e.g. Laminar's merge-on-write) handles the failure by dropping the merge — the original small files remain in the table (committed by FastAppend), so no data is lost. Tested: 4 concurrent replace_data_files against MemoryCatalog — 1 succeeds, 3 correctly rejected.
update_snapshot_summaries was looking up self.snapshot_id (the new snapshot being created) in the metadata to find the parent summary. Since the new snapshot doesn't exist yet, the lookup returned None, and cumulative fields (total-records, total-data-files, etc.) were computed without a previous base — always showing only the current commit's counts instead of accumulating. Fix: use table_metadata.current_snapshot() directly as the previous snapshot, since that's the latest committed state.
Tests FastAppend contention, ReplaceDataFiles race, retry behavior, and two-replica ingest simulation against MemoryCatalog. Verifies no data loss and no duplicates under concurrent commits.
![github-actions[bot]](/s/com/githubusercontent/avatars/G.https/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
license-eye has checked 390 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 319 | 2 | 69 | 0 |
Click to see the invalid file list
- PARQUET_MANIFESTS.md
- crates/iceberg/tests/occ_concurrency_test.rs
Use this command to fix any missing license headers
```bash
docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix
</details>
| @@ -0,0 +1,665 @@ | |||
| //! Iceberg OCC Concurrency Test | |||
There was a problem hiding this comment.
Suggested change
| //! Iceberg OCC Concurrency Test | |
| // Licensed to the Apache Software Foundation (ASF) under one | |
| // or more contributor license agreements. See the NOTICE file | |
| // distributed with this work for additional information | |
| // regarding copyright ownership. The ASF licenses this file | |
| // to you under the Apache License, Version 2.0 (the | |
| // "License"); you may not use this file except in compliance | |
| // with the License. You may obtain a copy of the License at | |
| // | |
| // http://www.apache.org/licenses/LICENSE-2.0 | |
| // | |
| // Unless required by applicable law or agreed to in writing, | |
| // software distributed under the License is distributed on an | |
| // "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | |
| // KIND, either express or implied. See the License for the | |
| // specific language governing permissions and limitations | |
| // under the License. | |
| //! Iceberg OCC Concurrency Test |
…ailures Add a FaultyCatalog decorator that injects update_table (register) failures, and three real-code fault tests driving the actual commit/scan path. In the real commit, an action WRITES its data/manifest files through FileIO and only then REGISTERS the snapshot via the catalog; failing the register models the crash between write and register (the sri-olly orphan-file incident family). Injected errors are non-retryable, so they surface as a failed commit. - dst_fault_commit_register_failure_is_atomic: a faulted append fails the commit, the table is unchanged (the file never becomes reachable), and a fresh retry after the fault clears lands the data with no loss/dup. - dst_fault_replace_register_failure_preserves_data: a faulted replace does not half-apply — nothing deleted/added; a clean re-run then yields the compacted set. - dst_fault_seeded_commit_crash_no_loss_no_phantom: seeded schedule that randomly crashes the register before commits; the reachable set always equals exactly the successfully-committed set — no lost commit, no crashed write made reachable. Deterministic, replayable, the real-code counterpart of the M1 over-deletion sim. 13 dst_ real-code tests green; full transaction suite green (94).
Complete the M3 real-code fault dimension over the actual commit/scan path. Transient (retryable) faults — extend FaultyCatalog with a retryable update_table fault mode and exercise the production backon retry loop (fast table props so the tests don't sleep): - dst_fault_transient_retryable_append_recovers: two retryable register faults are absorbed by the retry loop; the append lands (>=3 register attempts observed). - dst_fault_transient_retryable_replace_not_retried: disable_retry suppresses retry even for a retryable error on a replace — exactly one attempt, commit fails, table untouched. Ties the disable_retry wiring to fault behavior. - dst_fault_seeded_transient_no_loss: seeded storm of 0..=2 transient faults before each append; every commit still lands, reachable == cumulative, no loss/dup. Clock skew — dst_clockskew_ordering_and_tolerance drives the real MetadataBuilder::add_snapshot guards with synthetic snapshots: gross backwards skew (>1min) is rejected, small skew (<1min) tolerated, and a non-increasing sequence number is rejected regardless of timestamp (history is ordered by sequence, not wall clock — the skew-immune invariant). OCC conflict detection is snapshot-id based (RefSnapshotIdMatch), already covered by the conflicting-replace tests. 17 dst_ real-code tests green; full transaction suite green (98).
Follow-up to the collapse instrumentation, which did its job by
eliminating its own hypothesis. Measured live on v1.214:
35 v4 commits, ALL do_delta=true, chain_depth 29-36 (MAX_CHAIN=64)
ZERO collapses — recon_ms never logged
actions_ms n=82 p50=112ms max=30,534ms sum=444.9s
26 commits >2s carry 98% of it (436.6s of 444.9s)
retry=false on 25 of those 26; update_table_ms 2-3ms
So it is not the 1-in-64 chain collapse (never fired), not OCC
contention (no retries), and not the catalog (2-3ms). The cost is a
single action's apply() on the DELTA path — per_action_ms=[30534, 0] —
and it is strikingly uniform: logs 27-30s every time, metrics 15-16s.
That uniformity says fixed-size work per commit, not a race.
What remains on that path is the inline flush: grouping inline entries
by (content type, partition_spec_id), sub-grouping by the manifest
grouping key, and writing one child manifest per group to S3. This times
that block and counts the manifests written:
v4 delta flush: inline_entries=.. data_manifests=.. delete_manifests=..
flush_ms=.. tiered=..
If flush_ms accounts for the 27-30s, the cost is manifest writes per
commit and the fix is fewer, larger commits (batching). If it does not,
the time is elsewhere in apply() and this narrows it further.
Note `tiered` forces this block whenever inline_count > 0, bypassing the
inline_threshold/flush_bytes/flush_entries guards that apply to
non-tiered tables — worth checking against the numbers.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Add a deterministic opendal fault Layer (io/fault_layer.rs) that wraps the object store below iceberg's FileIO, so faults hit the RAW read/write/delete the commit and scan paths perform — not just the catalog register step the FaultyCatalog decorator can reach. A FaultController with exhaustible counters drives it; two test-only hooks wire it in: FileIO::memory_with_faults and MemoryCatalog::new_with_file_io (the catalog shares one FileIO with its tables, so the layer covers both metadata and manifest/data ops). opendal errors map to non-retryable iceberg errors, so a byte fault fails the commit outright. Three real-code tests: - dst_bytefault_write_failure_is_atomic: a manifest write failing mid-commit fails the commit, the table is unchanged (no half-written snapshot reachable), and a clean retry lands the data. Deeper than the catalog test — fails the store write, before the register. - dst_bytefault_read_failure_scan_errors_not_silent: a manifest read fault during scan planning surfaces as a scan ERROR, never as silently fewer rows (the most dangerous fault class — a read blip must not look like data loss). - dst_bytefault_seeded_write_crash_no_loss: seeded write-crash schedule; every faulted commit fails, reachable set always == successfully-committed set. 20 dst_ real-code tests green; full transaction (101) + memory catalog (62) suites green.
The delta path turns out to be narrower than assumed — on do_delta=true
with incremental=true, three candidate costs are all skipped:
* MDV scan — gated on !incremental
* collapse/reconstruct — gated on !do_delta
* merge_manifests_if_needed — gated on !do_delta
leaving only the inline flush, which metrics_1m also runs while staying
fast. So the cost must be in what the commit CARRIES, not what it does.
The comparison that points at removals: same commit_v4, same tables,
same cluster —
table laminar p50 tessellate p50 ratio cold leaves
logs 112 ms 3,970 ms 35x 499
metrics 114 ms 3,168 ms 28x 335
metrics_1m 104 ms 107 ms 1.0x 0
metrics_1m is IDENTICAL for both, so nothing about tessellate as a
process is slow. Laminar commits to logs/metrics too — same roots, same
leaf counts — at 112 ms, so leaf count alone is not sufficient either.
The structural difference is that laminar fast-appends while tessellate
replace_data_files removes many files per commit, and on an incremental
tiered root every removal becomes a carried-forward path tombstone.
Adds:
v4 removal tombstones: carried=.. new_removals=.. matched_inline=..
total=.. sweep_min=..
v4 elapsed at flush end: v4_ms=..
`carried` is the size inherited from the previous commit — the number
that grows until a sweep retires it, and the one to watch against
actions_ms. `v4_ms` bounds how much of the commit the flush accounts
for. Precedent for unbounded growth: this set previously reached 430k
paths / 108 MB of a 132 MB root.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ayer Extend the object-store fault layer with the two remaining fault dimensions: - Latency: per-operation async delay (set_latency_ms) applied before every raw read/write/delete below FileIO. - Network partition: partition()/heal() — while partitioned, every read and write fails (the store is unreachable). Two real-code tests: - dst_bytefault_partition_then_heal: during a partition a commit cannot land and a cold scan cannot read; the table is unchanged; after heal() both commit and scan recover with no loss/phantom. Models an S3 partition + recovery. - dst_bytefault_latency_preserves_correctness: a full append + compaction under injected store latency yields exactly the same live set (guards against timing races), with reads/writes confirmed to traverse the layer. 22 dst_ real-code tests green.
The previous timer started AFTER the root-load block and reported
v4_ms p50=34ms / max=59ms while actions_ms reached 33,075ms — it was
measuring 1% of the commit and missing the rest. Same class of mistake as
placing the first counters in existing_manifest: instrumenting a spot
without confirming it covers the cost.
Measured on v1.217 (55 commits, past-grace tick):
carried=0 on EVERY commit -> tombstone accumulation DISPROVEN
inline_entries=1, data_manifests=1, flush_ms p50=34ms
-> manifest writing DISPROVEN
v4_ms sum 1.95s vs actions_ms sum 196.75s
-> commit_v4 (as timed) is 1% of the cost
`actions_ms` wraps `TransactionAction::commit()` (mod.rs:325), of which
commit_v4 is the tail. The only substantial work between the two is this
block: reading and parsing the current root manifest from S3.
Adds `v4 root load: entries=.. chain_depth=.. root_load_ms=..` and
`v4_total_ms` alongside the old partial `v4_ms`, so the split between
root-load and everything after is explicit rather than inferred.
Four hypotheses now eliminated by measurement: V2/V3 manifest walk (wrong
sub-path), chain collapse (never fires, do_delta=true on all commits),
OCC contention (retry=false on 25 of 26 slow commits), tombstone
accumulation (carried=0).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Instruments the DISPATCH POINT rather than one suspected action, which covers every phase at once — replace_data_files, compact_cold_tier, graduate_buckets, rebalance_root_manifest, drop_cold_buckets, fast_append, update_statistics and the rest. Why: `per_action_ms` is positional, so a 36s entry could not be attributed to an action without inference. Inference sent instrumentation to the wrong place three times, each costing a fork -> tessellate -> build -> deploy cycle: 1. counters in existing_manifest — V4 returns early, never called 2. timer inside commit_v4 but placed after the root-load block 3. root-load timing itself — 26-42ms, not the cost The measurement that forced this: every commit_v4 invocation in the tick ran 61-96ms end to end (root_load 26-42ms, flush 33ms), while single-action commits in the SAME tick reached 36s and 22 of 64 commits carried 186s of 193s. So the expensive action was never the one being instrumented. Adds `fn action_name()` to the TransactionAction trait (defaulted to "unknown" so no implementor is forced to change), overridden in all 14 real actions, plus: slow action: name=.. ms=.. table=.. action_idx=.. of .. (>2s only) commit sub-steps: ... action_names=[..] (every commit) 2s threshold sits well above the ~100ms p50 and low enough to catch everything that matters. Cost is one &'static str per action per commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Naming the actions found the real cost: drop_cold_buckets (Phase 7b TTL) was the single most expensive action in the tick — 98.1s across 3 invocations of 203.1s total actions_ms, ahead of compact_cold_tier (50.9s), rebalance_root_manifest (28.3s) and graduate_buckets (17.4s). `replace_data_files`, which six earlier rounds of instrumentation targeted, does not appear in the slow list at all. The mechanism is a plain sequential scan: every cold leaf manifest is fetched from S3, one at a time, purely to read ONE number per leaf (max event-time) for the keep-vs-drop decision. Nothing batched, nothing concurrent, no cache consulted. The arithmetic matches the measurements almost exactly at ~35ms per sequential GET: logs 1282 leaves -> 45.4s metrics_1m 788 leaves -> 37.2s metrics 550 leaves -> 15.5s So TTL cost is strictly O(cold leaves) per run. That explains why it grows as leaves accumulate (nothing merges them), why laminar is unaffected on the same tables (it never runs this action), and why metrics_1m became slow again after its rebuild (it regenerated leaves). Logs `drop_cold_buckets scan: leaves=.. kept=.. dropped=.. load_ms=.. scan_ms=.. mean_load_ms=..` to confirm per-GET latency before changing behaviour. Fix to follow: graduate_buckets already maintains a max-ts SIDECAR for exactly this decision (see its sidecar_hits/sidecar_misses); this path does not consult it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…g every leaf Phase 7b TTL was the most expensive action in the tick — 98.1s of 203.1s actions_ms — because it fetched EVERY cold leaf manifest from S3, one at a time, purely to read one number per leaf (max event-time) for the keep-vs-drop decision. Nothing batched, nothing concurrent, no cache. Cost was strictly O(cold leaves), and the arithmetic matched at ~35ms per sequential GET: logs 1282 leaves -> 45.4s metrics_1m 788 leaves -> 37.2s metrics 550 leaves -> 15.5s graduate_buckets already maintains a max-ts sidecar keyed by leaf manifest path for exactly this decision; this path simply never consulted it. Now it loads the sidecar once and only pays a manifest GET on a miss, turning N sequential GETs into 1 read plus the misses. Behaviour is unchanged by construction. A miss — absent sidecar, stale ts_field_id, wrong format version, or a cached `None` — falls through to the original manifest load. `sidecar_hit` is what refuses to trust a cached non-answer; without it an all-None sidecar would permanently suppress TTL on tables whose retention field is not the partition source (logs on ingestion_time is exactly that case). Extracted `keep_leaf` so the fast path and the fallback provably apply the same rule rather than duplicating it — that equivalence IS the correctness claim here. 3 tests: cutoff boundary is inclusive-keep, absent max-ts keeps (dropping on missing data would be unrecoverable; keeping costs one more pass), and extremes do not wrap. Also logs sidecar_hits / sidecar_misses / manifest_loads so the hit rate is visible rather than assumed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… work Rebalance consolidates CLOSED-hour leaves. That work is disjoint from laminar's hot appends, and the outputs are fresh UUID-addressed S3 objects — the old manifests are not removed until the root swap. So a delayed or lost CAS does not invalidate them; only the small root write has to be redone. Yet every retry re-loaded and re-wrote every manifest. This is the same reasoning `compact_cold_tier` already documents for its `PreparedCompaction`: "the heavy S3-resident outputs are UUID-addressed and safe to reuse across CAS retries. The only per-retry work is writing the small delta root that points at them." replace_data_files has `cached_manifests` for the same purpose. Rebalance had neither. Caches Phase-A outputs keyed by (source manifest path, source MDV bytes). The MDV is part of the key because a concurrent writer tombstoning more rows in that leaf makes the cached rewrite stale. Keyed rather than all-or-nothing so one changed leaf does not discard the other N-1. Why it matters: rebalance is now the top cost after the TTL fix — 24 invocations, 90.0s per tick, mean 3.75s, capped at max_manifests_per_commit=10. That cap exists precisely because retries were unbounded redo work; its own field doc says it bounds "retry state ... not the total". With reuse the cap bounds only the FIRST pass, which is the precondition for raising it. Cache hits still consume the cap, deliberately: a retry then reproduces the same commit content rather than racing ahead to different manifests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…able Adds `rebalance phase A: rewrites_done=.. cache_hits=.. cap=.. entries_in_root=..`. Without this the memoisation is unobservable — I added the counter and forgot the log, which would have left us raising TESSELLATE_V2_REBALANCE_MAX_MANIFESTS_PER_COMMIT on faith. Two readings both argue for raising the cap, for opposite reasons: high cache_hits means retry redo is now cheap, so the thing the cap bounds no longer costs; near-zero retries means there was little redo to bound in the first place. Low hits WITH frequent retries would be the one signal to leave it alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…fold Cold leaves are the term that sets tessellate's tick cost: every cold-tier pass is O(leaves), and `compact_cold_tier` loads each leaf manifest at prep (264s of a 294s tick). On sri-olly they grew unbounded — logs 1,282 → 1,615 in a day, `nodes_moved=719` in a single tick. Two independent generators. 1. Graduation imported the HOT tier's manifest granularity into cold. Moving each closed node across by reference is O(1), but laminar writes one node per ~15-30s commit, so an hour arrives as 120-240 nodes and lands as that many leaves — the cold tier ends up shaped by how often the writer committed rather than by the data. `GraduateBucketsAction::with_leaf_fold` re-clusters graduating nodes (and any closed inline files, in the same write) by partition tuple instead: one leaf per partition, which is what the tiered design documents. Reads are parallel and live in `prepare()`, so they are paid once and reused across CAS retries. Nodes beyond the batch bound stay HOT and fold on a later pass — nothing graduates unfolded. Opt-in per writer; `commit_v4`'s hot-path collapse keeps the by-reference move. The fold subsumes `plan_graduated_node`: it reads every node anyway, so `fold_surviving_entries` applies the same pending-removal filter that keeps merged-away files out of cold (the 2026-07-16 dangling-ref bug). 2. `merge_manifests_if_needed` — the only thing that consolidates root refs — runs only on a collapse, and the collapse never fired. Tessellate's `rebalance_root_manifest` writes a fresh base (`chain_depth: 0`) every ~10 min, so the chain never approached 64: observed 1→36 within a tick, `do_delta=true` on 36 of 36 commits, `entries_in_root=2171`. The counter carries two unrelated concerns — read-chain depth (universal) and consolidation cadence (a per-writer choice) — so make the cap per-writer via `ICEBERG_ROOT_MANIFEST_MAX_CHAIN`, default 64. A maintenance writer can set it low and collapse on its own cadence; laminar keeps 64 and its O(1) delta. Shared by the three actions that must agree on the cap. Tests: e2e proves the fold turns 7 leaves into 1 with all 8 files intact and leaves existing cold leaves untouched (it bounds the rate, not the stock); units pin the removal filter, the tighter-of-two cap, opt-in/0-disables, and the cap-parse precedence including 0 → default. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… spec The fold rewrites entries under the table's CURRENT default spec. A node written under an older spec would come back relabelled — silently, since nothing downstream re-derives partition values. Needs a spec evolution to trigger, which is exactly why it should be structural rather than assumed away. Such nodes keep the by-reference move, like delete-content manifests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rest in parallel
Prep loaded EVERY cold-leaf manifest, serially, to answer one question per
leaf: does it hold any path in `removed`? Leaves answering "no" — 6,697 of
6,698 — were re-emitted as the very `ManifestFile` ref `read_bucket_index`
had already returned, so the fetch was pure waste. That made prep scale with
the whole cold tier: sri-olly metrics_1m 6,698 leaves × ~26ms/GET = the
measured 175.4s of a 294.4s tick (logs 2,021 → 61s, metrics 611 → 17-23s;
one constant fits all three).
A compaction never crosses partitions — merged outputs live in the same
partitions as the inputs they replace — and cold leaves are written
partition-tight. So a leaf whose summary pins it to some OTHER partition
provably holds no removed path, and that is decided from the bucket-index
alone with no S3 read. The module doc has carried this as a standing
follow-up ("target leaves by partition or a file→leaf index") since v1.
`leaf_may_hold_partitions` answers "no" only when it can PROVE a mismatch:
tight summary on every field (lower == upper) and no target matching. No
targets, absent summary, arity mismatch, a wide field, an unencodable value
⇒ load it. Comparison is byte equality against `Datum::to_bytes` — the same
encoding the manifest writer used — never an ordering test, since byte order
does not track value order for signed integers.
Whatever survives the prune now loads at 32-way concurrency, so the
conservative fallbacks stay fast too. `buffered` not `_unordered`: leaf order
determines the resulting bucket-index and determinism is worth the
head-of-line wait. New `compact_cold_tier leaf scan` line reports
leaves/pruned/loaded/scan_ms so the prune's hit rate is measurable.
Tests: a tight leaf in another partition is pruned; every not-knowing case
loads (pruning on a guess would silently drop files from the rewrite); any
one matching target keeps the leaf, since a batched swap spans partitions.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Folding re-clusters graduating nodes by partition, so it can only help when
several nodes SHARE a partition. Live, the split is stark and per-table:
metrics_1m 256 nodes -> 14 leaves (18x; rollup emits per checkpoint)
logs 80 nodes -> 80 leaves (nothing; Phase 6 already compacted
each partition-hour to ~1 file)
On logs it was pure cost — 2,860 / 3,080 / 2,544 ms of manifest rewriting
across three consecutive ticks for zero reduction, and it put
`graduate_buckets` into the slow-action list where it had never appeared.
The benefit is knowable for FREE. Partition-tight manifests advertise their
partition in the root/bucket-index, so `distinct_summary_partitions` counts
distinct partitions with no manifest loads and no S3 — the same observation
`compact_cold_tier`'s prune rests on. Fold only on a >=2x expected reduction.
Gated on the DATA, not on a table allowlist: the "logs doesn't benefit" fact
is a property of current ingest shape, and would go stale the moment that
changes — exactly how laminar's "small fan-in, serial reads are fine"
assumption expired once the batch threshold rose. Unknowable summaries (wide
or absent) still fold, preserving prior behaviour rather than silently
skipping work.
Tests pin both live shapes (80->80 must not fold, 256->14 must), the exact
2x boundary, key-collision safety across concatenated fields, and that
wide/absent summaries fall back to folding.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… cause
sri-olly logs carries 4,280 cold leaves against ~50 live data files across 45
partitions (census via count-files-per-partition) — ~85 leaves per live file.
Leaf count tracks commit history, not data.
Two candidate causes need different fixes and cannot be told apart from the
outside:
* leaves hold nothing live -> the APPEND path must validate. Graduation
already has the check (`GraduatedNodePlan::Skip` drops a fully-orphaned
node) but it is gated behind `!removed_paths.is_empty()`, and rebalance
Phase C sweeps removed_paths into MDVs — so the blind
`cold_leaves.extend(graduated_nodes)` branch is the COMMON path.
* leaves hold data -> a genuine per-partition multiplier, and the fix is
merging same-partition leaves instead.
`empty_leaves` / `entries_seen` ride the load prep already performs, so this
costs nothing and answers it on the next tick that compacts.
Three explanations for this table have already been corrected by measurement
today; not picking the fourth by reasoning.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
sri-olly logs: 4,280 cold leaves against ~50 live data files across 45
partitions. Leaf count tracks commit history, not data. Four candidate
mechanisms, none distinguishable from outside the process, so measure all of
them in one tick instead of guessing a fourth time:
compact_cold_tier leaf scan += empty_leaves, entries_seen, alive_entries,
index_partitions
empty_leaves -> are the leaves dead, or do they hold data?
index_partitions -> leaves-per-partition for the WHOLE index,
derived from summaries (no loads), which is
what decides "drop" vs "merge" as the fix
graduation append -> which branch runs. The blind
`cold_leaves.extend(graduated_nodes)` has
no liveness check; validation exists only
in the `!removed_paths.is_empty()` branch,
and Phase C sweeps removed_paths into MDVs,
so the blind path is likely the common one
graduation append plan -> how often Skip actually fires when the
validated branch DOES run
ttl cold-leaf prune none_ts= -> already present: leaves with no resolvable
max-ts are never expired, i.e. immortal
All of it rides loads already performed; no added I/O.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`loaded=191` was being read as "191 leaves in the target partition". It is not. The prune keeps a leaf for either of two very different reasons: it matches a target, OR its summary is too wide to prove it does not. Only the total was reported, so an unprunable population read as a per-partition multiplier — a different diagnosis with a different fix. The distinction matters because wide cold leaves have a known origin: graduation moves laminar's child manifests into cold BY REFERENCE, and laminar's flush groups on [timestamp_hour] only (the smart default), so those manifests are hour-tight but tenant-WIDE. Moved unchanged, they become cold leaves the prune structurally cannot skip. If loaded_unprunable dominates, `write.manifest.grouping-fields` is not only a rebalance-CPU question — it is what makes the cold tier prunable at all. Corroborating evidence already in hand: the table reporting `index_partitions=None` (⇒ at least one non-tight leaf) loads 191 of 791, while the table reporting `Some(42)` (⇒ every leaf tight) loads 2 of 217. Also confirms what this ISN'T: empty_leaves=0 and alive_entries == entries_seen on every scan, and `graduation append plan: skipped_orphaned=0 materialized=0` with mode=validated — the leaves hold live data and the liveness check already runs, so neither dropping dead leaves nor ungating that check would reclaim anything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e index `index_partitions` collapsed to None if ANY leaf was non-tight, so on the one table where the answer mattered it reported nothing. Report the population instead: `index_wide` (leaves structurally unprunable) and `index_partitions_tight` (distinct partitions among the rest). `index_wide` is the number that decides where the fix belongs. Tightness is what makes a leaf prunable AT ALL, independent of any target — a wide leaf is loaded on every compaction of every partition, forever. Cold leaves written by `write_entries_clustered` are tight by construction; leaves that arrived by REFERENCE from a hot manifest are only as tight as that manifest was, and laminar's flush groups on [timestamp_hour] alone by default: hour-tight, tenant-wide. So if index_wide dominates, the fix is at the WRITE side (`write.manifest.grouping-fields`, §11) and it is not merely a rebalance-CPU question — it is what makes the cold tier prunable. If index_wide is small, the loaded leaves genuinely share a partition and the fix is merging. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…are already loaded
A wide leaf — partition summary spanning more than one value — can never be
proven irrelevant, so it is fetched on EVERY compaction of EVERY partition,
forever. Measured on sri-olly:
logs leaves=4301 loaded=354 loaded_matched=2 loaded_unprunable=352
index_wide=352 scan_ms=569
metrics leaves=840 loaded=191 loaded_matched=2 loaded_unprunable=189
index_wide=189 scan_ms=349
metrics_1m leaves=292 loaded=2 loaded_matched=2 loaded_unprunable=0
index_wide=0 scan_ms=25 <- control, freshly reset
`loaded_unprunable` equals `index_wide` exactly on every table: the wide
population IS the scan cost. And `loaded_matched=2` everywhere — the target
partition holds 2 leaves, not the 191/354 the earlier single `loaded` total
was mis-read as. Per-partition duplication was never the problem.
Fixed where the leaf is already in memory: a wide, unaffected leaf is
re-emitted through the same partition-scoped clustered write the survivors
take, so it comes back tight and leaves every future scan permanently. Cost
is one write on a read already paid for.
Deliberately NOT a gate at graduation. Wideness is a metadata shape, not a
correctness problem — a wide leaf returns correct results, it just cannot be
skipped — so there is no reason to pay for it synchronously at a boundary,
and every reason not to: this system has already deadlocked once on a
graduation precondition (readiness check -> hot root ~14k).
Bounded twice so it cannot create work of its own: only runs when the commit
is happening regardless (`will_commit`), and caps leaves per commit. The
remainder carry forward and get their turn later, so the backlog drains
incrementally. At 32/commit and ~10 commits/tick, logs' 352 clear in about a
tick.
Tests pin that tightening never manufactures a commit, respects the cap, and
that every candidate is absorbed XOR carried — absorbing and carrying would
duplicate rows into the bucket-index, doing neither would drop them.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The first version of this test PASSED while proving nothing — its fixture
never produced a wide leaf, so it exercised the unchanged path. A vacuity
guard (`cold_wide_leaf_count > 0` before acting) caught that, and two fixture
bugs had to be fixed before the wide path was reached at all:
* inline entries are materialised by graduation through the partition-scoped
clustered write, so they arrive TIGHT. The wide case only exists for files
that reached a CHILD MANIFEST while hot and were then moved across by
reference — so the table needs `root-manifest.inline-threshold` low enough
to flush.
* more importantly, a tiered table runs a collapse-fold ON COMMIT, and
anything already past the bucket window graduates right there, inline and
tight. With the old `ts=1000` the append graduated everything itself
(`post-append root: entries=0`), leaving the explicit graduation a no-op.
The files must stay HOT through the append (`i64::MAX / 4`) and be closed
only by the graduation cutoff.
With both fixed the fixture produces the real thing — a child manifest with
partition bounds 0..5, `tight=false`, graduating by reference into one wide
cold leaf — and the test asserts what actually matters: every data file
survives tightening except the one deliberately compacted away, and no wide
leaf remains.
Also makes the per-commit cap tunable
(`ICEBERG_COLD_TIGHTEN_MAX_LEAVES_PER_COMMIT`, default raised 32 -> 128, `0`
disables). 32 was a guess. The real bound is that `write_entries_clustered`
emits one manifest per partition SEQUENTIALLY, so absorbing leaves spanning P
partitions costs P serial PUTs — logs' 352 wide leaves span ~210 partitions.
Note this is write-only work (the load is already paid), unlike rebalance's
`max_manifests_per_commit=100` which bounds load+write, so this can safely run
higher once that write is parallelised.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The fold batch bound was merged with the caller's graduation cap via
`effective_graduation_cap` (tighter of the two). The standalone graduation
action passes `max_graduate: None` — unbounded — so the fold's default of 256
silently became a 256-nodes-per-tick GRADUATION cap.
Measured on sri-olly: two of three tables graduated exactly 256 nodes every
tick, pinned at the bound. At ~6 ticks/hour that is ~1,536 nodes/hour, below
what ingest produces, so the cold tier fell ~5 hours behind the graduate
cutoff. Phase 6b's sweep window tracks that same cutoff (`[cutoff-5, cutoff]`),
so it never overlapped the newest cold data — newest cold leaf covered hour
496332 against a window starting at 496333 — and the sweep walked 0 of 13,099
leaves while compacting nothing.
The justification in the original comment ("nothing crosses into cold
unfolded") was invented. Folding is an optimisation; moving a node across BY
REFERENCE is the pre-existing, correct behaviour, and is what the non-folding
path has always done. There was never a reason to hold a node hot rather than
graduate it unfolded.
Now `fold_batch_size` bounds only how many graduating nodes get folded; the
remainder graduate by reference in the SAME pass. Graduation is bounded solely
by `max_graduate`, which is `None` for the standalone action — restoring the
pre-fold behaviour.
The test that asserted the merged bound is replaced, and renamed from
`graduation_cap_takes_the_tighter_bound` to
`fold_batch_bounds_folding_not_graduation` — a test whose NAME asserts the
invariant that caused the bug is worse than no test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A graduation delta writes NO entries (`entries_to_write = &[]`), so a
path-tombstone is its ONLY way to express a removal. It emitted an empty
`removed_paths`, and `finalize_reconstruct` matched tombstones against
inline data-file paths only — returning every `ManifestRef` verbatim:
RootManifestEntry::ManifestRef { .. } => true,
So graduation had no mechanism to retire a ref at all. The base root
beneath the delta kept listing the already-graduated refs, every later
`reconstruct_root` re-offered them, and each tick re-graduated the same
refs and appended them to the bucket-index again. `cold_leaves` is seeded
from the existing index and appended to with no dedup, so the copies
stuck.
Measured on sri-olly 2026-08-16 (metrics_1m): 230,456 bucket-index rows
for 18,053 distinct leaves — 92.2% duplicates, one leaf repeated 74x,
2.97 GB of cold manifests, growing ~3,938 rows/tick unbounded. 100% of
the duplicated paths were resident in the base root, and
nodes_moved(4,194) - fold_batch(256) = 3,938 exactly.
Not a pure metadata concern: without the fix the second pass also
re-materializes already-cold content into a fresh leaf, so the same data
file becomes reachable through two leaves and is counted twice.
Fix:
* `finalize_reconstruct` honours tombstones for `ManifestRef` (by
manifest path), matching the inline behaviour. Manifest paths never
collide with the data-file paths other consumers match against, and
nothing reclaims off `removed_paths` (physical deletion runs off the
JSONL ledger), so widening the match is inert elsewhere.
* graduation records the refs it moved and stamps them on the delta's
`removed_paths`. Flat-base is unchanged: it writes `kept` in full and
ends the chain, so a tombstone there would never match and would grow
the carried set forever.
* dedup `cold_leaves` by manifest path — defence in depth, so a replay
from any other path cannot corrupt the index.
Tests (all three fail without the fix / assert the gap):
* finalize_reconstruct_tombstones_leaf_refs_not_only_inline
* finalize_reconstruct_carries_unmatched_ref_tombstone_forward
* test_v4_graduate_twice_does_not_duplicate_cold_leaves — graduate
twice with no new data; asserts leaf count unchanged, no repeated
leaf path, no data file reachable via two leaves, visibility
unchanged. Verified to FAIL on the pre-fix tree (8 leaves -> 15).
1230 lib tests pass (test_delete_local_file fails identically on a clean
tree — pre-existing, unrelated).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The guard added in fe363a0 ran after `write_bucket_index`, so it mutated only the in-memory vec: the persisted index kept every duplicate while the log line claimed they had been dropped. Measured live on sri-olly metrics_1m: log: dropped 262473 duplicate leaf refs (281795 -> 19322 distinct) index: 281,795 rows / 19,322 distinct / 262,473 redundant i.e. the written file was exactly the guard's INPUT. Same for logs (dropped 5891, index still 5,891 redundant). This also produced a misleading warn every tick and sent me chasing a non-existent lost-update race between laminar's appends and tessellate's cold writes — there is no race; the clean index was never written. Moves the dedup above the write and extracts it as a pure `dedupe_leaves_by_path` so the behaviour is directly testable. The tombstone fix in fe363a0 is unaffected and remains the actual fix for duplicate GROWTH (verified: metrics_1m flat at ~281.6K across 4+ ticks, was +21K/hour). The e2e test from fe363a0 passed despite this because the tombstone fix stops duplicates from ever entering `cold_leaves`, so the dedup path was never exercised — hence the new unit test drives it directly. Test: dedupe_leaves_keeps_first_occurrence_and_counts_drops 1231 lib tests pass (test_delete_local_file fails identically on a clean tree — pre-existing). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…scan
INCOMPLETE BY DESIGN — no producer yet. See "Not deployable" below.
`materialize_carried_tombstones` answered "does any cold leaf still list this
file?" by loading every cold leaf manifest. On sri-olly that is ~7,300
manifest loads costing ~29s, on laminar's ingest commit path, six times an
hour — measured four times in 45 minutes, catalog healthy each time
(update_table_ms 185-457). It is O(cold tier) for work that is inherently
O(tombstones): 7,127 loads to retire 358 paths, leaving the same 705 behind
every pass because those are cold-vetoed and nothing rewrites those leaves.
It also does not scale. Cost grows with retention until `total_manifests >
max_manifests` (8192), after which it silently returns (0,0) and retires
nothing. metrics_1m is ALREADY past that at 17,481 leaves — its tombstones
have never been evaluated. At 6-month retention (~460k leaves) every table
lands there and removed_paths grows unbounded again, which is the leak this
code exists to prevent.
This adds the structure that answers the same question from one small object.
Bloom filter, because its error mode matches the safety rule exactly: no
false negatives, so "definitely absent" is authoritative — the only direction
retirement needs. A false positive vetoes conservatively and costs one
tombstone one cycle; a false negative would resurrect deleted data, and the
structure cannot produce one. Sized to 1% FPP (~9.6 bits/path: ~600KB per
500k files, ~5MB at the 1000-tenant x 6-month target).
Correctness rests on the filter being a SUPERSET of real cold paths. Inserts
are exact; TTL removals are skipped, which preserves the superset. A writer
that adds to cold and forgets to insert would create a false negative — so
that must not depend on remembering: the sidecar stamps the leaf set it was
built against (count + order-independent digest), and any mismatch resolves
to ColdPresence::Unknown, which vetoes everything. Forgetting becomes a
performance bug, never a correctness one.
Consumer rewired: the cold scan is gone and `total_manifests` counts hot refs
only (single digits to low tens), so the 8192 cliff and its silent no-op go
with it. Hashing is FNV-1a, deliberately not DefaultHasher, whose output is
not stable across Rust releases — a sidecar written by one build must be
readable by another.
NOT DEPLOYABLE AS-IS. Nothing writes the sidecar, so every table resolves to
Unknown and tombstones stop retiring entirely. That is safe (fail-closed, no
resurrection) but trades a 29s stall for unbounded removed_paths growth. Two
pieces remain:
1. incremental extend in the fold — carry the filter forward and insert
only newly-graduated leaves' paths (loading just those, not the tier)
2. a seeder/rebuild in tessellate, which already loads leaves during
compaction — the first build has no prior filter to extend
Tests: 12 new. The load-bearing one asserts zero false negatives across 5,000
paths; others cover FPP staying near target, sizing/clamping, deterministic
hashing, order-independent coverage digest, added/removed leaf detection, and
that Unknown vetoes while Empty does not.
1241 lib tests pass (test_delete_local_file fails identically on a clean tree
— pre-existing, unrelated).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Serves both purposes the design needs from one operation: the initial seed
(no prior filter to extend) and drift recovery.
Corrects a claim in the previous commit and in the design notes. A false
positive is NOT transient. A Bloom only ever sets bits, so a collision is
deterministic and monotonic — the same path collides on every later pass and
that tombstone is stuck until the filter is rebuilt. Inserts make it strictly
worse.
TTL drift compounds it. Removals are skipped (that is what preserves the
superset invariant), so a tier that fully turns over every retention period
keeps inserting into a filter that never forgets:
after 30 days at 3-day retention: ~10x overload -> FPP ~99.5%
-> retirement stops entirely, silently
Which is the same silent degradation this work exists to remove. So rebuilds
are mandatory, not a tuning detail.
The trigger is measured, not scheduled. The denominator is free: live path
count is the sum of added_files + existing_files across leaves, already in the
bucket index being read. `saturation = n_items / live_paths`, rebuild above
1.5x. Nothing has to be tuned against the retention setting, and saturation is
exposed so drift is observable instead of being discovered when retirement has
already stopped.
`rebuild_cold_paths_sidecar` is the one place that still pays O(cold tier),
which is exactly why it is public: it must run from tessellate, off laminar's
ingest commit path, where leaves are already being loaded during compaction.
It fails rather than writing a partial filter — a filter missing paths is a
false negative, the one error that resurrects data.
Per-hour filters were considered, to make TTL removal exact and drift
impossible. Rejected for now: combined false-positive rate degrades as filters
multiply (69 hourly filters need ~2x the bits for 1% overall, 4,320 at 6-month
retention need ~3x), it trades a drift problem for a scaling one, and it still
needs a rebuild path for seeding. Revisit if rebuild cost becomes the
bottleneck at 6-month scale.
Tests: 4 new (14 in the module). `oversaturation_makes_absent_paths_read_as_
present` pins the drift argument itself — if 10x overload ever stops stranding
retirements, the case for mandatory rebuilds is wrong and should be revisited.
1245 lib tests pass (test_delete_local_file fails identically on a clean tree
— pre-existing, unrelated).
Still no producer on the write path: graduation, compact_cold_tier and
drop_cold_buckets must each maintain the sidecar in the same operation that
writes the bucket index, since the coverage digest changes on every leaf-set
change. Until then every table resolves to Unknown and retires nothing —
safe, but not yet a win.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Completes the producer. All three writers now update the sidecar in the SAME
operation that writes the bucket index, because the coverage digest is a
function of the leaf set — an index written without a matching sidecar reads
as Unknown immediately, so this cannot lag behind as periodic maintenance.
graduation adds leaves -> reads ONLY the newly graduated leaves
(a handful per commit at steady state)
compact_cold_tier adds + removes -> inserts self.added (already in hand,
costs nothing) plus rewritten leaves
drop_cold_buckets removes only -> carries the filter forward untouched
TTL removals are deliberately skipped rather than deleted. That is what keeps
the filter a SUPERSET of the real cold set, which is the invariant the whole
design rests on: a superset over-vetoes (conservative), a subset under-vetoes
(resurrects data). The dead paths left behind are exactly the drift that
needs_rebuild() measures and a rebuild clears.
A saturated filter is deliberately NOT carried forward — doing so would
entrench its dead weight and its already-stranded retirements, so it is left
for a rebuild instead.
Failure is non-fatal everywhere: the new index simply gets no sidecar, the
sweep reads Unknown and retires nothing until a rebuild seeds it. A partial
filter is never written, because a filter missing paths is a false negative —
the one error that resurrects data. Each outcome is logged, including the
skips, so a tier silently stuck in Unknown is visible rather than looking like
a healthy sweep that found nothing to do.
Two call sites needed the prior leaf set snapshotted before it is consumed
(compact_cold_tier :479, drop_cold_buckets :198); graduation's cold_leaves is
mutated in place by the ttl-prune and fold, so its prior set is captured right
after the bucket-index load.
Tests: 16 in the module. newly_added_leaves_is_the_difference_by_path pins the
"read exactly the new leaves" rule — reading the tier is the cost being
removed, reading too few is a false negative — and covers the TTL (removals
only) and pure-recluster (no change) cases.
1246 lib tests pass (test_delete_local_file fails identically on a clean tree
— pre-existing, unrelated).
NOT YET VERIFIED ON REAL DATA. The seed still has to run once per table from
tessellate before any table leaves Unknown, and the incremental path has not
been exercised against a live tier.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ntenance service Single public call for tessellate: reads the index and sidecar (cheap), and pays the O(cold tier) rebuild ONLY when the sidecar is missing (first seed) or saturated past the threshold (drift recovery). Safe to call every tick — the saturation check decides, not a schedule, so there is no cadence to keep in sync with the retention setting. Reports seed vs saturated separately, so a table that keeps rebuilding (churn above the threshold) is distinguishable from one that has never been seeded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`load_manifest_list` builds the V4 manifest list from two sources: the reconstructed root's entries, then the cold bucket-index's leaves. Nothing checked whether a `manifest_path` had already come from the root, so a ref present in both tiers was listed twice and every data file under it was scanned — and returned — twice. Graduation moves a ref OUT of the root and INTO the index, so the two sets should be disjoint. Both sides already guard that invariant (`dedupe_leaves_by_path` on the index, leaf-path tombstoning in `finalize_reconstruct` on the root), but the read side had no final guard, so any leak upstream became silent double-counting. Observed on sri-olly: three refs from one graduation event present in both tiers. Root entries win the dedup. A root `ManifestRef` can carry an MDV bitmap and a bucket-index leaf cannot, so keeping the leaf copy would silently drop MDV filtering for that manifest. Warn rather than swallow it: an overlap means graduation failed to remove the root copy, which is worth seeing rather than papering over. Note this fork already applies path tombstones correctly — `build_manifest_file_contexts` skips entries in `removed_paths` before constructing a context — so the companion scan-side fix needed on the 0.9.1 line does not apply here. Pre-existing unrelated failure in `io::file_io::tests::test_delete_local_file` reproduces on a clean tree; 1246 other tests pass.
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
2 participants
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.
Summary
MergeAppend— auto-compact manifests at commit time (6da0719)FastAppendAction.with_snapshot_id()+ publicgenerate_unique_snapshot_id(bd7461c)with_snapshot_id()onReplaceDataFilesAction(fc85a11)with_snapshot_id()onRewriteManifestsAction(6d3124e)Two fixes were originally on main but then reverted there (they remain active in this branch's history):
b63983efix(transaction): allow multiple StatisticsFile entries per snapshot — reverted bycc16218c22c418fix: prevent u64 underflow in snapshot summary total-data-files — reverted byff4c1c1Because the original commits are already in main's history (just reverted), merging this PR will NOT re-apply those two changes automatically — git treats them as already-merged-then-reverted. When merging this PR, you must also revert the reverts on main:
(or cherry-pick the original changes back). Do not skip this step, or the StatisticsFile and u64-underflow fixes will silently stay missing from main.
🤖 Generated with Claude Code