feat: add snapshot util #420

zhjwpku · 2025-12-17T17:00:13Z

No description provided.

wgtmac

I haven't finished my review yet. Just post my findings so far. Will review it later.

wgtmac · 2025-12-19T09:13:56Z

src/iceberg/util/snapshot_util.cc

+    const Table& table, int64_t snapshot_id) {
+  ICEBERG_ASSIGN_OR_RAISE(auto start, table.SnapshotById(snapshot_id));
+  if (!start) {
+    return InvalidArgument("Cannot find snapshot: {}", snapshot_id);


Suggested change

return InvalidArgument("Cannot find snapshot: {}", snapshot_id);

return NotFound("Cannot find snapshot: {}", snapshot_id);

BTW, perhaps this should be a ICEBERG_DCHECK.

wgtmac · 2025-12-19T09:32:28Z

src/iceberg/util/snapshot_util.cc

+      break;
+    }
+    auto parent_result = table.SnapshotById(current->parent_snapshot_id.value());
+    if (!parent_result.has_value()) {


We might need to check if the error is NotFound and return the original error for other kind of errors.

wgtmac · 2025-12-19T09:32:58Z

src/iceberg/util/snapshot_util.cc

+      // Parent snapshot not found (e.g., expired), stop traversal
+      break;
+    }
+    current = parent_result.value();


Suggested change

current = parent_result.value();

current = std::move(parent_result.value());

wgtmac · 2025-12-19T09:35:16Z

src/iceberg/util/snapshot_util_internal.h

+  /// Snapshot.
+  ///
+  /// \param table The table
+  /// \return The oldest snapshot, or nullopt if there is no current snapshot


It would be good to document that all returned std::shared_ptr<Snapshot> cannot be nullptr.

wgtmac · 2025-12-19T09:38:22Z

src/iceberg/util/snapshot_util.cc

+
+Result<std::optional<std::shared_ptr<Snapshot>>> SnapshotUtil::OldestAncestorAfter(
+    const Table& table, TimePointMs timestamp_ms) {
+  ICEBERG_ASSIGN_OR_RAISE(auto current, table.current_snapshot());


Actually it is not an error if current snapshot is not found. We can return std::nullopt when the result error is NotFound.

wgtmac · 2025-12-19T09:38:54Z

src/iceberg/util/snapshot_util.cc

+Result<std::optional<std::shared_ptr<Snapshot>>> SnapshotUtil::OldestAncestorAfter(
+    const Table& table, TimePointMs timestamp_ms) {
+  ICEBERG_ASSIGN_OR_RAISE(auto current, table.current_snapshot());
+  if (!current) {


table.current_snapshot() should not return a nullptr, so this should be an error instead.

wgtmac · 2025-12-19T09:51:38Z

src/iceberg/util/snapshot_util.cc

+Result<bool> SnapshotUtil::IsAncestorOf(const Table& table, int64_t snapshot_id,
+                                        int64_t ancestor_snapshot_id) {
+  ICEBERG_ASSIGN_OR_RAISE(auto ancestors, AncestorsOf(table, snapshot_id));
+  for (const auto& snapshot : ancestors) {


nit: use std::ranges for this kind of processing.

wgtmac · 2025-12-19T12:39:04Z

src/iceberg/util/snapshot_util.cc

+    return std::nullopt;
+  }
+
+  std::shared_ptr<Snapshot> last_snapshot = nullptr;


What about using std::optional<std::shared_ptr<Snapshot>> so we don't need to check at line 111.

wgtmac · 2025-12-19T12:40:10Z

src/iceberg/util/snapshot_util.cc

+    return last_snapshot;
+  }
+
+  return ValidationFailed("Cannot find snapshot older than {}",


Suggested change

return ValidationFailed("Cannot find snapshot older than {}",

return NotFound("Cannot find snapshot older than {}",

I'm undecided which is better, ValidationFailed, NotFound or Invalid.

wgtmac · 2025-12-19T12:59:29Z

src/iceberg/util/snapshot_util.cc

+                                                              int64_t to_snapshot_id) {
+  ICEBERG_ASSIGN_OR_RAISE(auto to_snapshot, table.SnapshotById(to_snapshot_id));
+  if (!to_snapshot) {
+    return InvalidArgument("Cannot find snapshot: {}", to_snapshot_id);


InvalidArgument should be used for input argument. This is an invalid state so perhaps return Invalid?

wgtmac

Thanks for working on this!

wgtmac reviewed Dec 19, 2025

View reviewed changes

zhjwpku added 3 commits December 19, 2025 23:35

feat: add snapshot util

a0dc533

fix: review comments

8c6600b

fix: rebase conflict

2dc17d0

zhjwpku force-pushed the add_snapshot_util branch from 79b4b5c to 2dc17d0 Compare December 19, 2025 16:03

zhjwpku and others added 2 commits December 20, 2025 10:34

fix: resolve more feedback

219b030

refine error handling

3a7b9f2

wgtmac approved these changes Dec 22, 2025

View reviewed changes

wgtmac merged commit dba8f92 into apache:main Dec 22, 2025
9 checks passed

zhjwpku deleted the add_snapshot_util branch December 22, 2025 13:40

	return InvalidArgument("Cannot find snapshot: {}", snapshot_id);
	return NotFound("Cannot find snapshot: {}", snapshot_id);

	current = parent_result.value();
	current = std::move(parent_result.value());

	return ValidationFailed("Cannot find snapshot older than {}",
	return NotFound("Cannot find snapshot older than {}",

feat: add snapshot util #420

feat: add snapshot util #420

Uh oh!

Conversation

zhjwpku commented Dec 17, 2025

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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants