polish design and refine impl

Author: wgtmac

src/iceberg/arrow/arrow_io.cc

@@ -22,7 +22,6 @@
 #include <limits>
 #include <mutex>
 #include <optional>
-#include <unordered_map>
 #include <vector>
 
 #include <arrow/buffer.h>
@@ -36,8 +35,6 @@
 #include "iceberg/arrow/arrow_io_internal.h"
 #include "iceberg/arrow/arrow_io_util.h"
 #include "iceberg/arrow/arrow_status_internal.h"
-#include "iceberg/arrow/s3/arrow_s3_internal.h"
-#include "iceberg/util/location_util.h"
 #include "iceberg/util/macros.h"
 
 namespace iceberg::arrow {
@@ -486,53 +483,13 @@ class ArrowOutputFile : public OutputFile {
 
 }  // namespace
 
-const std::shared_ptr<::arrow::fs::FileSystem>& ArrowFileSystemFileIO::FileSystemForPath(
-    std::string_view location) const {
-  if (fs_by_prefix_.empty()) {
-    return arrow_fs_;
-  }
-  // Longest matching prefix wins; fall back to the default file system.
-  const std::string canonical = LocationUtil::CanonicalizeS3Scheme(location);
-  const std::shared_ptr<::arrow::fs::FileSystem>* best = &arrow_fs_;
-  size_t best_len = 0;
-  for (const auto& [prefix, fs] : fs_by_prefix_) {
-    if (prefix.size() > best_len && LocationUtil::PathHasPrefix(canonical, prefix)) {
-      best = &fs;
-      best_len = prefix.size();
-    }
-  }
-  return *best;
-}
-
-Status ArrowFileSystemFileIO::SetStorageCredentials(
-    const std::vector<
-        std::pair<std::string, std::unordered_map<std::string, std::string>>>&
-        properties_by_prefix) {
-#if ICEBERG_S3_ENABLED
-  std::vector<std::pair<std::string, std::shared_ptr<::arrow::fs::FileSystem>>>
-      fs_by_prefix;
-  fs_by_prefix.reserve(properties_by_prefix.size());
-  for (const auto& [prefix, properties] : properties_by_prefix) {
-    ICEBERG_ASSIGN_OR_RAISE(auto fs, BuildArrowS3FileSystem(properties));
-    fs_by_prefix.emplace_back(prefix, std::move(fs));
-  }
-  fs_by_prefix_ = std::move(fs_by_prefix);
-  return {};
-#else
-  (void)properties_by_prefix;
-  return NotSupported("S3 storage credentials require Arrow S3 support");
-#endif
-}
-
-Result<std::string> ArrowFileSystemFileIO::ResolvePath(
-    const std::shared_ptr<::arrow::fs::FileSystem>& fs,
-    const std::string& file_location) {
+Result<std::string> ArrowFileSystemFileIO::ResolvePath(const std::string& file_location) {
   const auto pos = file_location.find("://");
   if (pos == std::string::npos) {
     return file_location;
   }
 
-  auto path = fs->PathFromUri(file_location);
+  auto path = arrow_fs_->PathFromUri(file_location);
   if (path.ok()) {
     return std::move(path).ValueOrDie();
   }
@@ -555,14 +512,14 @@ Result<std::shared_ptr<::arrow::io::RandomAccessFile>> OpenArrowInputStream(
   ICEBERG_PRECHECK(io != nullptr, "FileIO cannot be null");
 
   if (auto arrow_io = std::dynamic_pointer_cast<ArrowFileSystemFileIO>(io)) {
-    const auto& fs = arrow_io->FileSystemForPath(path);
-    ICEBERG_ASSIGN_OR_RAISE(auto resolved_path, arrow_io->ResolvePath(fs, path));
+    ICEBERG_ASSIGN_OR_RAISE(auto resolved_path, arrow_io->ResolvePath(path));
     ::arrow::fs::FileInfo file_info(resolved_path, ::arrow::fs::FileType::File);
     if (length.has_value()) {
       ICEBERG_ASSIGN_OR_RAISE(auto size, ToInt64Length(*length));
       file_info.set_size(size);
     }
-    ICEBERG_ARROW_ASSIGN_OR_RETURN(auto input, fs->OpenInputFile(file_info));
+    ICEBERG_ARROW_ASSIGN_OR_RETURN(auto input,
+                                   arrow_io->arrow_fs_->OpenInputFile(file_info));
     return input;
   }
 
@@ -586,15 +543,16 @@ Result<std::shared_ptr<::arrow::io::OutputStream>> OpenArrowOutputStream(
   ICEBERG_PRECHECK(io != nullptr, "FileIO cannot be null");
 
   if (auto arrow_io = std::dynamic_pointer_cast<ArrowFileSystemFileIO>(io)) {
-    const auto& fs = arrow_io->FileSystemForPath(path);
-    ICEBERG_ASSIGN_OR_RAISE(auto resolved_path, arrow_io->ResolvePath(fs, path));
+    ICEBERG_ASSIGN_OR_RAISE(auto resolved_path, arrow_io->ResolvePath(path));
     if (!overwrite) {
-      ICEBERG_ARROW_ASSIGN_OR_RETURN(auto info, fs->GetFileInfo(resolved_path));
+      ICEBERG_ARROW_ASSIGN_OR_RETURN(auto info,
+                                     arrow_io->arrow_fs_->GetFileInfo(resolved_path));
       if (info.type() != ::arrow::fs::FileType::NotFound) {
         return AlreadyExists("File already exists: {}", path);
       }
     }
-    ICEBERG_ARROW_ASSIGN_OR_RETURN(auto output, fs->OpenOutputStream(resolved_path));
+    ICEBERG_ARROW_ASSIGN_OR_RETURN(auto output,
+                                   arrow_io->arrow_fs_->OpenOutputStream(resolved_path));
     return output;
   }
 
@@ -610,60 +568,42 @@ Result<std::shared_ptr<::arrow::io::OutputStream>> OpenArrowOutputStream(
 
 Result<std::unique_ptr<InputFile>> ArrowFileSystemFileIO::NewInputFile(
     std::string file_location) {
-  const auto& fs = FileSystemForPath(file_location);
-  ICEBERG_ASSIGN_OR_RAISE(auto path, ResolvePath(fs, file_location));
-  return std::make_unique<ArrowInputFile>(fs, std::move(file_location), std::move(path),
-                                          std::nullopt);
+  ICEBERG_ASSIGN_OR_RAISE(auto path, ResolvePath(file_location));
+  return std::make_unique<ArrowInputFile>(arrow_fs_, std::move(file_location),
+                                          std::move(path), std::nullopt);
 }
 
 Result<std::unique_ptr<InputFile>> ArrowFileSystemFileIO::NewInputFile(
     std::string file_location, size_t length) {
   ICEBERG_ASSIGN_OR_RAISE(auto size, ToInt64Length(length));
-  const auto& fs = FileSystemForPath(file_location);
-  ICEBERG_ASSIGN_OR_RAISE(auto path, ResolvePath(fs, file_location));
-  return std::make_unique<ArrowInputFile>(fs, std::move(file_location), std::move(path),
-                                          size);
+  ICEBERG_ASSIGN_OR_RAISE(auto path, ResolvePath(file_location));
+  return std::make_unique<ArrowInputFile>(arrow_fs_, std::move(file_location),
+                                          std::move(path), size);
 }
 
 Result<std::unique_ptr<OutputFile>> ArrowFileSystemFileIO::NewOutputFile(
     std::string file_location) {
-  const auto& fs = FileSystemForPath(file_location);
-  ICEBERG_ASSIGN_OR_RAISE(auto path, ResolvePath(fs, file_location));
-  return std::make_unique<ArrowOutputFile>(fs, std::move(file_location), std::move(path));
+  ICEBERG_ASSIGN_OR_RAISE(auto path, ResolvePath(file_location));
+  return std::make_unique<ArrowOutputFile>(arrow_fs_, std::move(file_location),
+                                           std::move(path));
 }
 
 /// \brief Delete a file at the given location.
 Status ArrowFileSystemFileIO::DeleteFile(const std::string& file_location) {
-  const auto& fs = FileSystemForPath(file_location);
-  ICEBERG_ASSIGN_OR_RAISE(auto path, ResolvePath(fs, file_location));
-  ICEBERG_ARROW_RETURN_NOT_OK(fs->DeleteFile(path));
+  ICEBERG_ASSIGN_OR_RAISE(auto path, ResolvePath(file_location));
+  ICEBERG_ARROW_RETURN_NOT_OK(arrow_fs_->DeleteFile(path));
   return {};
 }
 
 Status ArrowFileSystemFileIO::DeleteFiles(
     const std::vector<std::string>& file_locations) {
-  if (fs_by_prefix_.empty()) {
-    // No per-prefix routing: one batched delete on the default file system.
-    std::vector<std::string> paths;
-    paths.reserve(file_locations.size());
-    for (const auto& file_location : file_locations) {
-      ICEBERG_ASSIGN_OR_RAISE(auto path, ResolvePath(arrow_fs_, file_location));
-      paths.push_back(std::move(path));
-    }
-    ICEBERG_ARROW_RETURN_NOT_OK(arrow_fs_->DeleteFiles(paths));
-    return {};
-  }
-
-  // Paths may route to different file systems: group by fs, then batch per fs.
-  std::unordered_map<::arrow::fs::FileSystem*, std::vector<std::string>> paths_by_fs;
+  std::vector<std::string> paths;
+  paths.reserve(file_locations.size());
   for (const auto& file_location : file_locations) {
-    const auto& fs = FileSystemForPath(file_location);
-    ICEBERG_ASSIGN_OR_RAISE(auto path, ResolvePath(fs, file_location));
-    paths_by_fs[fs.get()].push_back(std::move(path));
-  }
-  for (auto& [fs, paths] : paths_by_fs) {
-    ICEBERG_ARROW_RETURN_NOT_OK(fs->DeleteFiles(paths));
+    ICEBERG_ASSIGN_OR_RAISE(auto path, ResolvePath(file_location));
+    paths.push_back(std::move(path));
   }
+  ICEBERG_ARROW_RETURN_NOT_OK(arrow_fs_->DeleteFiles(paths));
   return {};
 }
 

src/iceberg/arrow/arrow_io_internal.h

@@ -23,9 +23,6 @@
 #include <memory>
 #include <optional>
 #include <string>
-#include <string_view>
-#include <unordered_map>
-#include <utility>
 #include <vector>
 
 #include <arrow/filesystem/type_fwd.h>
@@ -55,8 +52,7 @@ OpenArrowOutputStream(const std::shared_ptr<FileIO>& io, const std::string& path
                       bool overwrite = true);
 
 /// \brief A concrete implementation of FileIO for Arrow file system.
-class ICEBERG_BUNDLE_EXPORT ArrowFileSystemFileIO : public FileIO,
-                                                    public SupportsStorageCredentials {
+class ICEBERG_BUNDLE_EXPORT ArrowFileSystemFileIO : public FileIO {
  public:
   explicit ArrowFileSystemFileIO(std::shared_ptr<::arrow::fs::FileSystem> arrow_fs)
       : arrow_fs_(std::move(arrow_fs)) {}
@@ -85,12 +81,6 @@ class ICEBERG_BUNDLE_EXPORT ArrowFileSystemFileIO : public FileIO,
   /// \brief Delete files at the given locations.
   Status DeleteFiles(const std::vector<std::string>& file_locations) override;
 
-  /// \brief Build one S3 file system per credential prefix for per-path routing.
-  Status SetStorageCredentials(
-      const std::vector<
-          std::pair<std::string, std::unordered_map<std::string, std::string>>>&
-          properties_by_prefix) override;
-
   /// \brief Get the Arrow file system.
   const std::shared_ptr<::arrow::fs::FileSystem>& fs() const { return arrow_fs_; }
 
@@ -102,18 +92,10 @@ class ICEBERG_BUNDLE_EXPORT ArrowFileSystemFileIO : public FileIO,
   friend Result<std::shared_ptr<::arrow::io::OutputStream>> OpenArrowOutputStream(
       const std::shared_ptr<FileIO>& io, const std::string& path, bool overwrite);
 
-  /// \brief Pick the file system for `location` (longest matching prefix, else
-  /// the default).
-  const std::shared_ptr<::arrow::fs::FileSystem>& FileSystemForPath(
-      std::string_view location) const;
-
-  /// \brief Resolve a file location to a filesystem path using `fs`.
-  Result<std::string> ResolvePath(const std::shared_ptr<::arrow::fs::FileSystem>& fs,
-                                  const std::string& file_location);
+  /// \brief Resolve a file location to a filesystem path.
+  Result<std::string> ResolvePath(const std::string& file_location);
 
   std::shared_ptr<::arrow::fs::FileSystem> arrow_fs_;
-  std::vector<std::pair<std::string, std::shared_ptr<::arrow::fs::FileSystem>>>
-      fs_by_prefix_;
 };
 
 }  // namespace iceberg::arrow