feat: add SnapshotManager by zhjwpku · Pull Request #542 · apache/iceberg-cpp

zhjwpku · 2026-01-28T10:31:59Z

The test cases are derived from Java's TestSnapshotManager.java. Since MergeAppend is not supported yet, some tests are omitted for now and can be added later.

evindj

Overall LGTM only question that I have is why is UpdateSnapshotReference and SnapShotManager both exposed. Also, is there a way we can integration test this change?

src/iceberg/test/snapshot_manager_test.cc

evindj · 2026-01-30T03:26:52Z

src/iceberg/transaction.h

  /// and tags) and commit the changes.
  Result<std::shared_ptr<UpdateSnapshotReference>> NewUpdateSnapshotReference();

+  /// \brief Create a new SnapshotManager to manage snapshots.


Can you be a bit more descriptive here especially how it differs from NewUpdateSnapshotReference() I am a little confuse as to why the transaction would expose both since SnapshotManager seems to be a wrapper on top of UpdateSnapshotReference

My understanding is that UpdateSnapshotReference should be added to the Transaction's pending updates, whereas SnapshotManager should no, just as Gang noted below, SnapshotManager has its own commit logic.

src/iceberg/update/snapshot_manager.h

src/iceberg/update/snapshot_manager.cc

src/iceberg/type_fwd.h

wgtmac · 2026-01-31T03:01:06Z

src/iceberg/update/snapshot_update.h

+  ///
+  /// \param branch Which is name of SnapshotRef of type branch
+  /// \return Reference to this for method chaining
+  auto& ToBranch(this auto& self, const std::string& branch) {


Should we rename SetTargetBranch to ToBranch as they are equivalent?

To be precise, I was suggesting to remove ToBranch by reusing SetTargetBranch because they are literally equivalent. If we think ToBranch is a better name, we should just rename it without adding a new one.

Got it, changed to SetTargetBranch.

src/iceberg/update/snapshot_manager.h

src/iceberg/update/snapshot_manager.cc

src/iceberg/update/snapshot_manager.h

src/iceberg/update/snapshot_manager.cc

wgtmac · 2026-02-04T15:20:50Z

src/iceberg/update/snapshot_update.h

+  ///
+  /// \param branch Which is name of SnapshotRef of type branch
+  /// \return Reference to this for method chaining
+  auto& ToBranch(this auto& self, const std::string& branch) {


To be precise, I was suggesting to remove ToBranch by reusing SetTargetBranch because they are literally equivalent. If we think ToBranch is a better name, we should just rename it without adding a new one.

src/iceberg/update/snapshot_manager.h

wgtmac · 2026-02-04T15:47:09Z

src/iceberg/update/snapshot_manager.h

+
+  /// \brief Roll this table's data back to a specific Snapshot identified by id.
+  ///
+  /// \param snapshot_id Long id of the snapshot to roll back table data to


Suggested change

/// \param snapshot_id Long id of the snapshot to roll back table data to

/// \param snapshot_id id of the snapshot to roll back table data to

Same for below

wgtmac · 2026-02-04T15:49:04Z

src/iceberg/update/snapshot_manager.h

+  /// \brief Create a new tag pointing to the given snapshot id.
+  ///
+  /// \param name Tag name
+  /// \param snapshot_id Snapshot ID for the head of the new tag


Let's be consistent with id, ID and Snapshot ID in these comments.

All changed to Snapshot ID since it's used in other places.

wgtmac · 2026-02-05T05:08:54Z

src/iceberg/update/snapshot_manager.cc

+  return *this;
+}
+
+SnapshotManager& SnapshotManager::SetCurrentSnapshot(int64_t snapshot_id) {


IIUC, we need to call CommitIfRefUpdatesExist() before anything in this SetCurrentSnapshot and RollbackToXXX below because different pending updates like UpdateSnapshotReference and SetSnapshot cannot be interleaved.

Yes, the Java impl behaves this way, but I'm not entirely sure about our implementation. After calling CommitIfRefUpdatesExist(), the transaction state becomes committed, which should prevent any further commits.

Maybe we can remove CommitIfRefUpdatesExist and put its logic directly into Commit.

src/iceberg/update/snapshot_manager.h

wgtmac · 2026-02-05T05:14:03Z

src/iceberg/update/snapshot_manager.cc

+Result<std::shared_ptr<Snapshot>> SnapshotManager::Apply() { return base().Snapshot(); }
+
+Status SnapshotManager::Commit() {
+  ICEBERG_RETURN_UNEXPECTED(CheckErrors());


Shouldn't we call CheckErrors() in the Apply()?

Apply() removed, so I think the CheckErrors() here is still needed.

src/iceberg/transaction.cc

wgtmac · 2026-02-05T05:28:50Z

src/iceberg/update/snapshot_manager.cc

+  return *this;
+}
+
+Result<std::shared_ptr<Snapshot>> SnapshotManager::Apply() { return base().Snapshot(); }


If nothing happens on the transaction side and nowhere calls this, perhaps we don't even need the Apply function?

Reasonable, let's just remove it for now.

feat: add SnapshotManager

f5007cc

The test cases are derived from Java's TestSnapshotManager.java. Since MergeAppend is not supported yet, some tests are omitted for now and can be added later.

evindj reviewed Jan 30, 2026

View reviewed changes

wgtmac reviewed Jan 31, 2026

View reviewed changes

zhjwpku added 2 commits February 3, 2026 14:36

fix: review comments

5caa472

fix: SnapshotManager::kind() returns kSnapshotManager

3fdb461

wgtmac reviewed Feb 5, 2026

View reviewed changes

fix: review comments

7d30c11

	/// \param snapshot_id Long id of the snapshot to roll back table data to
	/// \param snapshot_id id of the snapshot to roll back table data to

Conversation

zhjwpku commented Jan 28, 2026

Uh oh!

evindj left a comment

Choose a reason for hiding this comment

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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