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

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

No description provided.

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?

I think a hash could merge distinct keys on collisions(though little chance). Java accepts an Object key, so I used string here.

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.

This preserves Java's behavior: SnapshotUpdate.toBranch() defaults to throwing UnsupportedOperationException, and BaseRewriteManifests does not override it. Since the C++ base class provides a shared ToBranch implementation, RewriteManifests needs this check to preserve that restriction

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?

This hook lets derived snapshot updates validate branch support while reusing ToBranch. RewriteManifests uses it to preserve Java's unsupported-branch behavior.

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.

zhjwpku · 2026-08-02T15:39:44Z

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.

I think this standalone update lifecycle logic deserves a dedicated PR, so I do that in #868, please take a look

github-actions · 2026-09-02T00:48:06Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

github-actions · 2026-09-10T00:45:49Z

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

manuzhang · 2026-10-10T08:11:27Z

@zhjwpku could you please revive this PR?

zhjwpku · 2026-10-10T08:25:11Z

@zhjwpku could you please revive this PR?

OK, I plan to work on this tomorrow, thanks for the reminder :)

manuzhang · 2026-10-12T04:02:38Z

  return writer->ToManifestFile();
 }

+Result<ManifestFile> CopyRewriteManifest(


This has duplicate logic with CopyAddendManifest. It'd better to be put into a common helper.

I took a closer look at CopyAppendManifest. The implementations are similar, but their parameters and behavior differ, I don't see a clean way to extract a common helper :(

manuzhang · 2026-10-12T04:06:33Z

+  Status ValidateTargetBranch(const std::string& branch) const override;
+
+ private:
+  std::string table_name_;


where is this used?

This follows the existing pattern: several update operations retain this field without using it. We could revisit these fields together in a follow-up PR.

manuzhang · 2026-10-12T04:20:37Z

+  ICEBERG_BUILDER_CHECK(manifest.sequence_number == kInvalidSequenceNumber,
+                        "Sequence number must be assigned during commit");
+
+  if (can_inherit_snapshot_id()) {


We need to check !manifest.added_snapshot_id.has_value() here as well.

Agreed, Java impl does this, will fix.

manuzhang · 2026-10-12T04:35:37Z

+  return summary_.Build();
+}
+
+void RewriteManifests::SetSummaryProperty(const std::string& property,


This might be worth moving to SnapshotUpdate as a follow-up.

Will create a follow-up PR once this got merged.

manuzhang · 2026-10-12T04:39:45Z

  }

+  Result<std::vector<ManifestEntry>> ReadManifestEntries(const ManifestFile& manifest) {
+    return ReadManifestEntries(std::span<const ManifestFile>(&manifest, 1));


Might be worth replacing duplicates in fast_apend_test.cc with this as a follow-up.

Agreed. Reusing these helpers in fast_append_test.cc would be a useful follow-up cleanup, will do.

manuzhang · 2026-10-12T04:45:29Z

+  /// If manifest entries are allowed to inherit the snapshot ID assigned on
+  /// commit, the manifest should never be deleted manually if the commit succeeds
+  /// as it will become part of the table metadata and will be cleaned up on
+  /// expiry. If the manifest gets merged with others while preparing a new


Manifests are neither merged nor deleted in AddManifest.

Updated the docs to remove the merge/delete description and clarify ownership of the original manifest and its copy.

Normalize legacy replacement sequence numbers on format upgrades, invalidate cached output when rewrite settings change, and compute manifest counts in Summary. Require an unset snapshot ID for manifest reuse, clarify AddManifest ownership, and add regression coverage.

manuzhang · 2026-10-13T07:53:31Z

+  /// be cleaned up on expiry. This update does not delete the original on failure.
+  RewriteManifests& AddManifest(const ManifestFile& manifest);
+
+  std::string operation() override;


The methods from here should not be public interfaces.

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

Comment thread src/iceberg/update/rewrite_manifests.cc Outdated

manuzhang reviewed Jul 2, 2026

View reviewed changes

Comment thread src/iceberg/update/rewrite_manifests.cc Outdated

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

Comment thread src/iceberg/update/rewrite_manifests.cc Outdated

manuzhang added this to the 0.4.0 milestone Jul 21, 2026

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

Comment thread src/iceberg/update/pending_update.cc Outdated

Comment thread src/iceberg/update/pending_update.cc Outdated

Comment thread src/iceberg/update/pending_update.cc Outdated

github-actions Bot added the stale label Sep 2, 2026

github-actions Bot closed this Sep 10, 2026

wgtmac reopened this Sep 20, 2026

zhjwpku mentioned this pull request Sep 20, 2026

Track Iceberg v3 Feature Support (Roadmap for 0.4.0) #637

Closed

14 of 21 tasks

zhjwpku force-pushed the support_rewrite_manifests branch from ecc0058 to d8fd08b Compare September 20, 2026 14:57

github-actions Bot removed the stale label Sep 21, 2026

manuzhang modified the milestones: 0.4.0, 0.5.0 Sep 22, 2026

manuzhang mentioned this pull request Sep 23, 2026

Track remaining Iceberg v3 feature support (Roadmap for 0.5.0) #959

Open

8 tasks

zhjwpku added 2 commits October 11, 2026 17:14

feat: add rewrite manifests

a53c5f2

fix: bound manifest rewrite reads and use shared update ownership

d83d0a8

zhjwpku force-pushed the support_rewrite_manifests branch from d8fd08b to d83d0a8 Compare October 11, 2026 09:31

zhjwpku requested review from manuzhang and wgtmac October 12, 2026 00:45

manuzhang reviewed Oct 12, 2026

View reviewed changes

manuzhang reviewed Oct 13, 2026

View reviewed changes

Conversation

zhjwpku commented Jun 23, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zhjwpku commented Aug 2, 2026

Uh oh!

github-actions Bot commented Sep 2, 2026

Uh oh!

github-actions Bot commented Sep 10, 2026

Uh oh!

manuzhang commented Oct 10, 2026

Uh oh!

zhjwpku commented Oct 10, 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!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants