diff --git a/cpp/src/arrow/filesystem/gcsfs.cc b/cpp/src/arrow/filesystem/gcsfs.cc index 0287b4d58410f..58bbbbfd06cea 100644 --- 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(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(other); - return impl_ == fs.impl_; + return impl_->options().Equals(fs.impl_->options()); } Result GcsFileSystem::GetFileInfo(const std::string& path) { @@ -133,6 +141,7 @@ GcsFileSystem::GcsFileSystem(const GcsOptions& options, const io::IOContext& con namespace internal { std::shared_ptr MakeGcsFileSystemForTest(const GcsOptions& options) { + // Cannot use `std::make_shared<>` as the constructor is private. return std::shared_ptr( new GcsFileSystem(options, io::default_io_context())); } diff --git a/cpp/src/arrow/filesystem/gcsfs.h b/cpp/src/arrow/filesystem/gcsfs.h index 6dd4fe517b111..2583bdee8207c 100644 --- 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 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 GetFileInfo(const std::string& path) override; Result 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 index e1522c1b7dae6..5d8c7b5a40a39 100644 --- 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