Address review comments

apache · Oct 7, 2021 · 6898c5a · 6898c5a
1 parent ef60b0e
commit 6898c5a
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 25 deletions.
diff --git a/cpp/src/arrow/filesystem/gcsfs.cc b/cpp/src/arrow/filesystem/gcsfs.cc
@@ -30,10 +30,10 @@ namespace fs {
 
 namespace gcs = google::cloud::storage;
 
-google::cloud::Options AsGoogleCloudOptions(GcsOptions const& o) {
+google::cloud::Options AsGoogleCloudOptions(const GcsOptions& o) {
   auto options = google::cloud::Options{};
   if (!o.endpoint_override.empty()) {
-    auto scheme = o.scheme;
+    std::string scheme = o.scheme;
     if (scheme.empty()) scheme = "https";
     options.set<gcs::RestEndpointOption>(scheme + "://" + o.endpoint_override);
   }
@@ -42,12 +42,20 @@ google::cloud::Options AsGoogleCloudOptions(GcsOptions const& o) {
 
 class GcsFileSystem::Impl {
  public:
-  explicit Impl(GcsOptions const& o) : client_(AsGoogleCloudOptions(o)) {}
+  explicit Impl(GcsOptions o)
+      : options_(std::move(o)), client_(AsGoogleCloudOptions(options_)) {}
+
+  GcsOptions const& options() const { return options_; }
 
  private:
+  GcsOptions options_;
   gcs::Client client_;
 };
 
+bool GcsOptions::Equals(const GcsOptions& other) const {
+  return endpoint_override == other.endpoint_override && scheme == other.scheme;
+}
+
 std::string GcsFileSystem::type_name() const { return "gcs"; }
 
 bool GcsFileSystem::Equals(const FileSystem& other) const {
@@ -58,7 +66,7 @@ bool GcsFileSystem::Equals(const FileSystem& other) const {
     return false;
   }
   const auto& fs = ::arrow::internal::checked_cast<const GcsFileSystem&>(other);
-  return impl_ == fs.impl_;
+  return impl_->options().Equals(fs.impl_->options());
 }
 
 Result<FileInfo> GcsFileSystem::GetFileInfo(const std::string& path) {
@@ -133,6 +141,7 @@ GcsFileSystem::GcsFileSystem(const GcsOptions& options, const io::IOContext& con
 namespace internal {
 
 std::shared_ptr<GcsFileSystem> MakeGcsFileSystemForTest(const GcsOptions& options) {
+  // Cannot use `std::make_shared<>` as the constructor is private.
   return std::shared_ptr<GcsFileSystem>(
       new GcsFileSystem(options, io::default_io_context()));
 }

diff --git a/cpp/src/arrow/filesystem/gcsfs.h b/cpp/src/arrow/filesystem/gcsfs.h
@@ -28,8 +28,7 @@ namespace fs {
 class GcsFileSystem;
 struct GcsOptions;
 namespace internal {
-// TODO(ARROW-1231) - during development only tests should create a GcsFileSystem.
-//     Remove, and provide a public API, before declaring the feature complete.
+// TODO(ARROW-1231) - remove, and provide a public API (static GcsFileSystem::Make()).
 std::shared_ptr<GcsFileSystem> MakeGcsFileSystemForTest(const GcsOptions& options);
 }  // namespace internal
 
@@ -39,17 +38,6 @@ struct ARROW_EXPORT GcsOptions {
   std::string scheme;
 
   bool Equals(const GcsOptions& other) const;
-
-  /// \brief Initialize with default credentials provider chain
-  ///
-  /// This is recommended if you use the standard GCP environment variables
-  /// and/or configuration file.
-  static GcsOptions Defaults();
-
-  /// \brief Initialize with anonymous credentials.
-  ///
-  /// This will only let you access public buckets.
-  static GcsOptions Anonymous();
 };
 
 /// \brief GCS-backed FileSystem implementation.
@@ -81,7 +69,7 @@ class ARROW_EXPORT GcsFileSystem : public FileSystem {
   Result<FileInfo> GetFileInfo(const std::string& path) override;
   Result<FileInfoVector> GetFileInfo(const FileSelector& select) override;
 
-  Status CreateDir(const std::string& path, bool recursive = true) override;
+  Status CreateDir(const std::string& path, bool recursive) override;
 
   Status DeleteDir(const std::string& path) override;
 

diff --git a/cpp/src/arrow/filesystem/gcsfs_test.cc b/cpp/src/arrow/filesystem/gcsfs_test.cc
@@ -34,16 +34,29 @@ using ::testing::IsEmpty;
 using ::testing::Not;
 using ::testing::NotNull;
 
-TEST(GCSFileSystem, Compare) {
+TEST(GcsFileSystem, OptionsCompare) {
+  GcsOptions a;
+  GcsOptions b;
+  b.endpoint_override = "localhost:1234";
+  EXPECT_TRUE(a.Equals(a));
+  EXPECT_TRUE(b.Equals(b));
+  auto c = b;
+  c.scheme = "http";
+  EXPECT_FALSE(b.Equals(c));
+}
+
+TEST(GcsFileSystem, FileSystemCompare) {
   auto a = internal::MakeGcsFileSystemForTest(GcsOptions{});
-  EXPECT_THAT(a.get(), NotNull());
-  EXPECT_EQ(a, a);
+  EXPECT_THAT(a, NotNull());
+  EXPECT_TRUE(a->Equals(*a));
 
-  auto b = internal::MakeGcsFileSystemForTest(GcsOptions{});
-  EXPECT_THAT(b.get(), NotNull());
-  EXPECT_EQ(b, b);
+  GcsOptions options;
+  options.endpoint_override = "localhost:1234";
+  auto b = internal::MakeGcsFileSystemForTest(options);
+  EXPECT_THAT(b, NotNull());
+  EXPECT_TRUE(b->Equals(*b));
 
-  EXPECT_NE(a, b);
+  EXPECT_FALSE(a->Equals(*b));
 }
 
 }  // namespace