manuzhang commented on code in PR #1104:
URL: https://github.com/apache/iceberg-cpp/pull/1104#discussion_r4524102491


##########
src/iceberg/update/snapshot_update.cc:
##########
@@ -384,43 +398,33 @@ Result<SnapshotUpdate::ApplyResult> 
SnapshotUpdate::Apply() {
                      .stage_only = stage_only_};
 }
 
-Status SnapshotUpdate::Finalize([[maybe_unused]] const TableMetadata& 
metadata) {
-  if (staged_snapshot_ == nullptr) {
-    return {};
-  }
-
-  // Only files created by this update are tracked, and committed paths are 
kept below.
-  internal::LogAndIgnoreFailure("Snapshot cleanup", [this]() -> Status {
-    auto snapshot_reader = SnapshotReader(staged_snapshot_.get());
-    ICEBERG_ASSIGN_OR_RAISE(auto manifests, 
snapshot_reader.Manifests(ctx_->table->io()));
-    auto committed = manifests | std::views::transform([](const auto& 
manifest) {
-                       return manifest.manifest_path;
-                     }) |
-                     std::ranges::to<std::unordered_set<std::string>>();
-    internal::LogAndIgnoreFailure("Snapshot operation cleanup", [this, 
&committed] {
-      return CleanUncommitted(committed);
-    });
-    committed.insert(staged_snapshot_->manifest_list);
-    std::vector<std::string> unused;
-    {
-      std::lock_guard lock(staging_mutex_);
-      for (const auto& path : staged_files_) {
-        if (!committed.contains(path)) {
-          unused.push_back(path);
-        }
-      }
-    }
-    for (const auto& path : unused) {
-      std::ignore = DeleteFile(path);
-    }
-    return {};
+Status SnapshotUpdate::CleanUncommittedFiles(
+    const std::unordered_set<std::string>& committed) {
+  // All retained snapshots share this set. A later update may reuse our 
output.
+  internal::LogAndIgnoreFailure("Snapshot operation cleanup", [this, 
&committed] {
+    return CleanUncommitted(committed);
   });
-
+  std::vector<std::string> unused;
   {
     std::lock_guard lock(staging_mutex_);
-    staged_files_.clear();
+    for (const auto& path : staged_files_) {
+      if (!committed.contains(path)) {
+        unused.push_back(path);
+      }
+    }
+    for (const auto& path : committed) {

Review Comment:
   We can skip unused path with `std::erase_if` and `committed.contains`.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to