fix: resolve CI build failures on GCC/MSVC

- Add GetFileIO() helper to avoid direct transaction_ access in new
  methods (GCC could not resolve protected member in certain contexts)
- Remove goto/label pattern that MSVC rejected (syntax error before ':')
- Extract ReadDataFilesFromManifest() and FindManifestByPath() helpers
  to replace goto-based nested loop exits
- Use designated initializers for Error struct (clang-tidy)

Author: shangxinli

src/iceberg/update/expire_snapshots.cc

@@ -316,15 +316,18 @@ Status ExpireSnapshots::Finalize(std::optional<Error> commit_error) {
   return CleanExpiredFiles(apply_result_->snapshot_ids_to_remove);
 }
 
+std::shared_ptr<FileIO> ExpireSnapshots::GetFileIO() const {
+  return transaction_->table()->io();
+}
+
 void ExpireSnapshots::DeleteFilePath(const std::string& path) {
   try {
     if (delete_func_) {
       delete_func_(path);
     } else {
-      auto status = transaction_->table()->io()->DeleteFile(path);
       // Best-effort: ignore NotFound (file already deleted) and other errors.
       // Java uses suppressFailureWhenFinished + onFailure logging.
-      std::ignore = status;
+      std::ignore = GetFileIO()->DeleteFile(path);
     }
   } catch (...) {
     // Suppress all exceptions during file cleanup to match Java's
@@ -334,19 +337,14 @@ void ExpireSnapshots::DeleteFilePath(const std::string& path) {
 
 Status ExpireSnapshots::ReadManifestsForSnapshot(
     int64_t snapshot_id, std::unordered_set<std::string>& manifest_paths) {
-  const TableMetadata& metadata = base();
-  auto file_io = transaction_->table()->io();
-
-  auto snapshot_result = metadata.SnapshotById(snapshot_id);
+  auto snapshot_result = base().SnapshotById(snapshot_id);
   if (!snapshot_result.has_value()) {
     return {};
   }
-  auto& snapshot = snapshot_result.value();
 
-  SnapshotCache snapshot_cache(snapshot.get());
-  auto manifests_result = snapshot_cache.Manifests(file_io);
+  SnapshotCache snapshot_cache(snapshot_result.value().get());
+  auto manifests_result = snapshot_cache.Manifests(GetFileIO());
   if (!manifests_result.has_value()) {
-    // Best-effort: skip this snapshot if we can't read its manifests
     return {};
   }
 
@@ -357,46 +355,66 @@ Status ExpireSnapshots::ReadManifestsForSnapshot(
   return {};
 }
 
+// Helper to read data file paths from a manifest, collecting into or erasing from a set.
+void ExpireSnapshots::ReadDataFilesFromManifest(
+    const ManifestFile& manifest, std::shared_ptr<FileIO> file_io,
+    std::unordered_set<std::string>& file_paths, bool erase) {
+  const TableMetadata& metadata = base();
+  auto schema_result = metadata.Schema();
+  if (!schema_result.has_value()) return;
+  auto spec_result = metadata.PartitionSpecById(manifest.partition_spec_id);
+  if (!spec_result.has_value()) return;
+
+  auto reader_result =
+      ManifestReader::Make(manifest, file_io, schema_result.value(), spec_result.value());
+  if (!reader_result.has_value()) return;
+
+  auto entries_result = reader_result.value()->Entries();
+  if (!entries_result.has_value()) return;
+
+  for (const auto& entry : entries_result.value()) {
+    if (entry.data_file) {
+      if (erase) {
+        file_paths.erase(entry.data_file->file_path);
+      } else {
+        file_paths.insert(entry.data_file->file_path);
+      }
+    }
+  }
+}
+
+// Find the ManifestFile object for a given path by scanning all snapshots.
+// Returns true if found and invokes the callback with the manifest.
+bool ExpireSnapshots::FindManifestByPath(
+    const std::string& manifest_path, std::shared_ptr<FileIO> file_io,
+    const std::function<void(const ManifestFile&)>& callback) {
+  for (const auto& snapshot : base().snapshots) {
+    if (!snapshot) continue;
+    SnapshotCache snapshot_cache(snapshot.get());
+    auto manifests_result = snapshot_cache.Manifests(file_io);
+    if (!manifests_result.has_value()) continue;
+
+    for (const auto& manifest : manifests_result.value()) {
+      if (manifest.manifest_path == manifest_path) {
+        callback(manifest);
+        return true;
+      }
+    }
+  }
+  return false;
+}
+
 Status ExpireSnapshots::FindDataFilesToDelete(
     const std::unordered_set<std::string>& manifests_to_delete,
     const std::unordered_set<std::string>& retained_manifests,
     std::unordered_set<std::string>& data_files_to_delete) {
-  const TableMetadata& metadata = base();
-  auto file_io = transaction_->table()->io();
+  auto file_io = GetFileIO();
 
   // Step 1: Collect all file paths from manifests being deleted
   for (const auto& manifest_path : manifests_to_delete) {
-    // Find the ManifestFile for this path by scanning expired snapshots
-    for (const auto& snapshot : metadata.snapshots) {
-      if (!snapshot) continue;
-      SnapshotCache snapshot_cache(snapshot.get());
-      auto manifests_result = snapshot_cache.Manifests(file_io);
-      if (!manifests_result.has_value()) continue;
-
-      for (const auto& manifest : manifests_result.value()) {
-        if (manifest.manifest_path != manifest_path) continue;
-
-        auto schema_result = metadata.Schema();
-        if (!schema_result.has_value()) continue;
-        auto spec_result = metadata.PartitionSpecById(manifest.partition_spec_id);
-        if (!spec_result.has_value()) continue;
-
-        auto reader_result = ManifestReader::Make(
-            manifest, file_io, schema_result.value(), spec_result.value());
-        if (!reader_result.has_value()) continue;
-
-        auto entries_result = reader_result.value()->Entries();
-        if (!entries_result.has_value()) continue;
-
-        for (const auto& entry : entries_result.value()) {
-          if (entry.data_file) {
-            data_files_to_delete.insert(entry.data_file->file_path);
-          }
-        }
-        goto next_manifest;  // Found and processed this manifest, move to next
-      }
-    }
-  next_manifest:;
+    FindManifestByPath(manifest_path, file_io, [&](const ManifestFile& manifest) {
+      ReadDataFilesFromManifest(manifest, file_io, data_files_to_delete, /*erase=*/false);
+    });
   }
 
   if (data_files_to_delete.empty()) {
@@ -407,37 +425,9 @@ Status ExpireSnapshots::FindDataFilesToDelete(
   // This ensures we don't delete files that are shared across manifests.
   for (const auto& manifest_path : retained_manifests) {
     if (data_files_to_delete.empty()) break;
-
-    for (const auto& snapshot : metadata.snapshots) {
-      if (!snapshot) continue;
-      SnapshotCache snapshot_cache(snapshot.get());
-      auto manifests_result = snapshot_cache.Manifests(file_io);
-      if (!manifests_result.has_value()) continue;
-
-      for (const auto& manifest : manifests_result.value()) {
-        if (manifest.manifest_path != manifest_path) continue;
-
-        auto schema_result = metadata.Schema();
-        if (!schema_result.has_value()) continue;
-        auto spec_result = metadata.PartitionSpecById(manifest.partition_spec_id);
-        if (!spec_result.has_value()) continue;
-
-        auto reader_result = ManifestReader::Make(
-            manifest, file_io, schema_result.value(), spec_result.value());
-        if (!reader_result.has_value()) continue;
-
-        auto entries_result = reader_result.value()->Entries();
-        if (!entries_result.has_value()) continue;
-
-        for (const auto& entry : entries_result.value()) {
-          if (entry.data_file) {
-            data_files_to_delete.erase(entry.data_file->file_path);
-          }
-        }
-        goto next_retained;
-      }
-    }
-  next_retained:;
+    FindManifestByPath(manifest_path, file_io, [&](const ManifestFile& manifest) {
+      ReadDataFilesFromManifest(manifest, file_io, data_files_to_delete, /*erase=*/true);
+    });
   }
 
   return {};

src/iceberg/update/expire_snapshots.h

@@ -190,8 +190,10 @@ class ICEBERG_EXPORT ExpireSnapshots : public PendingUpdate {
   /// TODO(shangxinli): Add IncrementalFileCleanup strategy for linear ancestry.
   Status CleanExpiredFiles(const std::vector<int64_t>& expired_snapshot_ids);
 
-  /// \brief Read manifest paths from a single snapshot.
-  /// Best-effort: returns OK even if the snapshot or its manifests can't be read.
+  /// \brief Get the FileIO from the transaction.
+  std::shared_ptr<FileIO> GetFileIO() const;
+
+  /// \brief Read manifest paths from a single snapshot (best-effort).
   Status ReadManifestsForSnapshot(int64_t snapshot_id,
                                   std::unordered_set<std::string>& manifest_paths);
 
@@ -201,8 +203,17 @@ class ICEBERG_EXPORT ExpireSnapshots : public PendingUpdate {
                                const std::unordered_set<std::string>& retained_manifests,
                                std::unordered_set<std::string>& data_files_to_delete);
 
+  /// \brief Read data file paths from a manifest, inserting or erasing from the set.
+  void ReadDataFilesFromManifest(const ManifestFile& manifest,
+                                 std::shared_ptr<FileIO> file_io,
+                                 std::unordered_set<std::string>& file_paths, bool erase);
+
+  /// \brief Find a ManifestFile by path across all snapshots, invoke callback if found.
+  bool FindManifestByPath(const std::string& manifest_path,
+                          std::shared_ptr<FileIO> file_io,
+                          const std::function<void(const ManifestFile&)>& callback);
+
   /// \brief Delete a file, suppressing errors (best-effort).
-  /// Uses the custom delete function if set, otherwise FileIO::DeleteFile.
   void DeleteFilePath(const std::string& path);
 
  private: