GH-38699: [C++][FS][Azure] Implement `CreateDir()` #38708

kou · 2023-11-14T07:55:57Z

Rationale for this change

It seems that we can't create a directory explicitly without hierarchical namespace support.

It seems that Azure Blob Storage supports only virtual directory. There is no directory. If a file (blob) name has "/", it's treated that the file (blob) exists under a virtual directory.

It seems that Azure Data Lake Storage Gen2 supports a real directory.

See also:
https://learn.microsoft.com/en-us/azure/storage/blobs/storage-blobs-introduction

What changes are included in this PR?

This change chooses the following behavior:

Container can be created with/without hierarchical namespace support.
Directory can be created with hierarchical namespace support.
Directory can't be created without hierarchical namespace support. So do nothing without hierachical namespace support. (arrow::Status::OK() is just returned.)

Are these changes tested?

Azurite doesn't support hierarchical namespace yet. So I can't test the implementation for hierarchical namespace yet. Sorry.

Are there any user-facing changes?

Yes.

Closes: [C++][FS][Azure] Implement CreateDir() #38699

It seems that we can't create a directory explicitly without hierarchical namespace support. It seems that Azure Blob Storage supports only virtual directory. There is no directory. If a file (blob) name has "/", it's treated that the file (blob) exists under a virtual directory. It seems that Azure Data Lake Storage Gen2 supports a real directory. See also: https://learn.microsoft.com/en-us/azure/storage/blobs/storage-blobs-introduction This change chooses the following behavior: * Container can be created with/without hierarchical namespace support. * Directory can be created with hierarchical namespace support. * Directory can't be created without hierarchical namespace support. (`arrow::Status::NotImplemented` is returned for this case.) Azurite doesn't support hierarchical namespace yet. So I can't test the implementation for hierarchical namespace yet. Sorry.

kou · 2023-11-14T08:00:38Z

@Tom-Newton What do you think about this behavior?

This change chooses the following behavior:

Container can be created with/without hierarchical namespace support.

Directory can be created with hierarchical namespace support.

Directory can't be created without hierarchical namespace support. (arrow::Status::NotImplemented is returned for this case.)

Do you have any (simple) document how to setup an account for AzureHierarchicalNamespaceFileSystemTest?

Tom-Newton · 2023-11-14T08:18:03Z

What do you think about this behavior?

The only part I'm questioning is whether to return arrow::Status::NotImplemented on flat blob storage accounts. Possibly it would be better just to return a success status without doing anything.

I think the right choice will depend on how much CreateDir() is used. For example if CreateDir() is used every time arrow writes a partitioned parquet table, then returning an error status could be a bit of a problem.

Tom-Newton · 2023-11-14T08:38:02Z

Do you have any (simple) document how to setup an account for AzureHierarchicalNamespaceFileSystemTest?

You will need an azure account. You should be able to create a free account at https://azure.microsoft.com/en-gb/free/. You should the. Be able to create a storage account through the portal web UI.

I can probably write a more specific doc if needed but this is Azure's doc https://learn.microsoft.com/en-us/azure/storage/blobs/create-data-lake-storage-account

A few suggestions on configuration:

Use Standard general-purpose v2 not premium
Use LRS redundancy
Obviously you will want to enable hierarchical namespace.
Set the default access tier to hot
SFTP, NFS and file shares are not required.

felipecrv · 2023-11-15T01:46:47Z

cpp/src/arrow/filesystem/azurefs.cc

+          std::string_view body_text(reinterpret_cast<const char*>(body.data()),
+                                     body.size());


Is it safe to put this in a string? We are sure this is plain text?

I hope so. I expect that the body includes a detailed error message and it's helpful for debug.
But it's not an Azure spec.
Should we avoid this for safety?
Or can we keep this as-is with a concern comment?

It's fine to keep as is.

OK. I'll add a comment.

felipecrv · 2023-11-15T02:28:05Z

cpp/src/arrow/filesystem/azurefs.cc

+          "Cannot create a directory without hierarchical namespace: ", path.full_path);
+    }
+    auto directory_client = datalake_service_client_->GetFileSystemClient(path.container)
+                                .GetDirectoryClient(path.path_to_file);


I can see why path_to_file is actually the name of the directory in this context, but maybe a different name for this struct field would make things less confusing? This segment of filesystem paths is usually called "basename" [1].

[1] https://en.wikipedia.org/wiki/Basename

I spent quite some time trying to figure out what if (path.path_to_file.empty()) { meant here.

path.basename.empty() would be more clear IMO.

cc @Tom-Newton

I think that "basename" isn't suitable for this case.
I think that "d" is the basename of "a/b/c/d" but path_to_file is "b/c/d". ("a" is container.)

I think that path is suitable for "b/c/d" but AzurePath::path is strange... How about renaming AzurePath to AzureLocation and using container for a and path for b/c/d?

FYI:

Azure Blob Storage calls "a" as "container name" and "b/c/d" as "blob name" in https://learn.microsoft.com/en-us/azure/storage/blobs/storage-blobs-introduction .

Azure Data Lake Storage Gen2 calls "a/b/c" as "path" and "d" as "file_name" in https://learn.microsoft.com/en-us/azure/storage/blobs/data-lake-storage-introduction-abfs-uri .

We don't support abfs://... URI yet. (FileSystemFromUri() doesn't accept abfs://... yet.)

Oh. I misinterpreted the meaning of path_to_file. I think path would be OK. And Azure{Path->Location} is also a good rename.

👍 for @kou's the suggested re-name.

I've opened a new issue for this: #38758
I'll do it after we merge this.

felipecrv · 2023-11-15T02:35:15Z

cpp/src/arrow/filesystem/azurefs.cc

+                          hierarchical_namespace_.Enabled(path.container));
+    if (!hierarchical_namespace_enabled) {
+      return Status::NotImplemented(
+          "Cannot create a directory without hierarchical namespace: ", path.full_path);


I would write "...without hierarchical namespace support" so the user is less likely to interpret this as some missing part of the path passed as argument.

Thanks! Wording suggestion is always welcome for me because I'm not a native English speaker!
I'll add "support".

Thanks! Wording suggestion is always welcome for me because I'm not a native English speaker!

No worries. This could happen to native English speakers as well. :)

kou · 2023-11-16T01:56:28Z

The only part I'm questioning is whether to return arrow::Status::NotImplemented on flat blob storage accounts. Possibly it would be better just to return a success status without doing anything.

I think the right choice will depend on how much CreateDir() is used. For example if CreateDir() is used every time arrow writes a partitioned parquet table, then returning an error status could be a bit of a problem.

Good point! I'll change the behavior to just return arrow::Status::OK().

kou · 2023-11-16T01:58:42Z

Do you have any (simple) document how to setup an account for AzureHierarchicalNamespaceFileSystemTest?

You will need an azure account. You should be able to create a free account at https://azure.microsoft.com/en-gb/free/. You should the. Be able to create a storage account through the portal web UI.

I can probably write a more specific doc if needed but this is Azure's doc https://learn.microsoft.com/en-us/azure/storage/blobs/create-data-lake-storage-account

A few suggestions on configuration:

...

Thanks! I could create an account and confirm that the implementation passes the added tests.

I'll add the provided information as a comment for other developers.

kou · 2023-11-16T05:23:00Z

I've updated.

cpp/src/arrow/filesystem/azurefs.cc

Tom-Newton · 2023-11-16T19:28:11Z

cpp/src/arrow/filesystem/azurefs.cc

+          "Cannot create a directory without hierarchical namespace: ", path.full_path);
+    }
+    auto directory_client = datalake_service_client_->GetFileSystemClient(path.container)
+                                .GetDirectoryClient(path.path_to_file);


👍 for @kou's the suggested re-name.

Tom-Newton · 2023-11-16T21:09:06Z

cpp/src/arrow/filesystem/azurefs.cc

