GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` #38888

kou · 2023-11-27T07:50:40Z

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.

Closes: [C++][FS][Azure] Implement DeleteDirContents() #38701

github-actions · 2023-11-27T07:51:07Z

⚠️ GitHub issue #38701 has been automatically assigned in GitHub to PR creator.

kou · 2023-11-27T07:51:28Z

@Tom-Newton @felipecrv You may want to review this.

felipecrv · 2023-11-28T03:40:10Z

cpp/src/arrow/filesystem/azurefs.cc

Shouldn't this be a PathNotFound error?

Good catch!

felipecrv · 2023-11-28T03:42:21Z

cpp/src/arrow/filesystem/azurefs.cc

This could be me being overly cautious, but maybe we shouldn't stop when Blobs.empty(), but instead run continue; and evaluate MoveToNextPage() again?

Another thing: instead of while, you can use for (; list_response.HasPage(); list_response.MoveToNextPage()) { here.

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

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. 👍

(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...)

felipecrv · 2023-11-28T03:44:05Z

cpp/src/arrow/filesystem/azurefs.cc

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 /).

I think that we should do it in #38772 .
We have a test for this case. So we can detect the case when we work on #38772 .

felipecrv · 2023-11-28T03:45:44Z

cpp/src/arrow/filesystem/azurefs.cc

/*missing_dir_ok=*/true

OK. I'll add the comment.

felipecrv · 2023-11-28T03:46:33Z

cpp/src/arrow/filesystem/azurefs.cc

Shouldn't this be interpreted as deleting the contents of the container?

We have a special API for this case: DeleteRootDirContents()

arrow/cpp/src/arrow/filesystem/filesystem.h

Lines 233 to 249 in 63353ba

/// Delete a directory's contents, recursively.

///

/// Like DeleteDir, but doesn't delete the directory itself.

/// Passing an empty path ("" or "/") is disallowed, see DeleteRootDirContents.

virtual Status DeleteDirContents(const std::string& path,

bool missing_dir_ok = false) = 0;

/// Async version of DeleteDirContents.

virtual Future<> DeleteDirContentsAsync(const std::string& path,

bool missing_dir_ok = false);

/// EXPERIMENTAL: Delete the root directory's contents, recursively.

///

/// Implementations may decide to raise an error if this operation is

/// too dangerous.

// NOTE: may decide to remove this if it's deemed not useful

virtual Status DeleteRootDirContents() = 0;

So I think that we should return an error here.

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.

Can we clarify this discussion?

What should be "root dir"?

: Empty location (no container and no path)

container and container/: Container only

container/path: Container and path

My opinion and the current implementation:

Root dir

Root dir

Not root dir

FYI: s3fs:

: Empty path (no bucket and no key): Root dir

bucket and bucket/: Bucket only: Not root dir

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.

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.

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

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.

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.

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.

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. :-)

kou · 2023-11-28T05:31:21Z

Updated:

Use PathNotFound()
Use for
Add an argument name comment
Skip a test on macOS that has a problem with Azurite

felipecrv · 2023-11-28T15:31:36Z

cpp/src/arrow/filesystem/azurefs.cc

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.

felipecrv · 2023-11-28T15:54:12Z

cpp/src/arrow/filesystem/azurefs.cc

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. 👍

kou · 2023-11-29T09:10:32Z

Updated:

Accept DeleteDir("container")

felipecrv · 2023-11-29T20:37:18Z

cpp/src/arrow/filesystem/azurefs.cc

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 /.

EnsureTrailingSlash() does nothing for empty string:

arrow/cpp/src/arrow/filesystem/path_util.cc

Lines 151 to 159 in d66780d

std::string EnsureTrailingSlash(std::string_view v) {

if (v.length() > 0 && v.back() != kSep) {

// XXX How about "C:" on Windows? We probably don't want to turn it into "C:/"...

// Unless the local filesystem always uses absolute paths

return std::string(v) + kSep;

} else {

return std::string(v);

}

}

Should we add a (needless) special condition such as the following for readability?

if (!location.path.empty()) { options.Prefix = internal::EnsureTrailingSlash(location.path); }

Maybe just leave a comment saying that EnsureTrailingSlash doesn't alter empty string.

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.

if (!location.path.empty()) { options.Prefix = internal::EnsureTrailingSlash(location.path); }

OK. I'll use the if (!location.path.empty()).

Tom-Newton

I'm happy. Thanks

Tom-Newton · 2023-11-30T18:16:50Z

cpp/src/arrow/filesystem/azurefs_test.cc

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.

OK. I'll extract them as a method.

Tom-Newton · 2023-11-30T18:22:35Z

cpp/src/arrow/filesystem/azurefs.cc

Maybe just leave a comment saying that EnsureTrailingSlash doesn't alter empty string.

Tom-Newton · 2023-11-30T18:56:08Z

cpp/src/arrow/filesystem/azurefs.cc

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.

felipecrv

There are a few nits by @Tom-Newton for you to fix but correctness-wise, I think this is ready for merge.

kou · 2023-12-01T02:28:57Z

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.

conbench-apache-arrow · 2023-12-03T08:05:28Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit db69b67.

There was 1 benchmark result with an error:

Commit Run on arm64-m6g-linux-compute at 2023-12-03 01:33:08Z
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-15, scale_factor=10

There were 2 benchmark results indicating a performance regression:

Commit Run on ursa-i9-9960x at 2023-12-02 14:48:04Z
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-11, scale_factor=1
- tpch (R) with engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-11, scale_factor=1

The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.

…che#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. * Closes: apache#38701 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

github-actions bot added Component: C++ awaiting committer review Awaiting committer review labels Nov 27, 2023

felipecrv reviewed Nov 28, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting committer review Awaiting committer review awaiting changes Awaiting changes labels Nov 28, 2023

felipecrv approved these changes Nov 28, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting change review Awaiting change review awaiting changes Awaiting changes labels Nov 29, 2023

felipecrv reviewed Nov 29, 2023

View reviewed changes

kou mentioned this pull request Nov 30, 2023

[C++][FS][Azure] Add allow_container_deletion option #38999

Open

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Nov 30, 2023

Tom-Newton approved these changes Nov 30, 2023

View reviewed changes

felipecrv approved these changes Dec 1, 2023

View reviewed changes

kou added 8 commits December 1, 2023 11:08

apacheGH-38701: [C++][FS][Azure] Implement DeleteDirContents()

cc40c20

Use PathNotFound

f9e1846

Use for

19b4f6d

Add argument name comment

e0bc11b

Skip a test on macOS

510cfa0

Container isn't root directory

e573afd

Prefer null prefix to empty prefix

b1f07d6

Extract common codes as a convenient method

651769a

kou force-pushed the cpp-azurefs-delete-dir-contents branch from 2e40f6b to 651769a Compare December 1, 2023 02:26

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Dec 1, 2023

Fix lint

2d30676

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Dec 1, 2023

felipecrv approved these changes Dec 1, 2023

View reviewed changes

kou merged commit db69b67 into apache:main Dec 1, 2023

kou deleted the cpp-azurefs-delete-dir-contents branch December 1, 2023 20:43

kou removed the awaiting change review Awaiting change review label Dec 1, 2023

	/// Delete a directory's contents, recursively.
	///
	/// Like DeleteDir, but doesn't delete the directory itself.
	/// Passing an empty path ("" or "/") is disallowed, see DeleteRootDirContents.
	virtual Status DeleteDirContents(const std::string& path,
	bool missing_dir_ok = false) = 0;

	/// Async version of DeleteDirContents.
	virtual Future<> DeleteDirContentsAsync(const std::string& path,
	bool missing_dir_ok = false);

	/// EXPERIMENTAL: Delete the root directory's contents, recursively.
	///
	/// Implementations may decide to raise an error if this operation is
	/// too dangerous.
	// NOTE: may decide to remove this if it's deemed not useful
	virtual Status DeleteRootDirContents() = 0;

	std::string EnsureTrailingSlash(std::string_view v) {
	if (v.length() > 0 && v.back() != kSep) {
	// XXX How about "C:" on Windows? We probably don't want to turn it into "C:/"...
	// Unless the local filesystem always uses absolute paths
	return std::string(v) + kSep;
	} else {
	return std::string(v);
	}
	}

GH-38701: [C++][FS][Azure] Implement DeleteDirContents() #38888

GH-38701: [C++][FS][Azure] Implement DeleteDirContents() #38888

Uh oh!

Conversation

kou commented Nov 27, 2023 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Nov 27, 2023

Uh oh!

kou commented Nov 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tom-Newton Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kou commented Nov 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kou commented Nov 29, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tom-Newton left a comment

Choose a reason for hiding this comment

Uh oh!

GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` #38888

GH-38701: [C++][FS][Azure] Implement `DeleteDirContents()` #38888

kou commented Nov 27, 2023 •

edited by github-actions bot

Loading

Tom-Newton Nov 29, 2023 •

edited

Loading

Tom-Newton Nov 30, 2023 •

edited

Loading