feat: add rewrite manifests by zhjwpku · Pull Request #774 · apache/iceberg-cpp

zhjwpku · 2026-06-23T16:58:47Z

No description provided.

manuzhang · 2026-07-02T07:22:31Z

+  auto new_writer = [this, &schema](const RewriteWriter& rewrite_writer)
+      -> Result<std::unique_ptr<ManifestWriter>> {
+    std::optional<int64_t> first_row_id = std::nullopt;
+    if (base().format_version >= 3 && rewrite_writer.content == ManifestContent::kData) {


If the table was upgraded from v2, existing live files may still have null per-file row IDs. Then ManifestEntryAdapterV3::GetFirstRowId falls back to the writer-level value, so those files get 0, and the manifest list skips assignment because the manifest now has a non-null first_row_id. Java leaves the rewrite writer’s manifest firstRowId null via newManifestWriter, allowing manifest-list assignment when needed after upgrade.

Good catch. I removed the non-null rewrite writer first_row_id logic and added a v2 -> v3 regression test.

manuzhang · 2026-07-02T09:00:03Z

+  const bool has_direct_replacements = !deleted_manifests_.empty() ||
+                                       !added_manifests_.empty() ||
+                                       !rewritten_added_manifests_.empty();
+  if (!cluster_by_func_ && !predicate_ && has_direct_replacements) {


Java does not rewrite at all when clusterByFunc == null, while this rewrites when predicate_ is set. Is it intentional?

Not intentional. I changed RequiresRewrite to match Java.

manuzhang · 2026-07-06T07:09:41Z

+                                      rewrite_writer.content);
+  };
+
+  std::vector<ManifestFile> result;


Closed manifests are kept only in this local result vector until Rewrite() returns, and new_manifests_ is assigned only after the call succeeds in Apply(). If a later length(), WriteExistingEntry, Close(), or
ToManifestFile() returns an error, finalization/cleanup no longer has the already-created manifest paths.

Could we either append closed manifests to member state as soon as they are produced, or clean up the local partial result on the error path? It would also be good to add a test that injects an apply-time manifest writer failure after at least one rolled manifest has been closed.

Good catch. I'll track rewritten manifests once each writer is closed so that apply-time failures can clean up already-created manifests. Will add a regression test too.

wgtmac · 2026-07-28T05:49:03Z

+/// snapshot and reattempting the commit.
+class ICEBERG_EXPORT RewriteManifests : public SnapshotUpdate {
+ public:
+  using ClusterByFunc = std::function<std::string(const DataFile&)>;


Why this function returns string instead of size_t just like what C++ hash function does?

wgtmac · 2026-07-28T07:51:26Z

+  std::vector<ManifestFile> deleted_manifests_;
+  std::unordered_set<std::string> deleted_manifest_paths_;


We should combine deleted_manifests_ and deleted_manifest_paths_ into a single unordered_set<ManifestFile> by using pure ManifestFile.path in its hash function.

Same for rewritten_manifests_ and rewritten_manifest_paths_ below.

wgtmac · 2026-07-28T07:53:02Z

+
+struct RewriteWriter {
+  std::shared_ptr<PartitionSpec> spec;
+  ManifestContent content;


Why do we need this? This class only rewrites data manifests, right?

wgtmac · 2026-07-28T07:58:13Z

+  return *this;
+}
+
+Status RewriteManifests::ValidateTargetBranch(const std::string& branch) const {


Why do we need this? I didn't find this in the Java code.

wgtmac · 2026-07-28T07:58:48Z

+  ICEBERG_PRECHECK(snapshot != nullptr,
+                   "Cannot rewrite manifests without a current snapshot");
+
+  SnapshotCache cached_snapshot(snapshot.get());


I have to admit that SnapshotCache is not a good name. Perhaps we should rename it to SnapshotReader or something.

wgtmac · 2026-07-28T09:15:56Z

  if (!ctx_->transaction) {
    // Table-created path: no transaction exists yet, create a temporary one.
    ICEBERG_ASSIGN_OR_RAISE(auto txn, Transaction::Make(ctx_));
+    auto self = weak_from_this().lock();


Why do we need this kind of change?

Review comment from Codex: This registers standalone updates for commit retries, but explicit transaction updates still return from txn->Apply without calling Finalize on failure. A failed rewrite can leave generated manifests behind. Please finalize or abort transaction-owned updates on apply failure and add a Transaction::NewRewriteManifests fault-injection test.

wgtmac · 2026-07-28T09:16:41Z

      return self.AddError(ErrorKind::kInvalidArgument, "Branch name cannot be empty");
    }

+    if (auto status = static_cast<SnapshotUpdate&>(self).ValidateTargetBranch(branch);


Why do we need this?

wgtmac · 2026-07-28T09:26:13Z

+  return {};
+}
+
+Result<ManifestFile> RewriteManifests::CopyManifest(const ManifestFile& manifest) {


Let's move this function to src/iceberg/manifest/manifest_util_internal.h and follow the style of CopyAppendManifest?

wgtmac · 2026-07-28T09:46:53Z

+
+  ICEBERG_ASSIGN_OR_RAISE(
+      auto writer,
+      ManifestWriter::MakeWriter(base().format_version, SnapshotId(), ManifestPath(),


The generated path is lost after MakeWriter succeeds. If writing, closing, or ToManifestFile fails, the output is not reliably closed or tracked for cleanup. Please retain the path and manage it with RAII until the manifest is completed successfully.

Unfortunately, CopyAppendManifest in the manifest_util_internal.h has the same issue.

wgtmac · 2026-07-28T09:48:30Z

+                         "Manifest entry in {} is missing data_file",
+                         manifest_entry.manifest.manifest_path);
+        auto key = WriterKey{
+            cluster_by_func_ ? cluster_by_func_(*entry.data_file) : std::string{},


cluster_by_func_ is user code and may throw. An exception here bypasses the close-all loop and leaves partial manifests untracked. Please make writer cleanup RAII and add a throwing-callback test.

wgtmac

These comments are about the standalone update lifecycle:

Registration must be mandatory so commit retries reapply the update.
The temporary transaction must stay detached so its expired weak pointer does not break a later Commit().
No-op commits must still call Finalize() because cleanup is now delegated to Transaction.

Together, these keep one ownership, retry, and finalization path for standalone and explicit transactions.

wgtmac · 2026-07-29T03:35:29Z

+    auto self = weak_from_this().lock();
+    if (self) {
+      ICEBERG_RETURN_UNEXPECTED(txn->AddUpdate(self));
+    }


Please require shared ownership and always register the update.

Suggested change

auto self = weak_from_this().lock();

if (self) {

ICEBERG_RETURN_UNEXPECTED(txn->AddUpdate(self));

}

auto self = weak_from_this().lock();

ICEBERG_PRECHECK(self != nullptr,

"PendingUpdate must be owned by std::shared_ptr");

ICEBERG_RETURN_UNEXPECTED(txn->AddUpdate(self));

wgtmac · 2026-07-29T03:35:29Z

@@ -35,6 +35,10 @@ Status PendingUpdate::Commit() {
  if (!ctx_->transaction) {
    // Table-created path: no transaction exists yet, create a temporary one.
    ICEBERG_ASSIGN_OR_RAISE(auto txn, Transaction::Make(ctx_));


Please keep this temporary transaction detached. Remove ctx->transaction = ... from Transaction::Make(ctx); only explicit transactions should set it. Otherwise the next Commit() sees an expired transaction.

wgtmac · 2026-07-29T03:35:29Z

    }

-    std::ignore = Finalize(commit_result.value()->metadata().get());
+    if (!self) {


Please route the empty-update branch in Transaction::Commit() through the normal finalization block. Otherwise registered no-op updates skip Finalize().

zhjwpku force-pushed the support_rewrite_manifests branch 7 times, most recently from 18bade1 to 7de6949 Compare June 28, 2026 06:40

zhjwpku marked this pull request as ready for review June 28, 2026 06:42

zhjwpku force-pushed the support_rewrite_manifests branch 2 times, most recently from 5c1ee1f to dd9c7e6 Compare July 1, 2026 16:56

manuzhang reviewed Jul 2, 2026

View reviewed changes

zhjwpku force-pushed the support_rewrite_manifests branch from dd9c7e6 to 3736bc1 Compare July 4, 2026 16:01

manuzhang reviewed Jul 6, 2026

View reviewed changes

manuzhang added this to the 0.4.0 milestone Jul 21, 2026

zhjwpku added 3 commits July 24, 2026 15:00

feat: add rewrite manifests

8562e13

resolve review comments

d4ba26b

fix: clean up failed rewrite manifest outputs

ecc0058

zhjwpku force-pushed the support_rewrite_manifests branch from 3736bc1 to ecc0058 Compare July 24, 2026 09:19

wgtmac reviewed Jul 28, 2026

View reviewed changes

wgtmac reviewed Jul 29, 2026

View reviewed changes

		std::vector<ManifestFile> deleted_manifests_;
		std::unordered_set<std::string> deleted_manifest_paths_;

Uh oh!

Conversation

zhjwpku commented Jun 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants