kou commented on code in PR #38793:
URL: https://github.com/apache/arrow/pull/38793#discussion_r1399964747
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -724,6 +724,59 @@ class AzureFileSystem::Impl {
return Status::OK();
}
+
+ Status DeleteDir(const AzureLocation& location) {
+ if (location.container.empty()) {
+ return Status::Invalid("Cannot delete an empty container");
+ }
+
+ if (location.path.empty()) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(location.container);
+ try {
+ auto response = container_client.Delete();
+ if (response.Value.Deleted) {
+ return Status::OK();
+ } else {
+ return StatusFromErrorResponse(
+ container_client.GetUrl(), response.RawResponse.get(),
+ "Failed to delete a container: " + location.container);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to delete a container: " + location.container + ": " +
+ container_client.GetUrl(),
+ exception);
+ }
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(location.container));
+ if (!hierarchical_namespace_enabled) {
+ // Without hierarchical namespace enabled Azure blob storage has no
directories.
+ // Therefore we can't, and don't need to delete one.
+ return Status::OK();
+ }
Review Comment:
Ah, it makes sense. I'll do it.
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -507,6 +507,41 @@ TEST_F(AzuriteFileSystemTest, CreateDirUri) {
ASSERT_RAISES(Invalid, fs_->CreateDir("abfs://" + RandomContainerName(),
true));
}
+TEST_F(AzuriteFileSystemTest, DeleteContainerDirSuccess) {
+ auto container_name = RandomContainerName();
+ ASSERT_OK(fs_->CreateDir(container_name));
+ ASSERT_OK(fs_->DeleteDir(container_name));
+}
+
+TEST_F(AzuriteFileSystemTest, DeleteDirSuccess) {
+ const auto path =
+ internal::ConcatAbstractPath(PreexistingContainerName(),
RandomDirectoryName());
+ // There is only virtual directory without hierarchical namespace
+ // support. So the DeleteDir() does nothing.
Review Comment:
It make sense. I'll implement it and add a test for the case.
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -507,6 +507,41 @@ TEST_F(AzuriteFileSystemTest, CreateDirUri) {
ASSERT_RAISES(Invalid, fs_->CreateDir("abfs://" + RandomContainerName(),
true));
}
+TEST_F(AzuriteFileSystemTest, DeleteContainerDirSuccess) {
+ auto container_name = RandomContainerName();
+ ASSERT_OK(fs_->CreateDir(container_name));
+ ASSERT_OK(fs_->DeleteDir(container_name));
Review Comment:
OK. I'll add them.
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -507,6 +507,41 @@ TEST_F(AzuriteFileSystemTest, CreateDirUri) {
ASSERT_RAISES(Invalid, fs_->CreateDir("abfs://" + RandomContainerName(),
true));
}
+TEST_F(AzuriteFileSystemTest, DeleteContainerDirSuccess) {
+ auto container_name = RandomContainerName();
+ ASSERT_OK(fs_->CreateDir(container_name));
+ ASSERT_OK(fs_->DeleteDir(container_name));
+}
+
+TEST_F(AzuriteFileSystemTest, DeleteDirSuccess) {
Review Comment:
> I think it would be good to add a test case for a non-empty "virtual
directory" where we expect the contents of the directory to be deleted.
OK. I'll add it.
> Would be nice to add a non-empty test case for hierarchical namespace too.
`AzureHierarchicalNamespaceFileSystemTest.DeleteDirSuccess` is for the case.
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
