You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "kou (via GitHub)" <gi...@apache.org> on 2023/11/27 07:50:43 UTC
[PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
kou opened a new pull request, #38888:
URL: https://github.com/apache/arrow/pull/38888
### Rationale for this change
`DeleteDirContents()` deletes the given directory contents recursively like other filesystem implementations.
Azure file system treats the following cases as root directory:
* Empty container
* Empty path
### What changes are included in this PR?
List and delete approach is used with/without hierarchical namespace support. Because Azure doesn't provide "DeleteDirContents" API with hierarchical namespace support. We may want to use delete the given directory and create an empty approach instead of list and delete approach for performance but it may not be acceptable with some use cases.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #38888:
URL: https://github.com/apache/arrow/pull/38888#issuecomment-1829133709
Updated:
* Use `PathNotFound()`
* Use `for`
* Add an argument name comment
* Skip a test on macOS that has a problem with Azurite
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on code in PR #38888:
URL: https://github.com/apache/arrow/pull/38888#discussion_r1411096140
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -666,6 +666,119 @@ TEST_F(AzuriteFileSystemTest, DeleteDirUri) {
ASSERT_RAISES(Invalid, fs_->DeleteDir("abfs://" + PreexistingContainerPath()));
}
+TEST_F(AzuriteFileSystemTest, DeleteDirContentsSuccessContainer) {
+#ifdef __APPLE__
+ GTEST_SKIP() << "This test fails by an Azurite problem: "
+ "https://github.com/Azure/Azurite/pull/2302";
+#endif
+ const auto container_path = RandomContainerName();
+ const auto directory_path =
+ internal::ConcatAbstractPath(container_path, RandomDirectoryName());
+ const auto sub_directory_path = internal::ConcatAbstractPath(directory_path, "new-sub");
+ const auto sub_blob_path = internal::ConcatAbstractPath(sub_directory_path, "sub.txt");
+ const auto top_blob_path = internal::ConcatAbstractPath(directory_path, "top.txt");
+ ASSERT_OK(fs_->CreateDir(sub_directory_path, true));
+ ASSERT_OK_AND_ASSIGN(auto output, fs_->OpenOutputStream(sub_blob_path));
+ ASSERT_OK(output->Write(std::string_view("sub")));
+ ASSERT_OK(output->Close());
+ ASSERT_OK_AND_ASSIGN(output, fs_->OpenOutputStream(top_blob_path));
+ ASSERT_OK(output->Write(std::string_view("top")));
+ ASSERT_OK(output->Close());
Review Comment:
nit: Could all of this be extracted to a member function of `AzureFileSystemTest`? It looks like this has been copy pasted in 3 of the tests.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #38888:
URL: https://github.com/apache/arrow/pull/38888#discussion_r1410060487
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -1017,69 +1089,67 @@ class AzureFileSystem::Impl {
exception);
}
} else {
- auto container_client =
- blob_service_client_->GetBlobContainerClient(location.container);
- Azure::Storage::Blobs::ListBlobsOptions options;
- options.Prefix = internal::EnsureTrailingSlash(location.path);
- // https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#remarks
- //
- // Only supports up to 256 subrequests in a single batch. The
- // size of the body for a batch request can't exceed 4 MB.
- const int32_t kNumMaxRequestsInBatch = 256;
- options.PageSizeHint = kNumMaxRequestsInBatch;
+ return DeleteDirContentsWihtoutHierarchicalNamespace(location, true);
+ }
+ }
+
+ Status DeleteDirContents(const AzureLocation& location, bool missing_dir_ok) {
+ if (location.container.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
+ }
+ if (location.path.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
Review Comment:
The `allow_bucket_deletion` option is for `DeleteDir()` not `DeleteDirContents()`.
And this PR is for `DeleteDirContents()`.
So it should be discussed separately.
I've opened a new issue for it: #38999
We should discuss it further on the issue not here.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #38888:
URL: https://github.com/apache/arrow/pull/38888#discussion_r1409830602
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -1017,69 +1089,67 @@ class AzureFileSystem::Impl {
exception);
}
} else {
- auto container_client =
- blob_service_client_->GetBlobContainerClient(location.container);
- Azure::Storage::Blobs::ListBlobsOptions options;
- options.Prefix = internal::EnsureTrailingSlash(location.path);
- // https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#remarks
- //
- // Only supports up to 256 subrequests in a single batch. The
- // size of the body for a batch request can't exceed 4 MB.
- const int32_t kNumMaxRequestsInBatch = 256;
- options.PageSizeHint = kNumMaxRequestsInBatch;
+ return DeleteDirContentsWihtoutHierarchicalNamespace(location, true);
+ }
+ }
+
+ Status DeleteDirContents(const AzureLocation& location, bool missing_dir_ok) {
+ if (location.container.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
+ }
+ if (location.path.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
Review Comment:
> I think when comparing azure to s3, the Azure storage account is analogous to the S3 bucket and the Azure container is an extra level that S3 doesn't have. I think 1 doesn't really reference any storage on S3.
Azure Containers are S3 Buckets. Azure Blob Names are S3 keys. `s3fs` has an option [1] that forbids/allows creation of new buckets for when the user asks for the creation of a `/newbucket` directory [2].
[1] https://github.com/apache/arrow/blob/main/cpp/src/arrow/filesystem/s3fs.cc#L1820
[2] https://github.com/apache/arrow/blob/main/cpp/src/arrow/filesystem/s3fs.cc#L2672
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #38888:
URL: https://github.com/apache/arrow/pull/38888#discussion_r1410064470
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -970,6 +970,79 @@ class AzureFileSystem::Impl {
return stream;
}
+ private:
+ Status DeleteDirContentsWihtoutHierarchicalNamespace(const AzureLocation& location,
+ bool missing_dir_ok) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(location.container);
+ Azure::Storage::Blobs::ListBlobsOptions options;
+ options.Prefix = internal::EnsureTrailingSlash(location.path);
Review Comment:
`EnsureTrailingSlash()` does nothing for empty string:
https://github.com/apache/arrow/blob/d66780d90c6faf7bf051cbf12b4b68dff098bd54/cpp/src/arrow/filesystem/path_util.cc#L151-L159
Should we add a (needless) special condition such as the following for readability?
```cpp
if (!location.path.empty()) {
options.Prefix = internal::EnsureTrailingSlash(location.path);
}
```
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on code in PR #38888:
URL: https://github.com/apache/arrow/pull/38888#discussion_r1409948813
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -1017,69 +1089,67 @@ class AzureFileSystem::Impl {
exception);
}
} else {
- auto container_client =
- blob_service_client_->GetBlobContainerClient(location.container);
- Azure::Storage::Blobs::ListBlobsOptions options;
- options.Prefix = internal::EnsureTrailingSlash(location.path);
- // https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#remarks
- //
- // Only supports up to 256 subrequests in a single batch. The
- // size of the body for a batch request can't exceed 4 MB.
- const int32_t kNumMaxRequestsInBatch = 256;
- options.PageSizeHint = kNumMaxRequestsInBatch;
+ return DeleteDirContentsWihtoutHierarchicalNamespace(location, true);
+ }
+ }
+
+ Status DeleteDirContents(const AzureLocation& location, bool missing_dir_ok) {
+ if (location.container.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
+ }
+ if (location.path.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
Review Comment:
Probably I need to review the PR properly to understand the context. I don't feel strongly.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #38888:
URL: https://github.com/apache/arrow/pull/38888#discussion_r1411517620
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -970,6 +970,79 @@ class AzureFileSystem::Impl {
return stream;
}
+ private:
+ Status DeleteDirContentsWihtoutHierarchicalNamespace(const AzureLocation& location,
+ bool missing_dir_ok) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(location.container);
+ Azure::Storage::Blobs::ListBlobsOptions options;
+ options.Prefix = internal::EnsureTrailingSlash(location.path);
Review Comment:
OK. I'll use the `if (!location.path.empty())`.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #38888:
URL: https://github.com/apache/arrow/pull/38888#issuecomment-1827294798
@Tom-Newton @felipecrv You may want to review this.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #38888:
URL: https://github.com/apache/arrow/pull/38888#discussion_r1408655837
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -970,6 +970,78 @@ class AzureFileSystem::Impl {
return stream;
}
+ private:
+ Status DeleteDirContentsWihtoutHierarchicalNamespace(const AzureLocation& location,
+ bool missing_dir_ok) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(location.container);
+ Azure::Storage::Blobs::ListBlobsOptions options;
+ options.Prefix = internal::EnsureTrailingSlash(location.path);
+ // https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#remarks
+ //
+ // Only supports up to 256 subrequests in a single batch. The
+ // size of the body for a batch request can't exceed 4 MB.
+ const int32_t kNumMaxRequestsInBatch = 256;
+ options.PageSizeHint = kNumMaxRequestsInBatch;
+ try {
+ auto list_response = container_client.ListBlobs(options);
+ if (!missing_dir_ok && list_response.Blobs.empty()) {
+ return Status::IOError("Specified directory doesn't exist: ", location.path, ": ",
+ container_client.GetUrl());
+ }
+ while (list_response.HasPage() && !list_response.Blobs.empty()) {
Review Comment:
(Hmm. I think that the case (empty blob but it's not the last response) isn't happen but I'm not sure because Azure Blob Storage is a proprietary product...)
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #38888:
URL: https://github.com/apache/arrow/pull/38888#discussion_r1407044310
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -970,6 +970,78 @@ class AzureFileSystem::Impl {
return stream;
}
+ private:
+ Status DeleteDirContentsWihtoutHierarchicalNamespace(const AzureLocation& location,
+ bool missing_dir_ok) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(location.container);
+ Azure::Storage::Blobs::ListBlobsOptions options;
+ options.Prefix = internal::EnsureTrailingSlash(location.path);
+ // https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#remarks
+ //
+ // Only supports up to 256 subrequests in a single batch. The
+ // size of the body for a batch request can't exceed 4 MB.
+ const int32_t kNumMaxRequestsInBatch = 256;
+ options.PageSizeHint = kNumMaxRequestsInBatch;
+ try {
+ auto list_response = container_client.ListBlobs(options);
+ if (!missing_dir_ok && list_response.Blobs.empty()) {
+ return Status::IOError("Specified directory doesn't exist: ", location.path, ": ",
+ container_client.GetUrl());
Review Comment:
Shouldn't this be a `PathNotFound` error?
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -970,6 +970,78 @@ class AzureFileSystem::Impl {
return stream;
}
+ private:
+ Status DeleteDirContentsWihtoutHierarchicalNamespace(const AzureLocation& location,
+ bool missing_dir_ok) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(location.container);
+ Azure::Storage::Blobs::ListBlobsOptions options;
+ options.Prefix = internal::EnsureTrailingSlash(location.path);
+ // https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#remarks
+ //
+ // Only supports up to 256 subrequests in a single batch. The
+ // size of the body for a batch request can't exceed 4 MB.
+ const int32_t kNumMaxRequestsInBatch = 256;
+ options.PageSizeHint = kNumMaxRequestsInBatch;
+ try {
+ auto list_response = container_client.ListBlobs(options);
+ if (!missing_dir_ok && list_response.Blobs.empty()) {
+ return Status::IOError("Specified directory doesn't exist: ", location.path, ": ",
+ container_client.GetUrl());
+ }
+ while (list_response.HasPage() && !list_response.Blobs.empty()) {
Review Comment:
This could be me being overly cautious, but maybe we shouldn't stop when `Blobs.empty()`, but instead run `continue;` and evaluate `MoveToNextPage()` again?
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -1017,69 +1089,67 @@ class AzureFileSystem::Impl {
exception);
}
} else {
- auto container_client =
- blob_service_client_->GetBlobContainerClient(location.container);
- Azure::Storage::Blobs::ListBlobsOptions options;
- options.Prefix = internal::EnsureTrailingSlash(location.path);
- // https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#remarks
- //
- // Only supports up to 256 subrequests in a single batch. The
- // size of the body for a batch request can't exceed 4 MB.
- const int32_t kNumMaxRequestsInBatch = 256;
- options.PageSizeHint = kNumMaxRequestsInBatch;
+ return DeleteDirContentsWihtoutHierarchicalNamespace(location, true);
Review Comment:
`/*missing_dir_ok=*/true`
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -1017,69 +1089,67 @@ class AzureFileSystem::Impl {
exception);
}
} else {
- auto container_client =
- blob_service_client_->GetBlobContainerClient(location.container);
- Azure::Storage::Blobs::ListBlobsOptions options;
- options.Prefix = internal::EnsureTrailingSlash(location.path);
- // https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#remarks
- //
- // Only supports up to 256 subrequests in a single batch. The
- // size of the body for a batch request can't exceed 4 MB.
- const int32_t kNumMaxRequestsInBatch = 256;
- options.PageSizeHint = kNumMaxRequestsInBatch;
+ return DeleteDirContentsWihtoutHierarchicalNamespace(location, true);
+ }
+ }
+
+ Status DeleteDirContents(const AzureLocation& location, bool missing_dir_ok) {
+ if (location.container.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
+ }
+ if (location.path.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
Review Comment:
Shouldn't this be interpreted as deleting the contents of the container?
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -970,6 +970,78 @@ class AzureFileSystem::Impl {
return stream;
}
+ private:
+ Status DeleteDirContentsWihtoutHierarchicalNamespace(const AzureLocation& location,
+ bool missing_dir_ok) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(location.container);
+ Azure::Storage::Blobs::ListBlobsOptions options;
+ options.Prefix = internal::EnsureTrailingSlash(location.path);
+ // https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#remarks
+ //
+ // Only supports up to 256 subrequests in a single batch. The
+ // size of the body for a batch request can't exceed 4 MB.
+ const int32_t kNumMaxRequestsInBatch = 256;
+ options.PageSizeHint = kNumMaxRequestsInBatch;
+ try {
+ auto list_response = container_client.ListBlobs(options);
+ if (!missing_dir_ok && list_response.Blobs.empty()) {
+ return Status::IOError("Specified directory doesn't exist: ", location.path, ": ",
+ container_client.GetUrl());
+ }
+ while (list_response.HasPage() && !list_response.Blobs.empty()) {
+ auto batch = container_client.CreateBatch();
+ std::vector<Azure::Storage::DeferredResponse<
+ Azure::Storage::Blobs::Models::DeleteBlobResult>>
+ deferred_responses;
+ for (const auto& blob_item : list_response.Blobs) {
+ deferred_responses.push_back(batch.DeleteBlob(blob_item.Name));
Review Comment:
Skip when the `blob_item.Name` matches the `options.Prefix` because that could be the empty-dir marker blob (an empty blob ending with a `/`).
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -970,6 +970,78 @@ class AzureFileSystem::Impl {
return stream;
}
+ private:
+ Status DeleteDirContentsWihtoutHierarchicalNamespace(const AzureLocation& location,
+ bool missing_dir_ok) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(location.container);
+ Azure::Storage::Blobs::ListBlobsOptions options;
+ options.Prefix = internal::EnsureTrailingSlash(location.path);
+ // https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#remarks
+ //
+ // Only supports up to 256 subrequests in a single batch. The
+ // size of the body for a batch request can't exceed 4 MB.
+ const int32_t kNumMaxRequestsInBatch = 256;
+ options.PageSizeHint = kNumMaxRequestsInBatch;
+ try {
+ auto list_response = container_client.ListBlobs(options);
+ if (!missing_dir_ok && list_response.Blobs.empty()) {
+ return Status::IOError("Specified directory doesn't exist: ", location.path, ": ",
+ container_client.GetUrl());
+ }
+ while (list_response.HasPage() && !list_response.Blobs.empty()) {
Review Comment:
Another thing: instead of `while`, you can use `for (; list_response.HasPage(); list_response.MoveToNextPage()) {` here.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #38888:
URL: https://github.com/apache/arrow/pull/38888#discussion_r1407195527
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -970,6 +970,78 @@ class AzureFileSystem::Impl {
return stream;
}
+ private:
+ Status DeleteDirContentsWihtoutHierarchicalNamespace(const AzureLocation& location,
+ bool missing_dir_ok) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(location.container);
+ Azure::Storage::Blobs::ListBlobsOptions options;
+ options.Prefix = internal::EnsureTrailingSlash(location.path);
+ // https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#remarks
+ //
+ // Only supports up to 256 subrequests in a single batch. The
+ // size of the body for a batch request can't exceed 4 MB.
+ const int32_t kNumMaxRequestsInBatch = 256;
+ options.PageSizeHint = kNumMaxRequestsInBatch;
+ try {
+ auto list_response = container_client.ListBlobs(options);
+ if (!missing_dir_ok && list_response.Blobs.empty()) {
+ return Status::IOError("Specified directory doesn't exist: ", location.path, ": ",
+ container_client.GetUrl());
+ }
+ while (list_response.HasPage() && !list_response.Blobs.empty()) {
+ auto batch = container_client.CreateBatch();
+ std::vector<Azure::Storage::DeferredResponse<
+ Azure::Storage::Blobs::Models::DeleteBlobResult>>
+ deferred_responses;
+ for (const auto& blob_item : list_response.Blobs) {
+ deferred_responses.push_back(batch.DeleteBlob(blob_item.Name));
Review Comment:
I think that we should do it in https://github.com/apache/arrow/issues/38772 .
We have a test for this case. So we can detect the case when we work on https://github.com/apache/arrow/issues/38772 .
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -970,6 +970,78 @@ class AzureFileSystem::Impl {
return stream;
}
+ private:
+ Status DeleteDirContentsWihtoutHierarchicalNamespace(const AzureLocation& location,
+ bool missing_dir_ok) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(location.container);
+ Azure::Storage::Blobs::ListBlobsOptions options;
+ options.Prefix = internal::EnsureTrailingSlash(location.path);
+ // https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#remarks
+ //
+ // Only supports up to 256 subrequests in a single batch. The
+ // size of the body for a batch request can't exceed 4 MB.
+ const int32_t kNumMaxRequestsInBatch = 256;
+ options.PageSizeHint = kNumMaxRequestsInBatch;
+ try {
+ auto list_response = container_client.ListBlobs(options);
+ if (!missing_dir_ok && list_response.Blobs.empty()) {
+ return Status::IOError("Specified directory doesn't exist: ", location.path, ": ",
+ container_client.GetUrl());
Review Comment:
Good catch!
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -1017,69 +1089,67 @@ class AzureFileSystem::Impl {
exception);
}
} else {
- auto container_client =
- blob_service_client_->GetBlobContainerClient(location.container);
- Azure::Storage::Blobs::ListBlobsOptions options;
- options.Prefix = internal::EnsureTrailingSlash(location.path);
- // https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#remarks
- //
- // Only supports up to 256 subrequests in a single batch. The
- // size of the body for a batch request can't exceed 4 MB.
- const int32_t kNumMaxRequestsInBatch = 256;
- options.PageSizeHint = kNumMaxRequestsInBatch;
+ return DeleteDirContentsWihtoutHierarchicalNamespace(location, true);
+ }
+ }
+
+ Status DeleteDirContents(const AzureLocation& location, bool missing_dir_ok) {
+ if (location.container.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
+ }
+ if (location.path.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
Review Comment:
We have a special API for this case: `DeleteRootDirContents()`
https://github.com/apache/arrow/blob/63353baf1cda1d1fc7bb614ce01558c12990e073/cpp/src/arrow/filesystem/filesystem.h#L233-L249
So I think that we should return an error here.
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -1017,69 +1089,67 @@ class AzureFileSystem::Impl {
exception);
}
} else {
- auto container_client =
- blob_service_client_->GetBlobContainerClient(location.container);
- Azure::Storage::Blobs::ListBlobsOptions options;
- options.Prefix = internal::EnsureTrailingSlash(location.path);
- // https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#remarks
- //
- // Only supports up to 256 subrequests in a single batch. The
- // size of the body for a batch request can't exceed 4 MB.
- const int32_t kNumMaxRequestsInBatch = 256;
- options.PageSizeHint = kNumMaxRequestsInBatch;
+ return DeleteDirContentsWihtoutHierarchicalNamespace(location, true);
Review Comment:
OK. I'll add the comment.
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -970,6 +970,78 @@ class AzureFileSystem::Impl {
return stream;
}
+ private:
+ Status DeleteDirContentsWihtoutHierarchicalNamespace(const AzureLocation& location,
+ bool missing_dir_ok) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(location.container);
+ Azure::Storage::Blobs::ListBlobsOptions options;
+ options.Prefix = internal::EnsureTrailingSlash(location.path);
+ // https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#remarks
+ //
+ // Only supports up to 256 subrequests in a single batch. The
+ // size of the body for a batch request can't exceed 4 MB.
+ const int32_t kNumMaxRequestsInBatch = 256;
+ options.PageSizeHint = kNumMaxRequestsInBatch;
+ try {
+ auto list_response = container_client.ListBlobs(options);
+ if (!missing_dir_ok && list_response.Blobs.empty()) {
+ return Status::IOError("Specified directory doesn't exist: ", location.path, ": ",
+ container_client.GetUrl());
+ }
+ while (list_response.HasPage() && !list_response.Blobs.empty()) {
Review Comment:
Both of `HasPage()` and `!Blobs.empty()` checks are needed to avoid empty `SubmitBatch()` request.
If there are no blobs under the given path, `ListBlobs()` returns `HasPage() == true` and `Blobs.empty() == true` response.
I'll use `for` as you suggested. I also found an azure-sdk-for-cpp document that uses `for`: https://github.com/Azure/azure-sdk-for-cpp/blob/main/sdk/storage/MigrationGuide.md#listing-blobs-in-a-container
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on code in PR #38888:
URL: https://github.com/apache/arrow/pull/38888#discussion_r1411101893
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -970,6 +970,79 @@ class AzureFileSystem::Impl {
return stream;
}
+ private:
+ Status DeleteDirContentsWihtoutHierarchicalNamespace(const AzureLocation& location,
+ bool missing_dir_ok) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(location.container);
+ Azure::Storage::Blobs::ListBlobsOptions options;
+ options.Prefix = internal::EnsureTrailingSlash(location.path);
Review Comment:
Maybe just leave a comment saying that `EnsureTrailingSlash` doesn't alter empty string.
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -666,6 +666,119 @@ TEST_F(AzuriteFileSystemTest, DeleteDirUri) {
ASSERT_RAISES(Invalid, fs_->DeleteDir("abfs://" + PreexistingContainerPath()));
}
+TEST_F(AzuriteFileSystemTest, DeleteDirContentsSuccessContainer) {
+#ifdef __APPLE__
+ GTEST_SKIP() << "This test fails by an Azurite problem: "
+ "https://github.com/Azure/Azurite/pull/2302";
+#endif
+ const auto container_path = RandomContainerName();
+ const auto directory_path =
+ internal::ConcatAbstractPath(container_path, RandomDirectoryName());
+ const auto sub_directory_path = internal::ConcatAbstractPath(directory_path, "new-sub");
+ const auto sub_blob_path = internal::ConcatAbstractPath(sub_directory_path, "sub.txt");
+ const auto top_blob_path = internal::ConcatAbstractPath(directory_path, "top.txt");
+ ASSERT_OK(fs_->CreateDir(sub_directory_path, true));
+ ASSERT_OK_AND_ASSIGN(auto output, fs_->OpenOutputStream(sub_blob_path));
+ ASSERT_OK(output->Write(std::string_view("sub")));
+ ASSERT_OK(output->Close());
+ ASSERT_OK_AND_ASSIGN(output, fs_->OpenOutputStream(top_blob_path));
+ ASSERT_OK(output->Write(std::string_view("top")));
+ ASSERT_OK(output->Close());
Review Comment:
Could all of this be extracted to a member function of `AzureFileSystemTest`? It looks like this has been copy pasted in 3 of the tests.
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -1017,69 +1089,67 @@ class AzureFileSystem::Impl {
exception);
}
} else {
- auto container_client =
- blob_service_client_->GetBlobContainerClient(location.container);
- Azure::Storage::Blobs::ListBlobsOptions options;
- options.Prefix = internal::EnsureTrailingSlash(location.path);
- // https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#remarks
- //
- // Only supports up to 256 subrequests in a single batch. The
- // size of the body for a batch request can't exceed 4 MB.
- const int32_t kNumMaxRequestsInBatch = 256;
- options.PageSizeHint = kNumMaxRequestsInBatch;
+ return DeleteDirContentsWihtoutHierarchicalNamespace(location, true);
+ }
+ }
+
+ Status DeleteDirContents(const AzureLocation& location, bool missing_dir_ok) {
+ if (location.container.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
+ }
+ if (location.path.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
Review Comment:
Overall I'm happy with how this is currently implemented.
For the sake of argument though... I think `allow_container_deletion` is relevant to `DeleteDirContents` in the case of `location.container.empty()`. I would expect the behaviour in this case to be to delete all containers in the storage account. However, I'm happy to stick with just returning an error because it seems quite dangerous.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on code in PR #38888:
URL: https://github.com/apache/arrow/pull/38888#discussion_r1409948813
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -1017,69 +1089,67 @@ class AzureFileSystem::Impl {
exception);
}
} else {
- auto container_client =
- blob_service_client_->GetBlobContainerClient(location.container);
- Azure::Storage::Blobs::ListBlobsOptions options;
- options.Prefix = internal::EnsureTrailingSlash(location.path);
- // https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#remarks
- //
- // Only supports up to 256 subrequests in a single batch. The
- // size of the body for a batch request can't exceed 4 MB.
- const int32_t kNumMaxRequestsInBatch = 256;
- options.PageSizeHint = kNumMaxRequestsInBatch;
+ return DeleteDirContentsWihtoutHierarchicalNamespace(location, true);
+ }
+ }
+
+ Status DeleteDirContents(const AzureLocation& location, bool missing_dir_ok) {
+ if (location.container.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
+ }
+ if (location.path.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
Review Comment:
Probably I need to review the PR properly to understand the context, but I don't feel strongly. Deleting whole containers is something that will be done very rarely, and probably even more rare for it to be done from a filesystem abstraction. Personally I've always used terraform to create and delete containers.
I will try to review tomorrow morning.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on code in PR #38888:
URL: https://github.com/apache/arrow/pull/38888#discussion_r1409948813
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -1017,69 +1089,67 @@ class AzureFileSystem::Impl {
exception);
}
} else {
- auto container_client =
- blob_service_client_->GetBlobContainerClient(location.container);
- Azure::Storage::Blobs::ListBlobsOptions options;
- options.Prefix = internal::EnsureTrailingSlash(location.path);
- // https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#remarks
- //
- // Only supports up to 256 subrequests in a single batch. The
- // size of the body for a batch request can't exceed 4 MB.
- const int32_t kNumMaxRequestsInBatch = 256;
- options.PageSizeHint = kNumMaxRequestsInBatch;
+ return DeleteDirContentsWihtoutHierarchicalNamespace(location, true);
+ }
+ }
+
+ Status DeleteDirContents(const AzureLocation& location, bool missing_dir_ok) {
+ if (location.container.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
+ }
+ if (location.path.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
Review Comment:
Probably I need to review the PR properly to understand the context. I don't feel strongly.
I will try to review tomorrow morning.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #38888:
URL: https://github.com/apache/arrow/pull/38888#discussion_r1408654016
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -1017,69 +1089,67 @@ class AzureFileSystem::Impl {
exception);
}
} else {
- auto container_client =
- blob_service_client_->GetBlobContainerClient(location.container);
- Azure::Storage::Blobs::ListBlobsOptions options;
- options.Prefix = internal::EnsureTrailingSlash(location.path);
- // https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#remarks
- //
- // Only supports up to 256 subrequests in a single batch. The
- // size of the body for a batch request can't exceed 4 MB.
- const int32_t kNumMaxRequestsInBatch = 256;
- options.PageSizeHint = kNumMaxRequestsInBatch;
+ return DeleteDirContentsWihtoutHierarchicalNamespace(location, true);
+ }
+ }
+
+ Status DeleteDirContents(const AzureLocation& location, bool missing_dir_ok) {
+ if (location.container.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
+ }
+ if (location.path.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
Review Comment:
Can we clarify this discussion?
What should be "root dir"?
1. ` `: Empty location (no container and no path)
2. `container` and `container/`: Container only
3. `container/path`: Container and path
My opinion and the current implementation:
1. Root dir
2. Root dir
3. Not root dir
FYI: s3fs:
1. ` `: Empty path (no bucket and no key): Root dir
2. `bucket` and `bucket/`: Bucket only: Not root dir
3. `bucket/key`: Bucket and key: Not root dir
@Tom-Newton What do you think about this as an Azure Blob Storage user? I'm not an Azure Blob Storage user.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on code in PR #38888:
URL: https://github.com/apache/arrow/pull/38888#discussion_r1408952617
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -1017,69 +1089,67 @@ class AzureFileSystem::Impl {
exception);
}
} else {
- auto container_client =
- blob_service_client_->GetBlobContainerClient(location.container);
- Azure::Storage::Blobs::ListBlobsOptions options;
- options.Prefix = internal::EnsureTrailingSlash(location.path);
- // https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#remarks
- //
- // Only supports up to 256 subrequests in a single batch. The
- // size of the body for a batch request can't exceed 4 MB.
- const int32_t kNumMaxRequestsInBatch = 256;
- options.PageSizeHint = kNumMaxRequestsInBatch;
+ return DeleteDirContentsWihtoutHierarchicalNamespace(location, true);
+ }
+ }
+
+ Status DeleteDirContents(const AzureLocation& location, bool missing_dir_ok) {
+ if (location.container.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
+ }
+ if (location.path.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
Review Comment:
I would consider the storage account the root dir. So that means only `1` is the root dir. I believe this is how the fsspec implementation and the original arrow azurefs PR did it.
I think when comparing azure to s3, the Azure storage account is analogous to the S3 bucket and the Azure container is an extra level that S3 doesn't have. I think `1` doesn't really reference any storage on S3.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #38888:
URL: https://github.com/apache/arrow/pull/38888#discussion_r1409832642
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -1017,69 +1089,67 @@ class AzureFileSystem::Impl {
exception);
}
} else {
- auto container_client =
- blob_service_client_->GetBlobContainerClient(location.container);
- Azure::Storage::Blobs::ListBlobsOptions options;
- options.Prefix = internal::EnsureTrailingSlash(location.path);
- // https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#remarks
- //
- // Only supports up to 256 subrequests in a single batch. The
- // size of the body for a batch request can't exceed 4 MB.
- const int32_t kNumMaxRequestsInBatch = 256;
- options.PageSizeHint = kNumMaxRequestsInBatch;
+ return DeleteDirContentsWihtoutHierarchicalNamespace(location, true);
+ }
+ }
+
+ Status DeleteDirContents(const AzureLocation& location, bool missing_dir_ok) {
+ if (location.container.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
+ }
+ if (location.path.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
Review Comment:
There is also an `allow_bucket_deletion` [1] in s3fs. An analogous to that, but for Azure (let's say `allow_container_deletion`) would be relevant in this PR.
[1] https://github.com/apache/arrow/blob/main/cpp/src/arrow/filesystem/s3fs.cc#L2718
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #38888:
URL: https://github.com/apache/arrow/pull/38888#discussion_r1411517154
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -1017,69 +1089,67 @@ class AzureFileSystem::Impl {
exception);
}
} else {
- auto container_client =
- blob_service_client_->GetBlobContainerClient(location.container);
- Azure::Storage::Blobs::ListBlobsOptions options;
- options.Prefix = internal::EnsureTrailingSlash(location.path);
- // https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#remarks
- //
- // Only supports up to 256 subrequests in a single batch. The
- // size of the body for a batch request can't exceed 4 MB.
- const int32_t kNumMaxRequestsInBatch = 256;
- options.PageSizeHint = kNumMaxRequestsInBatch;
+ return DeleteDirContentsWihtoutHierarchicalNamespace(location, true);
+ }
+ }
+
+ Status DeleteDirContents(const AzureLocation& location, bool missing_dir_ok) {
+ if (location.container.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
+ }
+ if (location.path.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
Review Comment:
If `allow_container_deletion` is relevant to `DeleteDirContents()` and `location.container.empty()` is `true`, the case should be treated in `DeleteRootDirContents()` not `DeleteDirContents()`.
So we can defer it to #38999 anyway. :-)
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #38888:
URL: https://github.com/apache/arrow/pull/38888
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #38888:
URL: https://github.com/apache/arrow/pull/38888#issuecomment-1835332801
Updated:
* Rebased on main
* Don't set `ListBlobOptions.Prefix` when `location.path` is empty
* Extract common test data creation codes as a method
I'll merge this once CI is green.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #38888:
URL: https://github.com/apache/arrow/pull/38888#discussion_r1409837751
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -970,6 +970,79 @@ class AzureFileSystem::Impl {
return stream;
}
+ private:
+ Status DeleteDirContentsWihtoutHierarchicalNamespace(const AzureLocation& location,
+ bool missing_dir_ok) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(location.container);
+ Azure::Storage::Blobs::ListBlobsOptions options;
+ options.Prefix = internal::EnsureTrailingSlash(location.path);
Review Comment:
When `location.path` is empty, the `Prefix` should not be set or be "" (without the trailing slash) as the blob names within the container don't ever start with a `/`.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #38888:
URL: https://github.com/apache/arrow/pull/38888#issuecomment-1837403019
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit db69b67a55ae797a5e3665b963bf7fc6d38cdff5.
There was 1 benchmark result with an error:
- Commit Run on `arm64-m6g-linux-compute` at [2023-12-03 01:33:08Z](https://conbench.ursa.dev/compare/runs/abee1678033d4151b221a5d3ece45ddb...028a03dbd25d437daabdc3c9433f3c9c/)
- [`tpch` (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-15, scale_factor=10](https://conbench.ursa.dev/benchmark-results/0656bf94731a75138000cafde22d7535)
There were 2 benchmark results indicating a performance regression:
- Commit Run on `ursa-i9-9960x` at [2023-12-02 14:48:04Z](https://conbench.ursa.dev/compare/runs/a45d1c49abfc44108d694377dc006d72...7153283a30a440f2b5695462c604eff8/)
- [`tpch` (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-11, scale_factor=1](https://conbench.ursa.dev/compare/benchmarks/0656b31fa17c7f2b8000583365cf6c6e...0656b595f69b733180008dd79bc9348e)
- [`tpch` (R) with engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-11, scale_factor=1](https://conbench.ursa.dev/compare/benchmarks/0656b32048e0735e8000abe046dc0f70...0656b596cfa377578000b29cbf9a6c06)
The [full Conbench report](https://github.com/apache/arrow/runs/19257865919) has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #38888:
URL: https://github.com/apache/arrow/pull/38888#discussion_r1411506534
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -970,6 +970,79 @@ class AzureFileSystem::Impl {
return stream;
}
+ private:
+ Status DeleteDirContentsWihtoutHierarchicalNamespace(const AzureLocation& location,
+ bool missing_dir_ok) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(location.container);
+ Azure::Storage::Blobs::ListBlobsOptions options;
+ options.Prefix = internal::EnsureTrailingSlash(location.path);
Review Comment:
Wow. From the name, I though it **ensured** a trailing slash on the output. 🥲
I like this because `Prefix` is a nullable, so not doing anything to it makes even more sense than setting it to a `""` value.
```cpp
if (!location.path.empty()) {
options.Prefix = internal::EnsureTrailingSlash(location.path);
}
```
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -970,6 +970,79 @@ class AzureFileSystem::Impl {
return stream;
}
+ private:
+ Status DeleteDirContentsWihtoutHierarchicalNamespace(const AzureLocation& location,
+ bool missing_dir_ok) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(location.container);
+ Azure::Storage::Blobs::ListBlobsOptions options;
+ options.Prefix = internal::EnsureTrailingSlash(location.path);
Review Comment:
Wow. From the name, I though it **ensured** a trailing slash on the output. 🥲
I like this because `Prefix` is a nullable, so not doing anything to it makes even more sense than setting it to a `""` value.
```cpp
if (!location.path.empty()) {
options.Prefix = internal::EnsureTrailingSlash(location.path);
}
```
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #38888:
URL: https://github.com/apache/arrow/pull/38888#discussion_r1411527849
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -666,6 +666,119 @@ TEST_F(AzuriteFileSystemTest, DeleteDirUri) {
ASSERT_RAISES(Invalid, fs_->DeleteDir("abfs://" + PreexistingContainerPath()));
}
+TEST_F(AzuriteFileSystemTest, DeleteDirContentsSuccessContainer) {
+#ifdef __APPLE__
+ GTEST_SKIP() << "This test fails by an Azurite problem: "
+ "https://github.com/Azure/Azurite/pull/2302";
+#endif
+ const auto container_path = RandomContainerName();
+ const auto directory_path =
+ internal::ConcatAbstractPath(container_path, RandomDirectoryName());
+ const auto sub_directory_path = internal::ConcatAbstractPath(directory_path, "new-sub");
+ const auto sub_blob_path = internal::ConcatAbstractPath(sub_directory_path, "sub.txt");
+ const auto top_blob_path = internal::ConcatAbstractPath(directory_path, "top.txt");
+ ASSERT_OK(fs_->CreateDir(sub_directory_path, true));
+ ASSERT_OK_AND_ASSIGN(auto output, fs_->OpenOutputStream(sub_blob_path));
+ ASSERT_OK(output->Write(std::string_view("sub")));
+ ASSERT_OK(output->Close());
+ ASSERT_OK_AND_ASSIGN(output, fs_->OpenOutputStream(top_blob_path));
+ ASSERT_OK(output->Write(std::string_view("top")));
+ ASSERT_OK(output->Close());
Review Comment:
OK. I'll extract them as a method.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #38888:
URL: https://github.com/apache/arrow/pull/38888#issuecomment-1831499211
Updated:
* Accept `DeleteDir("container")`
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #38888:
URL: https://github.com/apache/arrow/pull/38888#discussion_r1407999501
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -970,6 +970,78 @@ class AzureFileSystem::Impl {
return stream;
}
+ private:
+ Status DeleteDirContentsWihtoutHierarchicalNamespace(const AzureLocation& location,
+ bool missing_dir_ok) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(location.container);
+ Azure::Storage::Blobs::ListBlobsOptions options;
+ options.Prefix = internal::EnsureTrailingSlash(location.path);
+ // https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#remarks
+ //
+ // Only supports up to 256 subrequests in a single batch. The
+ // size of the body for a batch request can't exceed 4 MB.
+ const int32_t kNumMaxRequestsInBatch = 256;
+ options.PageSizeHint = kNumMaxRequestsInBatch;
+ try {
+ auto list_response = container_client.ListBlobs(options);
+ if (!missing_dir_ok && list_response.Blobs.empty()) {
+ return Status::IOError("Specified directory doesn't exist: ", location.path, ": ",
+ container_client.GetUrl());
+ }
+ while (list_response.HasPage() && !list_response.Blobs.empty()) {
Review Comment:
> ListBlobs() returns HasPage() == true and Blobs.empty() == true response.
Is there a chance of that not being the last page? I see that you now only `continue;` instead of breaking the entire loop which I think is the more robust thing to do. 👍
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -1017,69 +1089,67 @@ class AzureFileSystem::Impl {
exception);
}
} else {
- auto container_client =
- blob_service_client_->GetBlobContainerClient(location.container);
- Azure::Storage::Blobs::ListBlobsOptions options;
- options.Prefix = internal::EnsureTrailingSlash(location.path);
- // https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#remarks
- //
- // Only supports up to 256 subrequests in a single batch. The
- // size of the body for a batch request can't exceed 4 MB.
- const int32_t kNumMaxRequestsInBatch = 256;
- options.PageSizeHint = kNumMaxRequestsInBatch;
+ return DeleteDirContentsWihtoutHierarchicalNamespace(location, true);
+ }
+ }
+
+ Status DeleteDirContents(const AzureLocation& location, bool missing_dir_ok) {
+ if (location.container.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
+ }
+ if (location.path.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
Review Comment:
But what is the root dir? Doesn't that mean *all the containers*?
`s3fs` maps the root-dir concept to *all containers*:
https://github.com/apache/arrow/blob/main/cpp/src/arrow/filesystem/s3fs.cc#L2762
Refusing to delete contents of a container can be very limiting as `CreateDir(container/)` will create a container.
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -970,6 +970,78 @@ class AzureFileSystem::Impl {
return stream;
}
+ private:
+ Status DeleteDirContentsWihtoutHierarchicalNamespace(const AzureLocation& location,
+ bool missing_dir_ok) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(location.container);
+ Azure::Storage::Blobs::ListBlobsOptions options;
+ options.Prefix = internal::EnsureTrailingSlash(location.path);
+ // https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#remarks
+ //
+ // Only supports up to 256 subrequests in a single batch. The
+ // size of the body for a batch request can't exceed 4 MB.
+ const int32_t kNumMaxRequestsInBatch = 256;
+ options.PageSizeHint = kNumMaxRequestsInBatch;
+ try {
+ auto list_response = container_client.ListBlobs(options);
+ if (!missing_dir_ok && list_response.Blobs.empty()) {
+ return Status::IOError("Specified directory doesn't exist: ", location.path, ": ",
+ container_client.GetUrl());
+ }
+ while (list_response.HasPage() && !list_response.Blobs.empty()) {
Review Comment:
> ListBlobs() returns HasPage() == true and Blobs.empty() == true response.
Is there a chance of that not being the last page? I see that you now only `continue;` instead of breaking the entire loop which I think is the more robust thing to do. 👍
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38888:
URL: https://github.com/apache/arrow/pull/38888#issuecomment-1827292143
:warning: GitHub issue #38701 **has been automatically assigned in GitHub** to PR creator.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #38888:
URL: https://github.com/apache/arrow/pull/38888#discussion_r1408956292
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -1017,69 +1089,67 @@ class AzureFileSystem::Impl {
exception);
}
} else {
- auto container_client =
- blob_service_client_->GetBlobContainerClient(location.container);
- Azure::Storage::Blobs::ListBlobsOptions options;
- options.Prefix = internal::EnsureTrailingSlash(location.path);
- // https://learn.microsoft.com/en-us/rest/api/storageservices/blob-batch#remarks
- //
- // Only supports up to 256 subrequests in a single batch. The
- // size of the body for a batch request can't exceed 4 MB.
- const int32_t kNumMaxRequestsInBatch = 256;
- options.PageSizeHint = kNumMaxRequestsInBatch;
+ return DeleteDirContentsWihtoutHierarchicalNamespace(location, true);
+ }
+ }
+
+ Status DeleteDirContents(const AzureLocation& location, bool missing_dir_ok) {
+ if (location.container.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
+ }
+ if (location.path.empty()) {
+ return internal::InvalidDeleteDirContents(location.all);
Review Comment:
OK. I'll do it.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org