kou commented on code in PR #38505:
URL: https://github.com/apache/arrow/pull/38505#discussion_r1382880057
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -179,12 +180,14 @@ class ObjectInputFile final : public io::RandomAccessFile
{
metadata_ = GetObjectMetadata(properties.Value.Metadata);
return Status::OK();
} catch (const Azure::Storage::StorageException& exception) {
- if (exception.StatusCode == Azure::Core::Http::HttpStatusCode::NotFound)
{
- // Could be either container or blob not found.
+ if (ContainerOrBlobNotFound(exception)) {
return PathNotFound(path_);
}
- return ErrorToStatus(
- "When fetching properties for '" + blob_client_->GetUrl() + "': ",
exception);
+ return internal::ErrorToStatus(
+ "GetProperties failed for '" + blob_client_->GetUrl() +
+ "' with an unexpected Azure error. Can not initialise an
ObjectInputFile "
+ "without knowing the file size. ",
Review Comment:
```suggestion
"without knowing the file size.",
```
##########
cpp/src/arrow/filesystem/azurefs_internal.h:
##########
@@ -0,0 +1,47 @@
+// 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 <optional>
+
+#include <azure/storage/files/datalake.hpp>
+
+#include "arrow/result.h"
+
+namespace arrow {
+namespace fs {
+namespace internal {
Review Comment:
```suggestion
namespace arrow::fs::internal {
```
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -317,27 +321,136 @@ class ObjectInputFile final : public
io::RandomAccessFile {
class AzureFileSystem::Impl {
public:
io::IOContext io_context_;
- std::shared_ptr<Azure::Storage::Blobs::BlobServiceClient> service_client_;
+ std::shared_ptr<Azure::Storage::Files::DataLake::DataLakeServiceClient>
+ datalake_service_client_;
+ std::shared_ptr<Azure::Storage::Blobs::BlobServiceClient>
blob_service_client_;
AzureOptions options_;
+ internal::HierarchicalNamespaceDetector hierarchical_namespace_;
explicit Impl(AzureOptions options, io::IOContext io_context)
: io_context_(io_context), options_(std::move(options)) {}
Status Init() {
- service_client_ =
std::make_shared<Azure::Storage::Blobs::BlobServiceClient>(
+ blob_service_client_ =
std::make_shared<Azure::Storage::Blobs::BlobServiceClient>(
options_.account_blob_url, options_.storage_credentials_provider);
+ datalake_service_client_ =
+
std::make_shared<Azure::Storage::Files::DataLake::DataLakeServiceClient>(
+ options_.account_dfs_url, options_.storage_credentials_provider);
+ RETURN_NOT_OK(hierarchical_namespace_.Init(datalake_service_client_));
return Status::OK();
}
const AzureOptions& options() const { return options_; }
+ public:
+ Result<FileInfo> GetFileInfo(const AzurePath& path) {
+ FileInfo info;
+ info.set_path(path.full_path);
+
+ if (path.container.empty()) {
+ DCHECK(path.path_to_file.empty()); // The path is invalid if the
container is empty
+ // but not path_to_file.
+ // path must refer to the root of the Azure storage account. This is a
directory,
+ // and there isn't any extra metadata to fetch.
+ return FileInfo(path.full_path, FileType::Directory);
Review Comment:
```suggestion
info.set_type(FileType::Directory);
return info;
```
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -137,34 +141,37 @@ TEST(AzureFileSystem, OptionsCompare) {
EXPECT_TRUE(options.Equals(options));
}
-class TestAzureFileSystem : public ::testing::Test {
+class AzureFileSystemTest : public ::testing::Test {
public:
std::shared_ptr<FileSystem> fs_;
- std::shared_ptr<Azure::Storage::Blobs::BlobServiceClient> service_client_;
+ std::shared_ptr<Azure::Storage::Blobs::BlobServiceClient>
blob_service_client_;
+ std::shared_ptr<Azure::Storage::Files::DataLake::DataLakeServiceClient>
+ datalake_service_client_;
+ AzureOptions options_;
std::mt19937_64 generator_;
std::string container_name_;
+ bool suite_skipped_ = false;
- TestAzureFileSystem() : generator_(std::random_device()()) {}
+ AzureFileSystemTest() : generator_(std::random_device()()) {}
- AzureOptions MakeOptions() {
- const std::string& account_name = GetAzuriteEnv()->account_name();
- const std::string& account_key = GetAzuriteEnv()->account_key();
- AzureOptions options;
- options.backend = AzureBackend::Azurite;
- ARROW_EXPECT_OK(options.ConfigureAccountKeyCredentials(account_name,
account_key));
- return options;
- }
+ virtual Result<AzureOptions> MakeOptions() = 0;
void SetUp() override {
- ASSERT_THAT(GetAzuriteEnv(), NotNull());
- ASSERT_OK(GetAzuriteEnv()->status());
-
- container_name_ = RandomChars(32);
auto options = MakeOptions();
- service_client_ =
std::make_shared<Azure::Storage::Blobs::BlobServiceClient>(
- options.account_blob_url, options.storage_credentials_provider);
- ASSERT_OK_AND_ASSIGN(fs_, AzureFileSystem::Make(options));
- auto container_client =
service_client_->GetBlobContainerClient(container_name_);
+ if (options.ok()) {
+ options_ = options.ValueOrDie();
Review Comment:
```suggestion
options_ = *options;
```
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -216,23 +226,184 @@ class TestAzureFileSystem : public ::testing::Test {
void UploadLines(const std::vector<std::string>& lines, const char*
path_to_file,
int total_size) {
// TODO(GH-38333): Switch to using Azure filesystem to write once its
implemented.
- auto blob_client =
service_client_->GetBlobContainerClient(PreexistingContainerName())
- .GetBlockBlobClient(path_to_file);
+ auto blob_client =
+
blob_service_client_->GetBlobContainerClient(PreexistingContainerName())
+ .GetBlockBlobClient(path_to_file);
std::string all_lines = std::accumulate(lines.begin(), lines.end(),
std::string(""));
blob_client.UploadFrom(reinterpret_cast<const uint8_t*>(all_lines.data()),
total_size);
}
+
+ void RunGetFileInfoObjectWithNestedStructureTest();
+ void RunGetFileInfoObjectTest();
};
-TEST_F(TestAzureFileSystem, OpenInputStreamString) {
+class AzuriteFileSystemTest : public AzureFileSystemTest {
+ Result<AzureOptions> MakeOptions() {
+ EXPECT_THAT(GetAzuriteEnv(), NotNull());
+ ARROW_EXPECT_OK(GetAzuriteEnv()->status());
+ AzureOptions options;
+ options.backend = AzureBackend::Azurite;
+ ARROW_EXPECT_OK(options.ConfigureAccountKeyCredentials(
+ GetAzuriteEnv()->account_name(), GetAzuriteEnv()->account_key()));
+ return options;
+ }
+};
+
+class AzureFlatNamespaceFileSystemTest : public AzureFileSystemTest {
+ Result<AzureOptions> MakeOptions() override {
+ AzureOptions options;
+ if (char* account_name = std::getenv("AZURE_FLAT_NAMESPACE_ACCOUNT_NAME"))
{
Review Comment:
```suggestion
if (const auto account_name =
std::getenv("AZURE_FLAT_NAMESPACE_ACCOUNT_NAME")) {
```
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -216,23 +226,184 @@ class TestAzureFileSystem : public ::testing::Test {
void UploadLines(const std::vector<std::string>& lines, const char*
path_to_file,
int total_size) {
// TODO(GH-38333): Switch to using Azure filesystem to write once its
implemented.
- auto blob_client =
service_client_->GetBlobContainerClient(PreexistingContainerName())
- .GetBlockBlobClient(path_to_file);
+ auto blob_client =
+
blob_service_client_->GetBlobContainerClient(PreexistingContainerName())
+ .GetBlockBlobClient(path_to_file);
std::string all_lines = std::accumulate(lines.begin(), lines.end(),
std::string(""));
blob_client.UploadFrom(reinterpret_cast<const uint8_t*>(all_lines.data()),
total_size);
}
+
+ void RunGetFileInfoObjectWithNestedStructureTest();
+ void RunGetFileInfoObjectTest();
};
-TEST_F(TestAzureFileSystem, OpenInputStreamString) {
+class AzuriteFileSystemTest : public AzureFileSystemTest {
+ Result<AzureOptions> MakeOptions() {
+ EXPECT_THAT(GetAzuriteEnv(), NotNull());
+ ARROW_EXPECT_OK(GetAzuriteEnv()->status());
+ AzureOptions options;
+ options.backend = AzureBackend::Azurite;
+ ARROW_EXPECT_OK(options.ConfigureAccountKeyCredentials(
+ GetAzuriteEnv()->account_name(), GetAzuriteEnv()->account_key()));
+ return options;
+ }
+};
+
+class AzureFlatNamespaceFileSystemTest : public AzureFileSystemTest {
+ Result<AzureOptions> MakeOptions() override {
+ AzureOptions options;
+ if (char* account_name = std::getenv("AZURE_FLAT_NAMESPACE_ACCOUNT_NAME"))
{
+ char* account_key = std::getenv("AZURE_FLAT_NAMESPACE_ACCOUNT_KEY");
+ EXPECT_THAT(account_key, NotNull());
+ ARROW_EXPECT_OK(options.ConfigureAccountKeyCredentials(account_name,
account_key));
+ return options;
+ }
+ return Status::Cancelled(
+ "Connection details not provided for a real flat namespace "
+ "account.");
+ }
+};
+
+class AzureHierarchicalNamespaceFileSystemTest : public AzureFileSystemTest {
+ Result<AzureOptions> MakeOptions() override {
+ AzureOptions options;
+ if (char* account_name =
std::getenv("AZURE_HIERARCHICAL_NAMESPACE_ACCOUNT_NAME")) {
+ char* account_key =
std::getenv("AZURE_HIERARCHICAL_NAMESPACE_ACCOUNT_KEY");
+ EXPECT_THAT(account_key, NotNull());
+ ARROW_EXPECT_OK(options.ConfigureAccountKeyCredentials(account_name,
account_key));
Review Comment:
dittto.
##########
cpp/src/arrow/filesystem/azurefs_internal.h:
##########
@@ -0,0 +1,47 @@
+// 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 <optional>
+
+#include <azure/storage/files/datalake.hpp>
+
+#include "arrow/result.h"
+
+namespace arrow {
+namespace fs {
+namespace internal {
+
+Status ErrorToStatus(const std::string& prefix,
+ const Azure::Storage::StorageException& exception);
+
+class HierarchicalNamespaceDetector {
+ public:
+ Status
Init(std::shared_ptr<Azure::Storage::Files::DataLake::DataLakeServiceClient>
+ datalake_service_client);
+ Result<bool> Enabled(const std::string& container_name);
+
+ private:
+ std::shared_ptr<Azure::Storage::Files::DataLake::DataLakeServiceClient>
+ datalake_service_client_;
+ std::optional<bool> is_hierarchical_namespace_enabled_;
Review Comment:
How about simplifying this? Because `hierarchical_namespace` is redundant in
`HierarchicalNamespaceDetector` class.
```suggestion
std::optional<bool> enabled_;
```
##########
cpp/src/arrow/filesystem/path_util.cc:
##########
@@ -191,12 +191,19 @@ std::string_view RemoveLeadingSlash(std::string_view key)
{
}
Status AssertNoTrailingSlash(std::string_view key) {
- if (key.back() == '/') {
+ if (HasTrailingSlash(key)) {
return NotAFile(key);
}
return Status::OK();
}
+bool HasTrailingSlash(std::string_view key) {
+ if (key.back() != '/') {
+ return false;
+ }
+ return true;
Review Comment:
```suggestion
return key.back() == '/';
```
(We may want to simplify `HasLeadingSlash()` too.)
##########
cpp/src/arrow/filesystem/azurefs_internal.cc:
##########
@@ -0,0 +1,93 @@
+// 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.
+
+#include "arrow/filesystem/azurefs_internal.h"
+
+#include <azure/storage/files/datalake.hpp>
+
+#include "arrow/result.h"
+
+namespace arrow {
+namespace fs {
+namespace internal {
Review Comment:
```suggestion
namespace arrow::fs::internal {
```
##########
cpp/src/arrow/filesystem/azurefs_internal.h:
##########
@@ -0,0 +1,47 @@
+// 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 <optional>
+
+#include <azure/storage/files/datalake.hpp>
+
+#include "arrow/result.h"
+
+namespace arrow {
+namespace fs {
+namespace internal {
+
+Status ErrorToStatus(const std::string& prefix,
Review Comment:
How about renaming this to `ExceptionToStatus()` because Azure SDK uses
`Exception` such as `StorageException`?
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -317,27 +321,136 @@ class ObjectInputFile final : public
io::RandomAccessFile {
class AzureFileSystem::Impl {
public:
io::IOContext io_context_;
- std::shared_ptr<Azure::Storage::Blobs::BlobServiceClient> service_client_;
+ std::shared_ptr<Azure::Storage::Files::DataLake::DataLakeServiceClient>
+ datalake_service_client_;
+ std::shared_ptr<Azure::Storage::Blobs::BlobServiceClient>
blob_service_client_;
Review Comment:
It seems that this isn't shared.
How about using `std::unique_ptr` instead?
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -308,12 +473,12 @@ TEST_F(TestAzureFileSystem, OpenInputStreamReadMetadata) {
std::shared_ptr<const KeyValueMetadata> actual;
ASSERT_OK_AND_ASSIGN(actual, stream->ReadMetadata());
- // TODO(GH-38330): This is asserting that the user defined metadata is
returned but this
- // is probably not the correct behaviour.
+ // TODO(GH-38330): This is asserting that the user defined metadata is
returned but
+ // this is probably not the correct behaviour.
Review Comment:
Could you revert this change?
This will be conflicted with #38524.
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -216,23 +226,184 @@ class TestAzureFileSystem : public ::testing::Test {
void UploadLines(const std::vector<std::string>& lines, const char*
path_to_file,
int total_size) {
// TODO(GH-38333): Switch to using Azure filesystem to write once its
implemented.
- auto blob_client =
service_client_->GetBlobContainerClient(PreexistingContainerName())
- .GetBlockBlobClient(path_to_file);
+ auto blob_client =
+
blob_service_client_->GetBlobContainerClient(PreexistingContainerName())
+ .GetBlockBlobClient(path_to_file);
std::string all_lines = std::accumulate(lines.begin(), lines.end(),
std::string(""));
blob_client.UploadFrom(reinterpret_cast<const uint8_t*>(all_lines.data()),
total_size);
}
+
+ void RunGetFileInfoObjectWithNestedStructureTest();
+ void RunGetFileInfoObjectTest();
};
-TEST_F(TestAzureFileSystem, OpenInputStreamString) {
+class AzuriteFileSystemTest : public AzureFileSystemTest {
+ Result<AzureOptions> MakeOptions() {
+ EXPECT_THAT(GetAzuriteEnv(), NotNull());
+ ARROW_EXPECT_OK(GetAzuriteEnv()->status());
+ AzureOptions options;
+ options.backend = AzureBackend::Azurite;
+ ARROW_EXPECT_OK(options.ConfigureAccountKeyCredentials(
+ GetAzuriteEnv()->account_name(), GetAzuriteEnv()->account_key()));
+ return options;
+ }
+};
+
+class AzureFlatNamespaceFileSystemTest : public AzureFileSystemTest {
+ Result<AzureOptions> MakeOptions() override {
+ AzureOptions options;
+ if (char* account_name = std::getenv("AZURE_FLAT_NAMESPACE_ACCOUNT_NAME"))
{
+ char* account_key = std::getenv("AZURE_FLAT_NAMESPACE_ACCOUNT_KEY");
Review Comment:
```suggestion
const auto account_key =
std::getenv("AZURE_FLAT_NAMESPACE_ACCOUNT_KEY");
```
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -216,23 +226,184 @@ class TestAzureFileSystem : public ::testing::Test {
void UploadLines(const std::vector<std::string>& lines, const char*
path_to_file,
int total_size) {
// TODO(GH-38333): Switch to using Azure filesystem to write once its
implemented.
- auto blob_client =
service_client_->GetBlobContainerClient(PreexistingContainerName())
- .GetBlockBlobClient(path_to_file);
+ auto blob_client =
+
blob_service_client_->GetBlobContainerClient(PreexistingContainerName())
+ .GetBlockBlobClient(path_to_file);
std::string all_lines = std::accumulate(lines.begin(), lines.end(),
std::string(""));
blob_client.UploadFrom(reinterpret_cast<const uint8_t*>(all_lines.data()),
total_size);
}
+
+ void RunGetFileInfoObjectWithNestedStructureTest();
+ void RunGetFileInfoObjectTest();
};
-TEST_F(TestAzureFileSystem, OpenInputStreamString) {
+class AzuriteFileSystemTest : public AzureFileSystemTest {
+ Result<AzureOptions> MakeOptions() {
+ EXPECT_THAT(GetAzuriteEnv(), NotNull());
+ ARROW_EXPECT_OK(GetAzuriteEnv()->status());
+ AzureOptions options;
+ options.backend = AzureBackend::Azurite;
+ ARROW_EXPECT_OK(options.ConfigureAccountKeyCredentials(
+ GetAzuriteEnv()->account_name(), GetAzuriteEnv()->account_key()));
+ return options;
+ }
+};
+
+class AzureFlatNamespaceFileSystemTest : public AzureFileSystemTest {
+ Result<AzureOptions> MakeOptions() override {
+ AzureOptions options;
+ if (char* account_name = std::getenv("AZURE_FLAT_NAMESPACE_ACCOUNT_NAME"))
{
+ char* account_key = std::getenv("AZURE_FLAT_NAMESPACE_ACCOUNT_KEY");
+ EXPECT_THAT(account_key, NotNull());
+ ARROW_EXPECT_OK(options.ConfigureAccountKeyCredentials(account_name,
account_key));
+ return options;
+ }
+ return Status::Cancelled(
+ "Connection details not provided for a real flat namespace "
+ "account.");
+ }
+};
+
+class AzureHierarchicalNamespaceFileSystemTest : public AzureFileSystemTest {
+ Result<AzureOptions> MakeOptions() override {
+ AzureOptions options;
+ if (char* account_name =
std::getenv("AZURE_HIERARCHICAL_NAMESPACE_ACCOUNT_NAME")) {
+ char* account_key =
std::getenv("AZURE_HIERARCHICAL_NAMESPACE_ACCOUNT_KEY");
+ EXPECT_THAT(account_key, NotNull());
+ ARROW_EXPECT_OK(options.ConfigureAccountKeyCredentials(account_name,
account_key));
+ return options;
+ }
+ return Status::Cancelled(
+ "Connection details not provided for a real hierachical namespace "
+ "account.");
+ }
+};
+
+TEST_F(AzureFlatNamespaceFileSystemTest, DetectHierarchicalNamespace) {
+ auto hierarchical_namespace = internal::HierarchicalNamespaceDetector();
+ ASSERT_OK(hierarchical_namespace.Init(datalake_service_client_));
+ ASSERT_OK_AND_EQ(false,
hierarchical_namespace.Enabled(PreexistingContainerName()));
+}
+
+TEST_F(AzureHierarchicalNamespaceFileSystemTest, DetectHierarchicalNamespace) {
+ auto hierarchical_namespace = internal::HierarchicalNamespaceDetector();
+ ASSERT_OK(hierarchical_namespace.Init(datalake_service_client_));
+ ASSERT_OK_AND_EQ(true,
hierarchical_namespace.Enabled(PreexistingContainerName()));
+}
+
+TEST_F(AzuriteFileSystemTest, DetectHierarchicalNamespace) {
+ auto hierarchical_namespace = internal::HierarchicalNamespaceDetector();
+ ASSERT_OK(hierarchical_namespace.Init(datalake_service_client_));
+ ASSERT_OK_AND_EQ(false,
hierarchical_namespace.Enabled(PreexistingContainerName()));
+}
+
+TEST_F(AzuriteFileSystemTest,
DetectHierarchicalNamespaceFailsWithMissingContainer) {
+ auto hierarchical_namespace = internal::HierarchicalNamespaceDetector();
+ ASSERT_OK(hierarchical_namespace.Init(datalake_service_client_));
+ ASSERT_NOT_OK(hierarchical_namespace.Enabled("non-existent-container"));
+}
+
+TEST_F(AzuriteFileSystemTest, GetFileInfoAccount) {
+ arrow::fs::AssertFileInfo(fs_.get(), "", FileType::Directory);
+
+ // URI
+ ASSERT_RAISES(Invalid, fs_->GetFileInfo("abfs://"));
+}
+
+TEST_F(AzuriteFileSystemTest, GetFileInfoContainer) {
+ arrow::fs::AssertFileInfo(fs_.get(), PreexistingContainerName(),
FileType::Directory);
+
+ arrow::fs::AssertFileInfo(fs_.get(), "non-existent-container",
FileType::NotFound);
Review Comment:
```suggestion
AssertFileInfo(fs_.get(), "non-existent-container", FileType::NotFound);
```
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -216,23 +226,184 @@ class TestAzureFileSystem : public ::testing::Test {
void UploadLines(const std::vector<std::string>& lines, const char*
path_to_file,
int total_size) {
// TODO(GH-38333): Switch to using Azure filesystem to write once its
implemented.
- auto blob_client =
service_client_->GetBlobContainerClient(PreexistingContainerName())
- .GetBlockBlobClient(path_to_file);
+ auto blob_client =
+
blob_service_client_->GetBlobContainerClient(PreexistingContainerName())
+ .GetBlockBlobClient(path_to_file);
std::string all_lines = std::accumulate(lines.begin(), lines.end(),
std::string(""));
blob_client.UploadFrom(reinterpret_cast<const uint8_t*>(all_lines.data()),
total_size);
}
+
+ void RunGetFileInfoObjectWithNestedStructureTest();
+ void RunGetFileInfoObjectTest();
};
-TEST_F(TestAzureFileSystem, OpenInputStreamString) {
+class AzuriteFileSystemTest : public AzureFileSystemTest {
+ Result<AzureOptions> MakeOptions() {
+ EXPECT_THAT(GetAzuriteEnv(), NotNull());
+ ARROW_EXPECT_OK(GetAzuriteEnv()->status());
+ AzureOptions options;
+ options.backend = AzureBackend::Azurite;
+ ARROW_EXPECT_OK(options.ConfigureAccountKeyCredentials(
+ GetAzuriteEnv()->account_name(), GetAzuriteEnv()->account_key()));
+ return options;
+ }
+};
+
+class AzureFlatNamespaceFileSystemTest : public AzureFileSystemTest {
+ Result<AzureOptions> MakeOptions() override {
+ AzureOptions options;
+ if (char* account_name = std::getenv("AZURE_FLAT_NAMESPACE_ACCOUNT_NAME"))
{
+ char* account_key = std::getenv("AZURE_FLAT_NAMESPACE_ACCOUNT_KEY");
+ EXPECT_THAT(account_key, NotNull());
+ ARROW_EXPECT_OK(options.ConfigureAccountKeyCredentials(account_name,
account_key));
Review Comment:
How about using `RETURN_NOT_OK()` or `ASSERT_OK()` to skip or stop the test
on failure?
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -216,23 +226,184 @@ class TestAzureFileSystem : public ::testing::Test {
void UploadLines(const std::vector<std::string>& lines, const char*
path_to_file,
int total_size) {
// TODO(GH-38333): Switch to using Azure filesystem to write once its
implemented.
- auto blob_client =
service_client_->GetBlobContainerClient(PreexistingContainerName())
- .GetBlockBlobClient(path_to_file);
+ auto blob_client =
+
blob_service_client_->GetBlobContainerClient(PreexistingContainerName())
+ .GetBlockBlobClient(path_to_file);
std::string all_lines = std::accumulate(lines.begin(), lines.end(),
std::string(""));
blob_client.UploadFrom(reinterpret_cast<const uint8_t*>(all_lines.data()),
total_size);
}
+
+ void RunGetFileInfoObjectWithNestedStructureTest();
+ void RunGetFileInfoObjectTest();
};
-TEST_F(TestAzureFileSystem, OpenInputStreamString) {
+class AzuriteFileSystemTest : public AzureFileSystemTest {
+ Result<AzureOptions> MakeOptions() {
+ EXPECT_THAT(GetAzuriteEnv(), NotNull());
+ ARROW_EXPECT_OK(GetAzuriteEnv()->status());
+ AzureOptions options;
+ options.backend = AzureBackend::Azurite;
+ ARROW_EXPECT_OK(options.ConfigureAccountKeyCredentials(
+ GetAzuriteEnv()->account_name(), GetAzuriteEnv()->account_key()));
+ return options;
+ }
+};
+
+class AzureFlatNamespaceFileSystemTest : public AzureFileSystemTest {
+ Result<AzureOptions> MakeOptions() override {
+ AzureOptions options;
+ if (char* account_name = std::getenv("AZURE_FLAT_NAMESPACE_ACCOUNT_NAME"))
{
+ char* account_key = std::getenv("AZURE_FLAT_NAMESPACE_ACCOUNT_KEY");
+ EXPECT_THAT(account_key, NotNull());
+ ARROW_EXPECT_OK(options.ConfigureAccountKeyCredentials(account_name,
account_key));
+ return options;
+ }
+ return Status::Cancelled(
+ "Connection details not provided for a real flat namespace "
+ "account.");
+ }
+};
+
+class AzureHierarchicalNamespaceFileSystemTest : public AzureFileSystemTest {
+ Result<AzureOptions> MakeOptions() override {
+ AzureOptions options;
+ if (char* account_name =
std::getenv("AZURE_HIERARCHICAL_NAMESPACE_ACCOUNT_NAME")) {
+ char* account_key =
std::getenv("AZURE_HIERARCHICAL_NAMESPACE_ACCOUNT_KEY");
+ EXPECT_THAT(account_key, NotNull());
+ ARROW_EXPECT_OK(options.ConfigureAccountKeyCredentials(account_name,
account_key));
+ return options;
+ }
+ return Status::Cancelled(
+ "Connection details not provided for a real hierachical namespace "
+ "account.");
+ }
+};
+
+TEST_F(AzureFlatNamespaceFileSystemTest, DetectHierarchicalNamespace) {
+ auto hierarchical_namespace = internal::HierarchicalNamespaceDetector();
+ ASSERT_OK(hierarchical_namespace.Init(datalake_service_client_));
+ ASSERT_OK_AND_EQ(false,
hierarchical_namespace.Enabled(PreexistingContainerName()));
+}
+
+TEST_F(AzureHierarchicalNamespaceFileSystemTest, DetectHierarchicalNamespace) {
+ auto hierarchical_namespace = internal::HierarchicalNamespaceDetector();
+ ASSERT_OK(hierarchical_namespace.Init(datalake_service_client_));
+ ASSERT_OK_AND_EQ(true,
hierarchical_namespace.Enabled(PreexistingContainerName()));
+}
+
+TEST_F(AzuriteFileSystemTest, DetectHierarchicalNamespace) {
+ auto hierarchical_namespace = internal::HierarchicalNamespaceDetector();
+ ASSERT_OK(hierarchical_namespace.Init(datalake_service_client_));
+ ASSERT_OK_AND_EQ(false,
hierarchical_namespace.Enabled(PreexistingContainerName()));
+}
+
+TEST_F(AzuriteFileSystemTest,
DetectHierarchicalNamespaceFailsWithMissingContainer) {
+ auto hierarchical_namespace = internal::HierarchicalNamespaceDetector();
+ ASSERT_OK(hierarchical_namespace.Init(datalake_service_client_));
+ ASSERT_NOT_OK(hierarchical_namespace.Enabled("non-existent-container"));
+}
+
+TEST_F(AzuriteFileSystemTest, GetFileInfoAccount) {
+ arrow::fs::AssertFileInfo(fs_.get(), "", FileType::Directory);
+
+ // URI
+ ASSERT_RAISES(Invalid, fs_->GetFileInfo("abfs://"));
+}
+
+TEST_F(AzuriteFileSystemTest, GetFileInfoContainer) {
+ arrow::fs::AssertFileInfo(fs_.get(), PreexistingContainerName(),
FileType::Directory);
+
+ arrow::fs::AssertFileInfo(fs_.get(), "non-existent-container",
FileType::NotFound);
+
+ // URI
+ ASSERT_RAISES(Invalid, fs_->GetFileInfo("abfs://" +
PreexistingContainerName()));
+}
+
+void AzureFileSystemTest::RunGetFileInfoObjectWithNestedStructureTest() {
+ // Adds detailed tests to handle cases of different edge cases
+ // with directory naming conventions (e.g. with and without slashes).
+ constexpr auto kObjectName =
"test-object-dir/some_other_dir/another_dir/foo";
+ // TODO(GH-38333): Switch to using Azure filesystem to write once its
implemented.
+ blob_service_client_->GetBlobContainerClient(PreexistingContainerName())
+ .GetBlockBlobClient(kObjectName)
+ .UploadFrom(reinterpret_cast<const uint8_t*>(kLoremIpsum),
strlen(kLoremIpsum));
+
+ // 0 is immediately after "/" lexicographically, ensure that this doesn't
+ // cause unexpected issues.
+ // TODO(GH-38333): Switch to using Azure filesystem to write once its
implemented.
+ blob_service_client_->GetBlobContainerClient(PreexistingContainerName())
+ .GetBlockBlobClient("test-object-dir/some_other_dir0")
+ .UploadFrom(reinterpret_cast<const uint8_t*>(kLoremIpsum),
strlen(kLoremIpsum));
+
+ blob_service_client_->GetBlobContainerClient(PreexistingContainerName())
+ .GetBlockBlobClient(std::string(kObjectName) + "0")
+ .UploadFrom(reinterpret_cast<const uint8_t*>(kLoremIpsum),
strlen(kLoremIpsum));
+
+ AssertFileInfo(fs_.get(), PreexistingContainerPath() + kObjectName,
FileType::File);
+ AssertFileInfo(fs_.get(), PreexistingContainerPath() + kObjectName + "/",
+ FileType::NotFound);
+ AssertFileInfo(fs_.get(), PreexistingContainerPath() + "test-object-dir",
+ FileType::Directory);
+ AssertFileInfo(fs_.get(), PreexistingContainerPath() + "test-object-dir/",
+ FileType::Directory);
+ AssertFileInfo(fs_.get(), PreexistingContainerPath() +
"test-object-dir/some_other_dir",
+ FileType::Directory);
+ AssertFileInfo(fs_.get(),
+ PreexistingContainerPath() +
"test-object-dir/some_other_dir/",
+ FileType::Directory);
+
+ AssertFileInfo(fs_.get(), PreexistingContainerPath() + "test-object-di",
+ FileType::NotFound);
+ AssertFileInfo(fs_.get(), PreexistingContainerPath() +
"test-object-dir/some_other_di",
+ FileType::NotFound);
+}
+
+TEST_F(AzuriteFileSystemTest, GetFileInfoObjectWithNestedStructure) {
+ RunGetFileInfoObjectWithNestedStructureTest();
+}
+
+TEST_F(AzureHierarchicalNamespaceFileSystemTest,
GetFileInfoObjectWithNestedStructure) {
+ RunGetFileInfoObjectWithNestedStructureTest();
+ datalake_service_client_->GetFileSystemClient(PreexistingContainerName())
+ .GetDirectoryClient("test-empty-object-dir")
+ .Create();
+
+ AssertFileInfo(fs_.get(), PreexistingContainerPath() +
"test-empty-object-dir",
+ FileType::Directory);
+}
+
+void AzureFileSystemTest::RunGetFileInfoObjectTest() {
+ auto object_properties =
+ blob_service_client_->GetBlobContainerClient(PreexistingContainerName())
+ .GetBlobClient(PreexistingObjectName())
+ .GetProperties()
+ .Value;
+
+ arrow::fs::AssertFileInfo(
Review Comment:
```suggestion
AssertFileInfo(
```
--
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]
