fix: review comments

Author: zhjwpku

src/iceberg/transaction.cc

@@ -93,7 +93,7 @@ std::string Transaction::MetadataFileLocation(std::string_view filename) const {
 
 Status Transaction::AddUpdate(const std::shared_ptr<PendingUpdate>& update) {
   ICEBERG_PRECHECK(update->kind() != PendingUpdate::Kind::kSnapshotManager,
-                   "SnapshotManager updates should not be added to the transaction");
+                   "SnapshotManager should not be added to the transaction");
   ICEBERG_CHECK(last_update_committed_,
                 "Cannot add update when previous update is not committed");
 

src/iceberg/update/snapshot_manager.cc

@@ -41,7 +41,7 @@ Result<std::shared_ptr<SnapshotManager>> SnapshotManager::Make(
 }
 
 SnapshotManager::SnapshotManager(std::shared_ptr<Transaction> transaction)
-    : PendingUpdate(transaction) {}
+    : PendingUpdate(std::move(transaction)) {}
 
 SnapshotManager::~SnapshotManager() = default;
 
@@ -58,9 +58,9 @@ SnapshotManager& SnapshotManager::SetCurrentSnapshot(int64_t snapshot_id) {
   return *this;
 }
 
-SnapshotManager& SnapshotManager::RollbackToTime(TimePointMs timestamp_ms) {
+SnapshotManager& SnapshotManager::RollbackToTime(int64_t timestamp_ms) {
   ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto set_snapshot, transaction_->NewSetSnapshot());
-  set_snapshot->RollbackToTime(UnixMsFromTimePointMs(timestamp_ms));
+  set_snapshot->RollbackToTime(timestamp_ms);
   ICEBERG_BUILDER_RETURN_IF_ERROR(set_snapshot->Commit());
   return *this;
 }
@@ -81,7 +81,7 @@ SnapshotManager& SnapshotManager::CreateBranch(const std::string& name) {
   const auto& current_refs = base().refs;
   ICEBERG_BUILDER_CHECK(!base().refs.contains(name), "Ref {} already exists", name);
   ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto fast_append, transaction_->NewFastAppend());
-  ICEBERG_BUILDER_RETURN_IF_ERROR(fast_append->ToBranch(name).Commit());
+  ICEBERG_BUILDER_RETURN_IF_ERROR(fast_append->SetTargetBranch(name).Commit());
   return *this;
 }
 
@@ -167,26 +167,24 @@ SnapshotManager& SnapshotManager::SetMaxRefAgeMs(const std::string& name,
   return *this;
 }
 
