kou commented on code in PR #35701: URL: https://github.com/apache/arrow/pull/35701#discussion_r1249804784
########## cpp/src/arrow/filesystem/azurefs.h: ########## @@ -0,0 +1,152 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#pragma once + +#include <memory> +#include <string> +#include <vector> + +#include "arrow/filesystem/filesystem.h" +#include "arrow/util/macros.h" +#include "arrow/util/uri.h" + +namespace Azure { +namespace Core { +namespace Credentials { + +class TokenCredential; + +} // namespace Credentials +} // namespace Core +namespace Storage { + +class StorageSharedKeyCredential; + +} // namespace Storage +} // namespace Azure + +namespace arrow { +namespace fs { + +enum class AzureCredentialsKind : int8_t { + /// Anonymous access (no credentials used), public + Anonymous, + /// Use explicitly-provided access key pair + StorageCredentials, + /// Use ServicePrincipleCredentials + ServicePrincipleCredentials, + /// Use Sas Token to authenticate + Sas, + /// Use Connection String + ConnectionString +}; + +/// Options for the AzureFileSystem implementation. +struct ARROW_EXPORT AzureOptions { + std::string scheme; + std::string account_dfs_url; + std::string account_blob_url; + bool is_azurite = false; + AzureCredentialsKind credentials_kind = AzureCredentialsKind::Anonymous; + + std::string sas_token; + std::string connection_string; + std::shared_ptr<Azure::Storage::StorageSharedKeyCredential> + storage_credentials_provider; + std::shared_ptr<Azure::Core::Credentials::TokenCredential> + service_principle_credentials_provider; + + AzureOptions(); + + bool Equals(const AzureOptions& other) const; +}; Review Comment: How about removing `AzureOptions` related codes from this PR to merge this PR quickly? ```diff diff --git a/cpp/src/arrow/filesystem/azurefs.cc b/cpp/src/arrow/filesystem/azurefs.cc index 0158c0cec..9fdf903b0 100644 --- a/cpp/src/arrow/filesystem/azurefs.cc +++ b/cpp/src/arrow/filesystem/azurefs.cc @@ -23,43 +23,21 @@ namespace arrow { namespace fs { -// ----------------------------------------------------------------------- -// AzureOptions Implementation - -AzureOptions::AzureOptions() {} - -bool AzureOptions::Equals(const AzureOptions& other) const { - return (account_dfs_url == other.account_dfs_url && - account_blob_url == other.account_blob_url && - credentials_kind == other.credentials_kind); -} - // ----------------------------------------------------------------------- // AzureFilesystem Implementation class AzureFileSystem::Impl { public: io::IOContext io_context_; - bool is_hierarchical_namespace_enabled_; - AzureOptions options_; - explicit Impl(AzureOptions options, io::IOContext io_context) - : io_context_(io_context), options_(std::move(options)) {} + explicit Impl(io::IOContext io_context) + : io_context_(io_context) {} Status Init() { - if (options_.backend == AzureBackend::Azurite) { - // gen1Client_->GetAccountInfo().Value.IsHierarchicalNamespaceEnabled - // throws error in azurite - is_hierarchical_namespace_enabled_ = false; - } return Status::OK(); } - - const AzureOptions& options() const { return options_; } }; -const AzureOptions& AzureFileSystem::options() const { return impl_->options(); } - bool AzureFileSystem::Equals(const FileSystem& other) const { if (this == &other) { return true; @@ -67,8 +45,7 @@ bool AzureFileSystem::Equals(const FileSystem& other) const { if (other.type_name() != type_name()) { return false; } - const auto& azure_fs = ::arrow::internal::checked_cast<const AzureFileSystem&>(other); - return options().Equals(azure_fs.options()); + return true; } Result<FileInfo> AzureFileSystem::GetFileInfo(const std::string& path) { @@ -138,15 +115,14 @@ Result<std::shared_ptr<io::OutputStream>> AzureFileSystem::OpenAppendStream( } Result<std::shared_ptr<AzureFileSystem>> AzureFileSystem::Make( - const AzureOptions& options, const io::IOContext& io_context) { - std::shared_ptr<AzureFileSystem> ptr(new AzureFileSystem(options, io_context)); + const io::IOContext& io_context) { + std::shared_ptr<AzureFileSystem> ptr(new AzureFileSystem(io_context)); RETURN_NOT_OK(ptr->impl_->Init()); return ptr; } -AzureFileSystem::AzureFileSystem(const AzureOptions& options, - const io::IOContext& io_context) - : FileSystem(io_context), impl_(std::make_unique<Impl>(options, io_context)) { +AzureFileSystem::AzureFileSystem(const io::IOContext& io_context) + : FileSystem(io_context), impl_(std::make_unique<Impl>(io_context)) { default_async_is_sync_ = false; } diff --git a/cpp/src/arrow/filesystem/azurefs.h b/cpp/src/arrow/filesystem/azurefs.h index 45087668d..31c014dcb 100644 --- a/cpp/src/arrow/filesystem/azurefs.h +++ b/cpp/src/arrow/filesystem/azurefs.h @@ -25,63 +25,9 @@ #include "arrow/util/macros.h" #include "arrow/util/uri.h" -namespace Azure { -namespace Core { -namespace Credentials { - -class TokenCredential; - -} // namespace Credentials -} // namespace Core -namespace Storage { - -class StorageSharedKeyCredential; - -} // namespace Storage -} // namespace Azure - namespace arrow { namespace fs { -enum class AzureCredentialsKind : int8_t { - /// Anonymous access (no credentials used), public - Anonymous, - /// Use explicitly-provided access key pair - StorageCredentials, - /// Use ServicePrincipleCredentials - ServicePrincipleCredentials, - /// Use Sas Token to authenticate - Sas, - /// Use Connection String - ConnectionString -}; - -enum class AzureBackend : bool { - /// Official Azure Remote Backend - Azure, - /// Local Simulated Storage - Azurite -}; - -/// Options for the AzureFileSystem implementation. -struct ARROW_EXPORT AzureOptions { - std::string account_dfs_url; - std::string account_blob_url; - AzureBackend backend = AzureBackend::Azure; - AzureCredentialsKind credentials_kind = AzureCredentialsKind::Anonymous; - - std::string sas_token; - std::string connection_string; - std::shared_ptr<Azure::Storage::StorageSharedKeyCredential> - storage_credentials_provider; - std::shared_ptr<Azure::Core::Credentials::TokenCredential> - service_principle_credentials_provider; - - AzureOptions(); - - bool Equals(const AzureOptions& other) const; -}; - /// \brief Azure-backed FileSystem implementation for ABFS and ADLS. /// /// ABFS (Azure Blob Storage - https://azure.microsoft.com/en-us/products/storage/blobs/) @@ -94,7 +40,7 @@ struct ARROW_EXPORT AzureOptions { /// compatibility. Gen1 exists as a separate object that will retired /// on Feb 29, 2024. New ADLS accounts will use Gen2 instead, which is /// implemented on top of ABFS. -/// +/// /// TODO: GH-18014 Complete the internal implementation /// and review the documentation class ARROW_EXPORT AzureFileSystem : public FileSystem { @@ -103,9 +49,6 @@ class ARROW_EXPORT AzureFileSystem : public FileSystem { std::string type_name() const override { return "abfs"; } - /// Return the original Azure options when constructing the filesystem - const AzureOptions& options() const; - bool Equals(const FileSystem& other) const override; Result<FileInfo> GetFileInfo(const std::string& path) override; @@ -146,10 +89,10 @@ class ARROW_EXPORT AzureFileSystem : public FileSystem { const std::shared_ptr<const KeyValueMetadata>& metadata = {}) override; static Result<std::shared_ptr<AzureFileSystem>> Make( - const AzureOptions& options, const io::IOContext& = io::default_io_context()); + const io::IOContext& = io::default_io_context()); private: - explicit AzureFileSystem(const AzureOptions& options, const io::IOContext& io_context); + explicit AzureFileSystem(const io::IOContext& io_context); class Impl; std::unique_ptr<Impl> impl_; diff --git a/cpp/src/arrow/filesystem/azurefs_test.cc b/cpp/src/arrow/filesystem/azurefs_test.cc index 0f03e8839..d7f97d1e4 100644 --- a/cpp/src/arrow/filesystem/azurefs_test.cc +++ b/cpp/src/arrow/filesystem/azurefs_test.cc @@ -34,11 +34,9 @@ using ::testing::IsEmpty; using ::testing::Not; using ::testing::NotNull; -// Placeholder test for file structure -// TODO: GH-18014 Remove once a proper test is added -TEST(AzureFileSystem, OptionsCompare) { - AzureOptions options; - EXPECT_TRUE(options.Equals(options)); +TEST(AzureFileSystem, type_name) { + ASSERT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make()); + ASSERT_EQ(fs->type_name(), "abfs"); } } // namespace ``` -- 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]