+    }
+
+    std::string current_path;
+    for (const auto& part : path.path_to_file_parts) {


I just did a quick test. It seems like directory_client.CreateIfNotExists() actually creates the directories recursively by default, so probably we don't need this for loop.

Wow! Really!? I'll also check it.

I confirmed that we don't need this loop too.
I'll simplify this.

I think the AzureHierarchicalNamespaceFileSystemTest, CreateDirRecursiveSuccessContainerOnly test is actually broken on main. It seems like directory_client.CreateIfNotExists() fails if the directory_client is just referencing the root of the container. Probably we need some logic to skip the call to directory_client.CreateIfNotExists() if location.path.empty()

Oh, sorry.
I'll fix it: #38782

Co-authored-by: Thomas Newton <thomas.w.newton@gmail.com>

kou · 2023-11-17T06:03:45Z

I've updated.

felipecrv · 2023-11-17T22:20:41Z

cpp/src/arrow/filesystem/azurefs_test.cc

+  // There is only virtual directory without hierarchical namespace
+  // support. So the CreateDir() does nothing.
+  arrow::fs::AssertFileInfo(fs_.get(), path, FileType::NotFound);
+  arrow::fs::AssertFileInfo(fs_.get(), parent, FileType::NotFound);


If we can disambiguate "file exists?" queries in the filesystem API, we should probably always reply true when the caller is asking if a directory exists. If creating a directory is a no-op that succeeds, the post-condition of CreateDir -- the directory now exists -- should be true.

There are might be bad consequences of this, so this is more of an idea than a suggestion.

It's an interring point. I don't feel strongly but personally I think the current behaviour is the best option.

GetFileInfo will return that a directory is present if at least one "file" has been created in that "directory". I think this behaviour is consistent with the GCS filesystem.

I agree with the opinion of Tom-Newton but let's discuss this further on #38772.

kou · 2023-11-18T02:08:35Z

I'll merge this.

conbench-apache-arrow · 2023-11-18T05:24:14Z

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

There were no benchmark performance regressions. 🎉

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

### Rationale for this change It seems that we can't create a directory explicitly without hierarchical namespace support. It seems that Azure Blob Storage supports only virtual directory. There is no directory. If a file (blob) name has "/", it's treated that the file (blob) exists under a virtual directory. It seems that Azure Data Lake Storage Gen2 supports a real directory. See also: https://learn.microsoft.com/en-us/azure/storage/blobs/storage-blobs-introduction ### What changes are included in this PR? This change chooses the following behavior: * Container can be created with/without hierarchical namespace support. * Directory can be created with hierarchical namespace support. * Directory can't be created without hierarchical namespace support. So do nothing without hierachical namespace support. (`arrow::Status::OK()` is just returned.) ### Are these changes tested? Azurite doesn't support hierarchical namespace yet. So I can't test the implementation for hierarchical namespace yet. Sorry. ### Are there any user-facing changes? Yes. * Closes: apache#38699 Lead-authored-by: Sutou Kouhei <kou@clear-code.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Co-authored-by: Thomas Newton <thomas.w.newton@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

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

felipecrv reviewed Nov 15, 2023

View reviewed changes

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

Do nothing without hierarchical namespace support

5dcac69

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

Add a comment how to enable tests with hierarchical namespace

aabc7b2

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Nov 16, 2023

Tom-Newton reviewed Nov 16, 2023

View reviewed changes

kou mentioned this pull request Nov 17, 2023

[C++][FS][Azure] Rename AzurePath to AzureLocation #38758

Closed

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

Improve description about virtual directory

6a16e9f

Co-authored-by: Thomas Newton <thomas.w.newton@gmail.com>

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 Nov 17, 2023

kou added 3 commits November 17, 2023 12:58

Add a comment about response body

4d312ad

Format

60890c6

Simplify

9dbc1c9

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

felipecrv reviewed Nov 17, 2023

View reviewed changes

felipecrv approved these changes Nov 17, 2023

View reviewed changes

kou mentioned this pull request Nov 18, 2023

[C++][FS][Azure] Should GetFileInfo() against a directory always return true without hierarchical namespace support? #38772

Closed

kou merged commit a394a39 into apache:main Nov 18, 2023
36 of 37 checks passed

github-actions bot removed the awaiting change review Awaiting change review label Nov 18, 2023

kou deleted the fs-azure-create-dir branch November 18, 2023 02:09

github-actions bot added the awaiting changes Awaiting changes label Nov 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-38699: [C++][FS][Azure] Implement `CreateDir()` #38708

GH-38699: [C++][FS][Azure] Implement `CreateDir()` #38708

kou commented Nov 14, 2023 •

edited

Loading

kou commented Nov 14, 2023

Tom-Newton commented Nov 14, 2023 •

edited

Loading

Tom-Newton commented Nov 14, 2023

felipecrv Nov 15, 2023

kou Nov 16, 2023

felipecrv Nov 16, 2023

kou Nov 17, 2023

felipecrv Nov 15, 2023 •

edited

Loading

felipecrv Nov 15, 2023

kou Nov 16, 2023

felipecrv Nov 16, 2023

Tom-Newton Nov 16, 2023

kou Nov 17, 2023

felipecrv Nov 15, 2023

kou Nov 16, 2023

felipecrv Nov 16, 2023

kou commented Nov 16, 2023

kou commented Nov 16, 2023

kou commented Nov 16, 2023

Tom-Newton Nov 16, 2023

Tom-Newton Nov 16, 2023 •

edited

Loading

kou Nov 17, 2023

kou Nov 17, 2023

Tom-Newton Nov 19, 2023

kou Nov 19, 2023

kou Nov 19, 2023

kou commented Nov 17, 2023

felipecrv Nov 17, 2023

Tom-Newton Nov 17, 2023 •

edited

Loading

kou Nov 18, 2023

kou commented Nov 18, 2023

conbench-apache-arrow bot commented Nov 18, 2023

		std::string_view body_text(reinterpret_cast<const char*>(body.data()),
		body.size());

GH-38699: [C++][FS][Azure] Implement CreateDir() #38708

GH-38699: [C++][FS][Azure] Implement CreateDir() #38708

Conversation

kou commented Nov 14, 2023 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

kou commented Nov 14, 2023

Tom-Newton commented Nov 14, 2023 • edited Loading

Tom-Newton commented Nov 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipecrv Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kou commented Nov 16, 2023

kou commented Nov 16, 2023

kou commented Nov 16, 2023

Choose a reason for hiding this comment

Tom-Newton Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kou commented Nov 17, 2023

Choose a reason for hiding this comment

Tom-Newton Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kou commented Nov 18, 2023

conbench-apache-arrow bot commented Nov 18, 2023

GH-38699: [C++][FS][Azure] Implement `CreateDir()` #38708

GH-38699: [C++][FS][Azure] Implement `CreateDir()` #38708

kou commented Nov 14, 2023 •

edited

Loading

Tom-Newton commented Nov 14, 2023 •

edited

Loading

felipecrv Nov 15, 2023 •

edited

Loading

Tom-Newton Nov 16, 2023 •

edited

Loading

Tom-Newton Nov 17, 2023 •

edited

Loading