-Result<std::shared_ptr<Snapshot>> SnapshotManager::Apply() { return base().Snapshot(); }
-
 Status SnapshotManager::Commit() {
   ICEBERG_RETURN_UNEXPECTED(CheckErrors());
   return CommitIfRefUpdatesExist();
 }
 
 Result<std::shared_ptr<UpdateSnapshotReference>>
 SnapshotManager::UpdateSnapshotReferencesOperation() {
-  if (update_snapshot_references_operation_ == nullptr) {
-    ICEBERG_ASSIGN_OR_RAISE(update_snapshot_references_operation_,
+  if (update_snap_refs_ == nullptr) {
+    ICEBERG_ASSIGN_OR_RAISE(update_snap_refs_,
                             transaction_->NewUpdateSnapshotReference());
   }
-  return update_snapshot_references_operation_;
+  return update_snap_refs_;
 }
 
 Status SnapshotManager::CommitIfRefUpdatesExist() {
-  if (update_snapshot_references_operation_ != nullptr) {
-    ICEBERG_RETURN_UNEXPECTED(update_snapshot_references_operation_->Commit());
-    update_snapshot_references_operation_ = nullptr;
+  if (update_snap_refs_ != nullptr) {
+    ICEBERG_RETURN_UNEXPECTED(update_snap_refs_->Commit());
+    update_snap_refs_ = nullptr;
   }
   return {};
 }

src/iceberg/update/snapshot_manager.h

@@ -24,10 +24,8 @@
 
 #include "iceberg/iceberg_export.h"
 #include "iceberg/result.h"
-#include "iceberg/snapshot.h"
 #include "iceberg/type_fwd.h"
 #include "iceberg/update/pending_update.h"
-#include "iceberg/util/timepoint.h"
 
 namespace iceberg {
 
@@ -51,25 +49,25 @@ class ICEBERG_EXPORT SnapshotManager : public PendingUpdate {
   /// \brief Apply supported changes in given snapshot and create a new snapshot which
   /// will be set as the current snapshot on commit.
   ///
-  /// \param snapshot_id A snapshot ID whose changes to apply
+  /// \param snapshot_id A Snapshot ID whose changes to apply
   /// \return Reference to this for method chaining
   SnapshotManager& Cherrypick(int64_t snapshot_id);
 
-  /// \brief Roll this table's data back to a specific Snapshot identified by id.
+  /// \brief Roll this table's data back to a specific Snapshot ID.
   ///
-  /// \param snapshot_id Long id of the snapshot to roll back table data to
+  /// \param snapshot_id Snapshot ID to roll back table data to
   /// \return Reference to this for method chaining
   SnapshotManager& SetCurrentSnapshot(int64_t snapshot_id);
 
-  /// \brief Roll this table's data back to the last Snapshot before the given timestamp.
+  /// \brief Roll this table's data back to the last snapshot before the given timestamp.
   ///
   /// \param timestamp_ms A timestamp in milliseconds
   /// \return Reference to this for method chaining
-  SnapshotManager& RollbackToTime(TimePointMs timestamp_ms);
+  SnapshotManager& RollbackToTime(int64_t timestamp_ms);
 
-  /// \brief Rollback table's state to a specific Snapshot identified by id.
+  /// \brief Rollback table's state to a specific Snapshot ID.
   ///
-  /// \param snapshot_id Long id of snapshot id to roll back table to. Must be an ancestor
+  /// \param snapshot_id Snapshot ID to roll back table to. Must be an ancestor
   /// of the current snapshot
   /// \return Reference to this for method chaining
   SnapshotManager& RollbackTo(int64_t snapshot_id);
@@ -82,14 +80,14 @@ class ICEBERG_EXPORT SnapshotManager : public PendingUpdate {
   /// \return Reference to this for method chaining
   SnapshotManager& CreateBranch(const std::string& name);
 
-  /// \brief Create a new branch pointing to the given snapshot id.
+  /// \brief Create a new branch pointing to the given Snapshot ID.
   ///
   /// \param name Branch name
-  /// \param snapshot_id ID of the snapshot which will be the head of the branch
+  /// \param snapshot_id Snapshot ID which will be the head of the branch
   /// \return Reference to this for method chaining
   SnapshotManager& CreateBranch(const std::string& name, int64_t snapshot_id);
 
-  /// \brief Create a new tag pointing to the given snapshot id.
+  /// \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
@@ -111,30 +109,30 @@ class ICEBERG_EXPORT SnapshotManager : public PendingUpdate {
   /// \brief Replaces the tag with the given name to point to the specified snapshot.
   ///
   /// \param name Tag to replace
-  /// \param snapshot_id New snapshot id for the given tag
+  /// \param snapshot_id New Snapshot ID for the given tag
   /// \return Reference to this for method chaining
   SnapshotManager& ReplaceTag(const std::string& name, int64_t snapshot_id);
 
   /// \brief Replaces the branch with the given name to point to the specified snapshot.
   ///
   /// \param name Branch to replace
-  /// \param snapshot_id New snapshot id for the given branch
+  /// \param snapshot_id New Snapshot ID for the given branch
   /// \return Reference to this for method chaining
   SnapshotManager& ReplaceBranch(const std::string& name, int64_t snapshot_id);
 
-  /// \brief Replaces the from branch to point to the to snapshot. The to will remain
-  /// unchanged, and from branch will retain its retention properties. If the from branch
-  /// does not exist, it will be created with default retention properties.
+  /// \brief Replaces the `from` branch to point to the `to` snapshot. The `to` will
+  /// remain unchanged, and `from` branch will retain its retention properties. If the
+  /// `from` branch does not exist, it will be created with default retention properties.
   ///
   /// \param from Branch to replace
   /// \param to The branch from should be replaced with
   /// \return Reference to this for method chaining
   SnapshotManager& ReplaceBranch(const std::string& from, const std::string& to);
 
-  /// \brief Performs a fast-forward of from up to the to snapshot if from is an ancestor
-  /// of to. The to will remain unchanged, and from will retain its retention properties.
-  /// If the from branch does not exist, it will be created with default retention
-  /// properties.
+  /// \brief Performs a fast-forward of `from` up to the `to` snapshot if `from` is an
+  /// ancestor of `to`. The `to` will remain unchanged, and `from` will retain its
+  /// retention properties. If the `from` branch does not exist, it will be created with
+  /// default retention properties.
   ///
   /// \param from Branch to fast-forward
   /// \param to Ref for the from branch to be fast forwarded to
@@ -171,11 +169,6 @@ class ICEBERG_EXPORT SnapshotManager : public PendingUpdate {
   /// \return Reference to this for method chaining
   SnapshotManager& SetMaxRefAgeMs(const std::string& name, int64_t max_ref_age_ms);
 
-  /// \brief Apply the pending changes and return the current snapshot.
-  ///
-  /// \return The current snapshot after applying changes, or an error
-  Result<std::shared_ptr<Snapshot>> Apply();
-
   /// \brief Commit all pending changes.
   ///
   /// \return Status indicating success or failure
@@ -193,7 +186,7 @@ class ICEBERG_EXPORT SnapshotManager : public PendingUpdate {
   /// \brief Commit any pending reference updates if they exist.
   Status CommitIfRefUpdatesExist();
 
-  std::shared_ptr<UpdateSnapshotReference> update_snapshot_references_operation_;
+  std::shared_ptr<UpdateSnapshotReference> update_snap_refs_;
 };
 
 }  // namespace iceberg

src/iceberg/update/snapshot_update.cc

@@ -331,20 +331,6 @@ Status SnapshotUpdate::Finalize(std::optional<Error> commit_error) {
   return {};
 }
 
-Status SnapshotUpdate::SetTargetBranch(const std::string& branch) {
-  ICEBERG_PRECHECK(!branch.empty(), "Branch name cannot be empty");
-
-  if (auto ref_it = base().refs.find(branch); ref_it != base().refs.end()) {
-    ICEBERG_PRECHECK(
-        ref_it->second->type() == SnapshotRefType::kBranch,
-        "{} is a tag, not a branch. Tags cannot be targets for producing snapshots",
-        branch);
-  }
-
-  target_branch_ = branch;
-  return {};
-}
-
 Result<std::unordered_map<std::string, std::string>> SnapshotUpdate::ComputeSummary(
     const TableMetadata& previous) {
   std::unordered_map<std::string, std::string> summary = Summary();

src/iceberg/update/snapshot_update.h

@@ -80,11 +80,21 @@ class ICEBERG_EXPORT SnapshotUpdate : public PendingUpdate {
   ///
   /// \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) {
-    auto status = self.SetTargetBranch(branch);
-    if (!status.has_value()) {
-      return self.AddError(status.error());
+  auto& SetTargetBranch(this auto& self, const std::string& branch) {
+    if (branch.empty()) [[unlikely]] {
+      return self.AddError(ErrorKind::kInvalidArgument, "Branch name cannot be empty");
     }
+
+    if (auto ref_it = self.base().refs.find(branch); ref_it != self.base().refs.end()) {
+      if (ref_it->second->type() != SnapshotRefType::kBranch) {
+        return self.AddError(ErrorKind::kInvalidArgument,
+                             "{} is a tag, not a branch. Tags cannot be targets for "
+                             "producing snapshots",
+                             branch);
+      }
+    }
+
+    self.target_branch_ = branch;
     return self;
   }
 
@@ -133,7 +143,6 @@ class ICEBERG_EXPORT SnapshotUpdate : public PendingUpdate {
       std::span<const std::shared_ptr<DataFile>> files,
       const std::shared_ptr<PartitionSpec>& spec);
 
-  Status SetTargetBranch(const std::string& branch);
   const std::string& target_branch() const { return target_branch_; }
   bool can_inherit_snapshot_id() const { return can_inherit_snapshot_id_; }
   const std::string& commit_uuid() const { return commit_uuid_; }