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/14 07:55:59 UTC
[PR] GH-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
kou opened a new pull request, #38708:
URL: https://github.com/apache/arrow/pull/38708
### 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. (`arrow::Status::NotImplemented` is returned for this case.)
### 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.
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on PR #38708:
URL: https://github.com/apache/arrow/pull/38708#issuecomment-1809728356
> 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.
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #38708:
URL: https://github.com/apache/arrow/pull/38708#discussion_r1396670968
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -611,6 +621,109 @@ class AzureFileSystem::Impl {
RETURN_NOT_OK(ptr->Init());
return ptr;
}
+
+ Status CreateDir(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ if (path.path_to_file.empty()) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ auto response = container_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ return StatusFromErrorResponse(
+ container_client.GetUrl(), response.RawResponse.get(),
+ "Failed to create a container: " + path.container);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + ": " +
+ container_client.GetUrl(),
+ exception);
+ }
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ // We can't create a directory without hierarchical namespace
+ // support. There is only "virtual directory" without
+ // hierarchical namespace support. And a "virtual directory" is
+ // (virtually) created a blob with ".../.../blob" blob name
+ // automatically.
Review Comment:
Thanks for the suggestion!
I'll use the suggested wording!
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #38708:
URL: https://github.com/apache/arrow/pull/38708#discussion_r1395808537
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -611,6 +611,110 @@ class AzureFileSystem::Impl {
RETURN_NOT_OK(ptr->Init());
return ptr;
}
+
+ Status CreateDir(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ if (path.path_to_file.empty()) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ auto response = container_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ const auto& body = response.RawResponse->GetBody();
+ std::string_view body_text(reinterpret_cast<const char*>(body.data()),
+ body.size());
+ return Status::IOError("Failed to create a container: ", path.container, " (",
+ container_client.GetUrl(),
+ "): ", response.RawResponse->GetReasonPhrase(), " (",
+ static_cast<int>(response.RawResponse->GetStatusCode()),
+ "): ", body_text);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + " (" +
+ container_client.GetUrl() + ")",
+ exception);
+ }
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ return Status::NotImplemented(
+ "Cannot create a directory without hierarchical namespace: ", path.full_path);
+ }
+ auto directory_client = datalake_service_client_->GetFileSystemClient(path.container)
+ .GetDirectoryClient(path.path_to_file);
Review Comment:
Oh. I misinterpreted the meaning of `path_to_file`. I think `path` would be OK. And `Azure{Path->Location}` is also a good rename.
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #38708:
URL: https://github.com/apache/arrow/pull/38708#issuecomment-1813808383
I've updated.
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on code in PR #38708:
URL: https://github.com/apache/arrow/pull/38708#discussion_r1396338372
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -611,6 +621,109 @@ class AzureFileSystem::Impl {
RETURN_NOT_OK(ptr->Init());
return ptr;
}
+
+ Status CreateDir(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ if (path.path_to_file.empty()) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ auto response = container_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ return StatusFromErrorResponse(
+ container_client.GetUrl(), response.RawResponse.get(),
+ "Failed to create a container: " + path.container);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + ": " +
+ container_client.GetUrl(),
+ exception);
+ }
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ // We can't create a directory without hierarchical namespace
+ // support. There is only "virtual directory" without
+ // hierarchical namespace support. And a "virtual directory" is
+ // (virtually) created a blob with ".../.../blob" blob name
+ // automatically.
+ return Status::OK();
+ }
+
+ auto directory_client = datalake_service_client_->GetFileSystemClient(path.container)
+ .GetDirectoryClient(path.path_to_file);
+ try {
+ auto response = directory_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ return StatusFromErrorResponse(
+ directory_client.GetUrl(), response.RawResponse.get(),
+ "Failed to create a directory: " + path.path_to_file);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a directory: " + path.path_to_file + ": " +
+ directory_client.GetUrl(),
+ exception);
+ }
+ }
+
+ Status CreateDirRecursive(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ auto container_client = blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ container_client.CreateIfNotExists();
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + " (" +
+ container_client.GetUrl() + ")",
+ exception);
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ // We can't create a directory without hierarchical namespace
+ // support. There is only "virtual directory" without
+ // hierarchical namespace support. And a "virtual directory" is
+ // (virtually) created a blob with ".../.../blob" blob name
+ // automatically.
+ return Status::OK();
+ }
+
+ std::string current_path;
+ for (const auto& part : path.path_to_file_parts) {
Review Comment:
I just did a quick test. It seems like `directory_client.CreateIfNotExists()` actually creates the directories recursively be default, so probably we don't need this for loop.
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -611,6 +621,109 @@ class AzureFileSystem::Impl {
RETURN_NOT_OK(ptr->Init());
return ptr;
}
+
+ Status CreateDir(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ if (path.path_to_file.empty()) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ auto response = container_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ return StatusFromErrorResponse(
+ container_client.GetUrl(), response.RawResponse.get(),
+ "Failed to create a container: " + path.container);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + ": " +
+ container_client.GetUrl(),
+ exception);
+ }
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ // We can't create a directory without hierarchical namespace
+ // support. There is only "virtual directory" without
+ // hierarchical namespace support. And a "virtual directory" is
+ // (virtually) created a blob with ".../.../blob" blob name
+ // automatically.
Review Comment:
Personally I would reword this a bit. I wouldn't say that anything is created automatically. Blob storage clients just tend to imply directories when they see `/`s in the blob names.
```suggestion
// Without hierarchical namespace enabled Azure blob storage has no directories.
// Therefore we can't, and don't need to create one. Simply creating a blob with `/` in the name
// implies directories.
```
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -611,6 +611,110 @@ class AzureFileSystem::Impl {
RETURN_NOT_OK(ptr->Init());
return ptr;
}
+
+ Status CreateDir(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ if (path.path_to_file.empty()) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ auto response = container_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ const auto& body = response.RawResponse->GetBody();
+ std::string_view body_text(reinterpret_cast<const char*>(body.data()),
+ body.size());
+ return Status::IOError("Failed to create a container: ", path.container, " (",
+ container_client.GetUrl(),
+ "): ", response.RawResponse->GetReasonPhrase(), " (",
+ static_cast<int>(response.RawResponse->GetStatusCode()),
+ "): ", body_text);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + " (" +
+ container_client.GetUrl() + ")",
+ exception);
+ }
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ return Status::NotImplemented(
+ "Cannot create a directory without hierarchical namespace: ", path.full_path);
+ }
+ auto directory_client = datalake_service_client_->GetFileSystemClient(path.container)
+ .GetDirectoryClient(path.path_to_file);
Review Comment:
:+1: for @kou's the suggested re-name.
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #38708:
URL: https://github.com/apache/arrow/pull/38708#issuecomment-1813644050
> 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()`.
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #38708:
URL: https://github.com/apache/arrow/pull/38708#discussion_r1393558114
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -611,6 +611,110 @@ class AzureFileSystem::Impl {
RETURN_NOT_OK(ptr->Init());
return ptr;
}
+
+ Status CreateDir(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ if (path.path_to_file.empty()) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ auto response = container_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ const auto& body = response.RawResponse->GetBody();
+ std::string_view body_text(reinterpret_cast<const char*>(body.data()),
+ body.size());
+ return Status::IOError("Failed to create a container: ", path.container, " (",
+ container_client.GetUrl(),
+ "): ", response.RawResponse->GetReasonPhrase(), " (",
+ static_cast<int>(response.RawResponse->GetStatusCode()),
+ "): ", body_text);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + " (" +
+ container_client.GetUrl() + ")",
+ exception);
+ }
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ return Status::NotImplemented(
+ "Cannot create a directory without hierarchical namespace: ", path.full_path);
+ }
+ auto directory_client = datalake_service_client_->GetFileSystemClient(path.container)
+ .GetDirectoryClient(path.path_to_file);
Review Comment:
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 would make things less confusing? This segment of filesystem paths is usually called "basename" [1].
[1] https://en.wikipedia.org/wiki/Basename
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -611,6 +611,110 @@ class AzureFileSystem::Impl {
RETURN_NOT_OK(ptr->Init());
return ptr;
}
+
+ Status CreateDir(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ if (path.path_to_file.empty()) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ auto response = container_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ const auto& body = response.RawResponse->GetBody();
+ std::string_view body_text(reinterpret_cast<const char*>(body.data()),
+ body.size());
+ return Status::IOError("Failed to create a container: ", path.container, " (",
+ container_client.GetUrl(),
+ "): ", response.RawResponse->GetReasonPhrase(), " (",
+ static_cast<int>(response.RawResponse->GetStatusCode()),
+ "): ", body_text);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + " (" +
+ container_client.GetUrl() + ")",
+ exception);
+ }
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ return Status::NotImplemented(
+ "Cannot create a directory without hierarchical namespace: ", path.full_path);
+ }
+ auto directory_client = datalake_service_client_->GetFileSystemClient(path.container)
+ .GetDirectoryClient(path.path_to_file);
Review Comment:
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
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -611,6 +611,110 @@ class AzureFileSystem::Impl {
RETURN_NOT_OK(ptr->Init());
return ptr;
}
+
+ Status CreateDir(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ if (path.path_to_file.empty()) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ auto response = container_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ const auto& body = response.RawResponse->GetBody();
+ std::string_view body_text(reinterpret_cast<const char*>(body.data()),
+ body.size());
+ return Status::IOError("Failed to create a container: ", path.container, " (",
+ container_client.GetUrl(),
+ "): ", response.RawResponse->GetReasonPhrase(), " (",
+ static_cast<int>(response.RawResponse->GetStatusCode()),
+ "): ", body_text);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + " (" +
+ container_client.GetUrl() + ")",
+ exception);
+ }
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ return Status::NotImplemented(
+ "Cannot create a directory without hierarchical namespace: ", path.full_path);
+ }
+ auto directory_client = datalake_service_client_->GetFileSystemClient(path.container)
+ .GetDirectoryClient(path.path_to_file);
+ try {
+ auto response = directory_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ const auto& body = response.RawResponse->GetBody();
+ std::string_view body_text(reinterpret_cast<const char*>(body.data()),
+ body.size());
+ return Status::IOError("Failed to create a directory: ", path.path_to_file, " (",
+ directory_client.GetUrl(),
+ "): ", response.RawResponse->GetReasonPhrase(), " (",
+ static_cast<int>(response.RawResponse->GetStatusCode()),
+ "): ", body_text);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a directory: " + path.path_to_file + " (" +
+ directory_client.GetUrl() + ")",
+ exception);
+ }
+ }
+
+ Status CreateDirRecursive(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ auto container_client = blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ container_client.CreateIfNotExists();
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + " (" +
+ container_client.GetUrl() + ")",
+ exception);
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ return Status::NotImplemented(
+ "Cannot create a directory without hierarchical namespace: ", path.full_path);
Review Comment:
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.
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #38708:
URL: https://github.com/apache/arrow/pull/38708#discussion_r1393558114
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -611,6 +611,110 @@ class AzureFileSystem::Impl {
RETURN_NOT_OK(ptr->Init());
return ptr;
}
+
+ Status CreateDir(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ if (path.path_to_file.empty()) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ auto response = container_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ const auto& body = response.RawResponse->GetBody();
+ std::string_view body_text(reinterpret_cast<const char*>(body.data()),
+ body.size());
+ return Status::IOError("Failed to create a container: ", path.container, " (",
+ container_client.GetUrl(),
+ "): ", response.RawResponse->GetReasonPhrase(), " (",
+ static_cast<int>(response.RawResponse->GetStatusCode()),
+ "): ", body_text);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + " (" +
+ container_client.GetUrl() + ")",
+ exception);
+ }
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ return Status::NotImplemented(
+ "Cannot create a directory without hierarchical namespace: ", path.full_path);
+ }
+ auto directory_client = datalake_service_client_->GetFileSystemClient(path.container)
+ .GetDirectoryClient(path.path_to_file);
Review Comment:
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
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #38708:
URL: https://github.com/apache/arrow/pull/38708#discussion_r1398332037
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -611,6 +621,109 @@ class AzureFileSystem::Impl {
RETURN_NOT_OK(ptr->Init());
return ptr;
}
+
+ Status CreateDir(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ if (path.path_to_file.empty()) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ auto response = container_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ return StatusFromErrorResponse(
+ container_client.GetUrl(), response.RawResponse.get(),
+ "Failed to create a container: " + path.container);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + ": " +
+ container_client.GetUrl(),
+ exception);
+ }
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ // We can't create a directory without hierarchical namespace
+ // support. There is only "virtual directory" without
+ // hierarchical namespace support. And a "virtual directory" is
+ // (virtually) created a blob with ".../.../blob" blob name
+ // automatically.
+ return Status::OK();
+ }
+
+ auto directory_client = datalake_service_client_->GetFileSystemClient(path.container)
+ .GetDirectoryClient(path.path_to_file);
+ try {
+ auto response = directory_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ return StatusFromErrorResponse(
+ directory_client.GetUrl(), response.RawResponse.get(),
+ "Failed to create a directory: " + path.path_to_file);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a directory: " + path.path_to_file + ": " +
+ directory_client.GetUrl(),
+ exception);
+ }
+ }
+
+ Status CreateDirRecursive(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ auto container_client = blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ container_client.CreateIfNotExists();
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + " (" +
+ container_client.GetUrl() + ")",
+ exception);
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ // We can't create a directory without hierarchical namespace
+ // support. There is only "virtual directory" without
+ // hierarchical namespace support. And a "virtual directory" is
+ // (virtually) created a blob with ".../.../blob" blob name
+ // automatically.
+ return Status::OK();
+ }
+
+ std::string current_path;
+ for (const auto& part : path.path_to_file_parts) {
Review Comment:
PR: #38783
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on code in PR #38708:
URL: https://github.com/apache/arrow/pull/38708#discussion_r1397960992
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -451,10 +470,14 @@ TEST_F(AzureHierarchicalNamespaceFileSystemTest, CreateDirRecursiveSuccessDirect
arrow::fs::AssertFileInfo(fs_.get(), parent, FileType::Directory);
}
-TEST_F(AzuriteFileSystemTest, CreateDirRecursiveFailureDirectoryOnly) {
+TEST_F(AzuriteFileSystemTest, CreateDirRecursiveSuccessDirectoryOnly) {
const auto parent = PreexistingContainerPath() + RandomDirectoryName();
const auto path = internal::ConcatAbstractPath(parent, "new-sub");
- ASSERT_RAISES(NotImplemented, fs_->CreateDir(path, true));
+ ASSERT_OK(fs_->CreateDir(path, true));
+ // 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);
Review Comment:
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.
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #38708:
URL: https://github.com/apache/arrow/pull/38708#issuecomment-1817327397
I'll merge 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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #38708:
URL: https://github.com/apache/arrow/pull/38708#discussion_r1398331519
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -611,6 +621,109 @@ class AzureFileSystem::Impl {
RETURN_NOT_OK(ptr->Init());
return ptr;
}
+
+ Status CreateDir(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ if (path.path_to_file.empty()) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ auto response = container_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ return StatusFromErrorResponse(
+ container_client.GetUrl(), response.RawResponse.get(),
+ "Failed to create a container: " + path.container);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + ": " +
+ container_client.GetUrl(),
+ exception);
+ }
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ // We can't create a directory without hierarchical namespace
+ // support. There is only "virtual directory" without
+ // hierarchical namespace support. And a "virtual directory" is
+ // (virtually) created a blob with ".../.../blob" blob name
+ // automatically.
+ return Status::OK();
+ }
+
+ auto directory_client = datalake_service_client_->GetFileSystemClient(path.container)
+ .GetDirectoryClient(path.path_to_file);
+ try {
+ auto response = directory_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ return StatusFromErrorResponse(
+ directory_client.GetUrl(), response.RawResponse.get(),
+ "Failed to create a directory: " + path.path_to_file);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a directory: " + path.path_to_file + ": " +
+ directory_client.GetUrl(),
+ exception);
+ }
+ }
+
+ Status CreateDirRecursive(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ auto container_client = blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ container_client.CreateIfNotExists();
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + " (" +
+ container_client.GetUrl() + ")",
+ exception);
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ // We can't create a directory without hierarchical namespace
+ // support. There is only "virtual directory" without
+ // hierarchical namespace support. And a "virtual directory" is
+ // (virtually) created a blob with ".../.../blob" blob name
+ // automatically.
+ return Status::OK();
+ }
+
+ std::string current_path;
+ for (const auto& part : path.path_to_file_parts) {
Review Comment:
Oh, sorry.
I'll fix it: #38782
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #38708:
URL: https://github.com/apache/arrow/pull/38708#issuecomment-1815794548
I've updated.
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #38708:
URL: https://github.com/apache/arrow/pull/38708#issuecomment-1813648064
> > 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.
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on code in PR #38708:
URL: https://github.com/apache/arrow/pull/38708#discussion_r1396338372
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -611,6 +621,109 @@ class AzureFileSystem::Impl {
RETURN_NOT_OK(ptr->Init());
return ptr;
}
+
+ Status CreateDir(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ if (path.path_to_file.empty()) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ auto response = container_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ return StatusFromErrorResponse(
+ container_client.GetUrl(), response.RawResponse.get(),
+ "Failed to create a container: " + path.container);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + ": " +
+ container_client.GetUrl(),
+ exception);
+ }
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ // We can't create a directory without hierarchical namespace
+ // support. There is only "virtual directory" without
+ // hierarchical namespace support. And a "virtual directory" is
+ // (virtually) created a blob with ".../.../blob" blob name
+ // automatically.
+ return Status::OK();
+ }
+
+ auto directory_client = datalake_service_client_->GetFileSystemClient(path.container)
+ .GetDirectoryClient(path.path_to_file);
+ try {
+ auto response = directory_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ return StatusFromErrorResponse(
+ directory_client.GetUrl(), response.RawResponse.get(),
+ "Failed to create a directory: " + path.path_to_file);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a directory: " + path.path_to_file + ": " +
+ directory_client.GetUrl(),
+ exception);
+ }
+ }
+
+ Status CreateDirRecursive(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ auto container_client = blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ container_client.CreateIfNotExists();
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + " (" +
+ container_client.GetUrl() + ")",
+ exception);
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ // We can't create a directory without hierarchical namespace
+ // support. There is only "virtual directory" without
+ // hierarchical namespace support. And a "virtual directory" is
+ // (virtually) created a blob with ".../.../blob" blob name
+ // automatically.
+ return Status::OK();
+ }
+
+ std::string current_path;
+ for (const auto& part : path.path_to_file_parts) {
Review Comment:
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.
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #38708:
URL: https://github.com/apache/arrow/pull/38708#issuecomment-1817387630
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit a394a399ee0d0e25587b31b5c2d2abcdc0e67234.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/18807376935) has more details. It also includes information about 18 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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #38708:
URL: https://github.com/apache/arrow/pull/38708
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #38708:
URL: https://github.com/apache/arrow/pull/38708#discussion_r1396671402
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -611,6 +621,109 @@ class AzureFileSystem::Impl {
RETURN_NOT_OK(ptr->Init());
return ptr;
}
+
+ Status CreateDir(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ if (path.path_to_file.empty()) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ auto response = container_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ return StatusFromErrorResponse(
+ container_client.GetUrl(), response.RawResponse.get(),
+ "Failed to create a container: " + path.container);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + ": " +
+ container_client.GetUrl(),
+ exception);
+ }
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ // We can't create a directory without hierarchical namespace
+ // support. There is only "virtual directory" without
+ // hierarchical namespace support. And a "virtual directory" is
+ // (virtually) created a blob with ".../.../blob" blob name
+ // automatically.
+ return Status::OK();
+ }
+
+ auto directory_client = datalake_service_client_->GetFileSystemClient(path.container)
+ .GetDirectoryClient(path.path_to_file);
+ try {
+ auto response = directory_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ return StatusFromErrorResponse(
+ directory_client.GetUrl(), response.RawResponse.get(),
+ "Failed to create a directory: " + path.path_to_file);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a directory: " + path.path_to_file + ": " +
+ directory_client.GetUrl(),
+ exception);
+ }
+ }
+
+ Status CreateDirRecursive(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ auto container_client = blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ container_client.CreateIfNotExists();
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + " (" +
+ container_client.GetUrl() + ")",
+ exception);
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ // We can't create a directory without hierarchical namespace
+ // support. There is only "virtual directory" without
+ // hierarchical namespace support. And a "virtual directory" is
+ // (virtually) created a blob with ".../.../blob" blob name
+ // automatically.
+ return Status::OK();
+ }
+
+ std::string current_path;
+ for (const auto& part : path.path_to_file_parts) {
Review Comment:
Wow! Really!? I'll also check 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
Re: [PR] GH-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #38708:
URL: https://github.com/apache/arrow/pull/38708#discussion_r1395094715
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -611,6 +611,110 @@ class AzureFileSystem::Impl {
RETURN_NOT_OK(ptr->Init());
return ptr;
}
+
+ Status CreateDir(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ if (path.path_to_file.empty()) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ auto response = container_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ const auto& body = response.RawResponse->GetBody();
+ std::string_view body_text(reinterpret_cast<const char*>(body.data()),
+ body.size());
+ return Status::IOError("Failed to create a container: ", path.container, " (",
+ container_client.GetUrl(),
+ "): ", response.RawResponse->GetReasonPhrase(), " (",
+ static_cast<int>(response.RawResponse->GetStatusCode()),
+ "): ", body_text);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + " (" +
+ container_client.GetUrl() + ")",
+ exception);
+ }
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ return Status::NotImplemented(
+ "Cannot create a directory without hierarchical namespace: ", path.full_path);
+ }
+ auto directory_client = datalake_service_client_->GetFileSystemClient(path.container)
+ .GetDirectoryClient(path.path_to_file);
Review Comment:
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.)
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -611,6 +611,110 @@ class AzureFileSystem::Impl {
RETURN_NOT_OK(ptr->Init());
return ptr;
}
+
+ Status CreateDir(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ if (path.path_to_file.empty()) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ auto response = container_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ const auto& body = response.RawResponse->GetBody();
+ std::string_view body_text(reinterpret_cast<const char*>(body.data()),
+ body.size());
Review Comment:
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?
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -611,6 +611,110 @@ class AzureFileSystem::Impl {
RETURN_NOT_OK(ptr->Init());
return ptr;
}
+
+ Status CreateDir(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ if (path.path_to_file.empty()) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ auto response = container_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ const auto& body = response.RawResponse->GetBody();
+ std::string_view body_text(reinterpret_cast<const char*>(body.data()),
+ body.size());
+ return Status::IOError("Failed to create a container: ", path.container, " (",
+ container_client.GetUrl(),
+ "): ", response.RawResponse->GetReasonPhrase(), " (",
+ static_cast<int>(response.RawResponse->GetStatusCode()),
+ "): ", body_text);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + " (" +
+ container_client.GetUrl() + ")",
+ exception);
+ }
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ return Status::NotImplemented(
+ "Cannot create a directory without hierarchical namespace: ", path.full_path);
+ }
+ auto directory_client = datalake_service_client_->GetFileSystemClient(path.container)
+ .GetDirectoryClient(path.path_to_file);
+ try {
+ auto response = directory_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ const auto& body = response.RawResponse->GetBody();
+ std::string_view body_text(reinterpret_cast<const char*>(body.data()),
+ body.size());
+ return Status::IOError("Failed to create a directory: ", path.path_to_file, " (",
+ directory_client.GetUrl(),
+ "): ", response.RawResponse->GetReasonPhrase(), " (",
+ static_cast<int>(response.RawResponse->GetStatusCode()),
+ "): ", body_text);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a directory: " + path.path_to_file + " (" +
+ directory_client.GetUrl() + ")",
+ exception);
+ }
+ }
+
+ Status CreateDirRecursive(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ auto container_client = blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ container_client.CreateIfNotExists();
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + " (" +
+ container_client.GetUrl() + ")",
+ exception);
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ return Status::NotImplemented(
+ "Cannot create a directory without hierarchical namespace: ", path.full_path);
Review Comment:
Thanks! Wording suggestion is always welcome for me because I'm not a native English speaker!
I'll add "support".
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #38708:
URL: https://github.com/apache/arrow/pull/38708#issuecomment-1809706690
@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`?
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on PR #38708:
URL: https://github.com/apache/arrow/pull/38708#issuecomment-1809755366
> 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.
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #38708:
URL: https://github.com/apache/arrow/pull/38708#discussion_r1396670542
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -611,6 +611,110 @@ class AzureFileSystem::Impl {
RETURN_NOT_OK(ptr->Init());
return ptr;
}
+
+ Status CreateDir(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ if (path.path_to_file.empty()) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ auto response = container_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ const auto& body = response.RawResponse->GetBody();
+ std::string_view body_text(reinterpret_cast<const char*>(body.data()),
+ body.size());
+ return Status::IOError("Failed to create a container: ", path.container, " (",
+ container_client.GetUrl(),
+ "): ", response.RawResponse->GetReasonPhrase(), " (",
+ static_cast<int>(response.RawResponse->GetStatusCode()),
+ "): ", body_text);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + " (" +
+ container_client.GetUrl() + ")",
+ exception);
+ }
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ return Status::NotImplemented(
+ "Cannot create a directory without hierarchical namespace: ", path.full_path);
+ }
+ auto directory_client = datalake_service_client_->GetFileSystemClient(path.container)
+ .GetDirectoryClient(path.path_to_file);
Review Comment:
I've opened a new issue for this: #38758
I'll do it after we merge 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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on code in PR #38708:
URL: https://github.com/apache/arrow/pull/38708#discussion_r1396227884
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -611,6 +621,109 @@ class AzureFileSystem::Impl {
RETURN_NOT_OK(ptr->Init());
return ptr;
}
+
+ Status CreateDir(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ if (path.path_to_file.empty()) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ auto response = container_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ return StatusFromErrorResponse(
+ container_client.GetUrl(), response.RawResponse.get(),
+ "Failed to create a container: " + path.container);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + ": " +
+ container_client.GetUrl(),
+ exception);
+ }
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ // We can't create a directory without hierarchical namespace
+ // support. There is only "virtual directory" without
+ // hierarchical namespace support. And a "virtual directory" is
+ // (virtually) created a blob with ".../.../blob" blob name
+ // automatically.
Review Comment:
Personally I would reword this a bit. I wouldn't say that anything is created automatically. Blob storage clients just imply directories when they see `/`s in the blob names.
```suggestion
// Without hierarchical namespace enabled Azure blob storage has no directories.
// Therefore we can't, and don't need to create one. Simply creating a blob with `/` in the name
// implies directories.
```
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #38708:
URL: https://github.com/apache/arrow/pull/38708#discussion_r1397948435
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -451,10 +470,14 @@ TEST_F(AzureHierarchicalNamespaceFileSystemTest, CreateDirRecursiveSuccessDirect
arrow::fs::AssertFileInfo(fs_.get(), parent, FileType::Directory);
}
-TEST_F(AzuriteFileSystemTest, CreateDirRecursiveFailureDirectoryOnly) {
+TEST_F(AzuriteFileSystemTest, CreateDirRecursiveSuccessDirectoryOnly) {
const auto parent = PreexistingContainerPath() + RandomDirectoryName();
const auto path = internal::ConcatAbstractPath(parent, "new-sub");
- ASSERT_RAISES(NotImplemented, fs_->CreateDir(path, true));
+ ASSERT_OK(fs_->CreateDir(path, true));
+ // 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);
Review Comment:
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.
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on code in PR #38708:
URL: https://github.com/apache/arrow/pull/38708#discussion_r1397960992
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -451,10 +470,14 @@ TEST_F(AzureHierarchicalNamespaceFileSystemTest, CreateDirRecursiveSuccessDirect
arrow::fs::AssertFileInfo(fs_.get(), parent, FileType::Directory);
}
-TEST_F(AzuriteFileSystemTest, CreateDirRecursiveFailureDirectoryOnly) {
+TEST_F(AzuriteFileSystemTest, CreateDirRecursiveSuccessDirectoryOnly) {
const auto parent = PreexistingContainerPath() + RandomDirectoryName();
const auto path = internal::ConcatAbstractPath(parent, "new-sub");
- ASSERT_RAISES(NotImplemented, fs_->CreateDir(path, true));
+ ASSERT_OK(fs_->CreateDir(path, true));
+ // 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);
Review Comment:
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.
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #38708:
URL: https://github.com/apache/arrow/pull/38708#discussion_r1398039051
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -451,10 +470,14 @@ TEST_F(AzureHierarchicalNamespaceFileSystemTest, CreateDirRecursiveSuccessDirect
arrow::fs::AssertFileInfo(fs_.get(), parent, FileType::Directory);
}
-TEST_F(AzuriteFileSystemTest, CreateDirRecursiveFailureDirectoryOnly) {
+TEST_F(AzuriteFileSystemTest, CreateDirRecursiveSuccessDirectoryOnly) {
const auto parent = PreexistingContainerPath() + RandomDirectoryName();
const auto path = internal::ConcatAbstractPath(parent, "new-sub");
- ASSERT_RAISES(NotImplemented, fs_->CreateDir(path, true));
+ ASSERT_OK(fs_->CreateDir(path, true));
+ // 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);
Review Comment:
I agree with the opinion of Tom-Newton but let's discuss this further on #38772.
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on code in PR #38708:
URL: https://github.com/apache/arrow/pull/38708#discussion_r1398310217
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -611,6 +621,109 @@ class AzureFileSystem::Impl {
RETURN_NOT_OK(ptr->Init());
return ptr;
}
+
+ Status CreateDir(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ if (path.path_to_file.empty()) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ auto response = container_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ return StatusFromErrorResponse(
+ container_client.GetUrl(), response.RawResponse.get(),
+ "Failed to create a container: " + path.container);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + ": " +
+ container_client.GetUrl(),
+ exception);
+ }
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ // We can't create a directory without hierarchical namespace
+ // support. There is only "virtual directory" without
+ // hierarchical namespace support. And a "virtual directory" is
+ // (virtually) created a blob with ".../.../blob" blob name
+ // automatically.
+ return Status::OK();
+ }
+
+ auto directory_client = datalake_service_client_->GetFileSystemClient(path.container)
+ .GetDirectoryClient(path.path_to_file);
+ try {
+ auto response = directory_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ return StatusFromErrorResponse(
+ directory_client.GetUrl(), response.RawResponse.get(),
+ "Failed to create a directory: " + path.path_to_file);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a directory: " + path.path_to_file + ": " +
+ directory_client.GetUrl(),
+ exception);
+ }
+ }
+
+ Status CreateDirRecursive(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ auto container_client = blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ container_client.CreateIfNotExists();
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + " (" +
+ container_client.GetUrl() + ")",
+ exception);
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ // We can't create a directory without hierarchical namespace
+ // support. There is only "virtual directory" without
+ // hierarchical namespace support. And a "virtual directory" is
+ // (virtually) created a blob with ".../.../blob" blob name
+ // automatically.
+ return Status::OK();
+ }
+
+ std::string current_path;
+ for (const auto& part : path.path_to_file_parts) {
Review Comment:
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()`
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #38708:
URL: https://github.com/apache/arrow/pull/38708#discussion_r1393530485
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -611,6 +611,110 @@ class AzureFileSystem::Impl {
RETURN_NOT_OK(ptr->Init());
return ptr;
}
+
+ Status CreateDir(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ if (path.path_to_file.empty()) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ auto response = container_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ const auto& body = response.RawResponse->GetBody();
+ std::string_view body_text(reinterpret_cast<const char*>(body.data()),
+ body.size());
Review Comment:
Is it safe to put this in a string? We are sure this is plain text?
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #38708:
URL: https://github.com/apache/arrow/pull/38708#discussion_r1395811459
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -611,6 +611,110 @@ class AzureFileSystem::Impl {
RETURN_NOT_OK(ptr->Init());
return ptr;
}
+
+ Status CreateDir(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ if (path.path_to_file.empty()) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ auto response = container_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ const auto& body = response.RawResponse->GetBody();
+ std::string_view body_text(reinterpret_cast<const char*>(body.data()),
+ body.size());
+ return Status::IOError("Failed to create a container: ", path.container, " (",
+ container_client.GetUrl(),
+ "): ", response.RawResponse->GetReasonPhrase(), " (",
+ static_cast<int>(response.RawResponse->GetStatusCode()),
+ "): ", body_text);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + " (" +
+ container_client.GetUrl() + ")",
+ exception);
+ }
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ return Status::NotImplemented(
+ "Cannot create a directory without hierarchical namespace: ", path.full_path);
+ }
+ auto directory_client = datalake_service_client_->GetFileSystemClient(path.container)
+ .GetDirectoryClient(path.path_to_file);
+ try {
+ auto response = directory_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ const auto& body = response.RawResponse->GetBody();
+ std::string_view body_text(reinterpret_cast<const char*>(body.data()),
+ body.size());
+ return Status::IOError("Failed to create a directory: ", path.path_to_file, " (",
+ directory_client.GetUrl(),
+ "): ", response.RawResponse->GetReasonPhrase(), " (",
+ static_cast<int>(response.RawResponse->GetStatusCode()),
+ "): ", body_text);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a directory: " + path.path_to_file + " (" +
+ directory_client.GetUrl() + ")",
+ exception);
+ }
+ }
+
+ Status CreateDirRecursive(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ auto container_client = blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ container_client.CreateIfNotExists();
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + " (" +
+ container_client.GetUrl() + ")",
+ exception);
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ return Status::NotImplemented(
+ "Cannot create a directory without hierarchical namespace: ", path.full_path);
Review Comment:
> 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. :)
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #38708:
URL: https://github.com/apache/arrow/pull/38708#discussion_r1395800509
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -611,6 +611,110 @@ class AzureFileSystem::Impl {
RETURN_NOT_OK(ptr->Init());
return ptr;
}
+
+ Status CreateDir(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ if (path.path_to_file.empty()) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ auto response = container_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ const auto& body = response.RawResponse->GetBody();
+ std::string_view body_text(reinterpret_cast<const char*>(body.data()),
+ body.size());
Review Comment:
It's fine to keep as is.
--
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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #38708:
URL: https://github.com/apache/arrow/pull/38708#discussion_r1396680796
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -611,6 +621,109 @@ class AzureFileSystem::Impl {
RETURN_NOT_OK(ptr->Init());
return ptr;
}
+
+ Status CreateDir(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ if (path.path_to_file.empty()) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ auto response = container_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ return StatusFromErrorResponse(
+ container_client.GetUrl(), response.RawResponse.get(),
+ "Failed to create a container: " + path.container);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + ": " +
+ container_client.GetUrl(),
+ exception);
+ }
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ // We can't create a directory without hierarchical namespace
+ // support. There is only "virtual directory" without
+ // hierarchical namespace support. And a "virtual directory" is
+ // (virtually) created a blob with ".../.../blob" blob name
+ // automatically.
+ return Status::OK();
+ }
+
+ auto directory_client = datalake_service_client_->GetFileSystemClient(path.container)
+ .GetDirectoryClient(path.path_to_file);
+ try {
+ auto response = directory_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ return StatusFromErrorResponse(
+ directory_client.GetUrl(), response.RawResponse.get(),
+ "Failed to create a directory: " + path.path_to_file);
+ }
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a directory: " + path.path_to_file + ": " +
+ directory_client.GetUrl(),
+ exception);
+ }
+ }
+
+ Status CreateDirRecursive(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ auto container_client = blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ container_client.CreateIfNotExists();
+ } catch (const Azure::Storage::StorageException& exception) {
+ return internal::ExceptionToStatus(
+ "Failed to create a container: " + path.container + " (" +
+ container_client.GetUrl() + ")",
+ exception);
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto hierarchical_namespace_enabled,
+ hierarchical_namespace_.Enabled(path.container));
+ if (!hierarchical_namespace_enabled) {
+ // We can't create a directory without hierarchical namespace
+ // support. There is only "virtual directory" without
+ // hierarchical namespace support. And a "virtual directory" is
+ // (virtually) created a blob with ".../.../blob" blob name
+ // automatically.
+ return Status::OK();
+ }
+
+ std::string current_path;
+ for (const auto& part : path.path_to_file_parts) {
Review Comment:
I confirmed that we don't need this loop too.
I'll simplify 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-38699: [C++][FS][Azure] Implement `CreateDir()` [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #38708:
URL: https://github.com/apache/arrow/pull/38708#discussion_r1396670639
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -611,6 +611,110 @@ class AzureFileSystem::Impl {
RETURN_NOT_OK(ptr->Init());
return ptr;
}
+
+ Status CreateDir(const AzurePath& path) {
+ if (path.container.empty()) {
+ return Status::Invalid("Cannot create an empty container");
+ }
+
+ if (path.path_to_file.empty()) {
+ auto container_client =
+ blob_service_client_->GetBlobContainerClient(path.container);
+ try {
+ auto response = container_client.Create();
+ if (response.Value.Created) {
+ return Status::OK();
+ } else {
+ const auto& body = response.RawResponse->GetBody();
+ std::string_view body_text(reinterpret_cast<const char*>(body.data()),
+ body.size());
Review Comment:
OK. I'll add a comment.
--
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