Skip to content

fix(iceberg): merge manifests during COW overwrite - #26992

Open
chenzl25 wants to merge 2 commits into
mainfrom
dylan/main-cow-merge-manifests
Open

chenzl25 wants to merge 2 commits into
mainfrom
dylan/main-cow-merge-manifests

Conversation

@chenzl25

@chenzl25 chenzl25 commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Forward-port #26990 to RisingWave main. COW publication should prune obsolete deletion-only manifests and merge small data manifests during the existing overwrite commit, preventing metadata growth while the live file set stays small.

Update the pinned dependencies to iceberg-rust#234, commit 54d7ddc4565254fb284d0199edd0f8d331d0e3f9, and a compaction pin-alignment commit. Both are based on the exact dependencies already used by RisingWave main; no unrelated upstream dependency changes are pulled in. The lockfile changes only these two Git revisions and resolves a single Iceberg source.

Main's Iceberg dependency line no longer contains the old inline merge implementation, so the dependency patch adapts it into one private snapshot-producer helper. It retains the 8 MiB target / 100-manifest first-bin threshold, groups by partition spec, preserves original entry sequences and the current commit's added/deleted entries, and honors explicit property overrides. It adds no extra snapshot or data-file rewrite. No tests or V3-specific handling are added.

Validation:

  • 141 existing Iceberg transaction tests pass; the affected test modules are unchanged.
  • Iceberg Clippy with warnings denied and workspace formatting pass.
  • Compaction RUSTUP_TOOLCHAIN=nightly-2026-03-05 make check passes.
  • RW CXXFLAGS='-include cerrno' cargo +nightly-2026-06-21 check -p risingwave_storage --lib --locked passes. The local C++ flag supplies a missing FAISS header and is not part of the source change.
  • RW formatting, locked metadata resolution and diff checks pass.
  • External in-memory smoke validation: start with 100 manifests and execute three partial overwrites; each yields one manifest with exactly 100 expected live files and the current deletion, preserving original sequences, historical manifest readability and exactly one snapshot per operation. The smoke program is not part of any PR.
  • Request Iceberg e2e coverage alongside default PR CI.

Checklist

  • I have written necessary rustdoc comments.
  • I have added necessary unit tests and integration tests. (No new tests; existing tests and external smoke validation pass.)
  • I have added test labels as necessary.
  • I have added fuzzing tests or opened an issue to track them.
  • My PR contains breaking changes.
  • My PR changes performance-critical code, so I will run benchmarks and present the results.
  • I have checked which release branches need this fix. (release-3.0 counterpart: fix(iceberg): merge COW overwrite manifests on release-3.0 #26990.)

Documentation

  • My PR needs documentation updates. (Internal manifest maintenance; no supported user interface or operational workflow changes.)
Release note

Reduce Iceberg copy-on-write manifest accumulation and metadata overhead during publication.

Summary by CodeRabbit

  • Chores
    • Updated underlying table-format and storage components to newer revisions.
    • Included the latest available improvements and compatibility updates from these components.
    • Added an internal performance benchmark for storage-related operations.
    • No user-facing configuration or feature changes were introduced.

@chenzl25
chenzl25 requested a review from a team as a code owner September 8, 2026 13:30
@chenzl25
chenzl25 requested a review from wenym1 September 8, 2026 13:30
@github-actions github-actions Bot added type/fix Type: Bug fix. Only for pull requests. ci/run-e2e-iceberg-tests labels Sep 8, 2026
@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

Understand this PR’s impact

Explore downstream dependencies and potential security impact with Blast Radius.

View blast radius →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: fcef50ee-4403-45c5-b22f-421b847addca

📥 Commits

Reviewing files that changed from the base of the PR and between d305589 and d2a6c89.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (1)
  • src/storage/Cargo.toml

Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

The pull request updates pinned Git revisions for Iceberg and iceberg-compaction-core. It also adds the bench_sstable_builder benchmark target with harness = false and the test feature requirement.

Changes

Dependency revisions and benchmark configuration

Layer / File(s) Summary
Manifest revision pins
Cargo.toml, src/storage/Cargo.toml
The manifests now reference newer commits for the Iceberg and iceberg-compaction-core dependencies. Feature lists and the repository URL remain unchanged.
SSTable builder benchmark
src/storage/Cargo.toml
The storage manifest defines bench_sstable_builder with harness = false and required-features = ["test"].

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Bug fix

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the primary change: merging manifests during Iceberg copy-on-write overwrites.
Description check ✅ Passed The description follows the required template, explains the implementation and rationale, documents validation, addresses checklist items, and includes a release note. Optional test and documentation …
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chenzl25
chenzl25 requested a review from Li0k September 20, 2026 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-iceberg ci/run-e2e-iceberg-tests type/fix Type: Bug fix. Only for pull requests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant