kou commented on code in PR #39361:
URL: https://github.com/apache/arrow/pull/39361#discussion_r1439034922
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -1033,47 +1042,88 @@ class AzureFileSystem::Impl {
return info;
} catch (const Storage::StorageException& exception) {
if (exception.StatusCode == Http::HttpStatusCode::NotFound) {
- ARROW_ASSIGN_OR_RAISE(auto hns_support,
- HierarchicalNamespaceSupport(adlfs_client));
- if (hns_support == HNSSupport::kContainerNotFound ||
- hns_support == HNSSupport::kEnabled) {
- // If the hierarchical namespace is enabled, then the storage
account will
- // have explicit directories. Neither a file nor a directory was
found.
- info.set_type(FileType::NotFound);
+ return FileInfo{location.all, FileType::NotFound};
+ }
+ return ExceptionToStatus(
+ exception, "GetProperties for '", file_client.GetUrl(),
+ "' failed. GetFileInfo is unable to determine whether the path
exists.");
+ }
+ }
+
+ /// On flat namespace accounts there are no real directories. Directories are
+ /// implied by empty directory marker blobs with names ending in "/" or there
+ /// being blobs with names starting with the directory path.
+ ///
+ /// \pre location.path is not empty.
+ Result<FileInfo> GetFileInfo(const Blobs::BlobContainerClient&
container_client,
+ const AzureLocation& location) {
+ DCHECK(!location.path.empty());
+ Blobs::ListBlobsOptions options;
+ options.Prefix = internal::RemoveTrailingSlash(location.path);
+ options.PageSizeHint = 1;
+
+ try {
+ FileInfo info{location.all};
+ auto list_response = container_client.ListBlobsByHierarchy(kDelimiter,
options);
+ // Since PageSizeHint=1, we expect at most one entry in either Blobs or
+ // BlobPrefixes. A BlobPrefix always ends with kDelimiter ("/"), so we
can
+ // distinguish between a directory and a file by checking if we received
a
+ // prefix or a blob.
+ if (!list_response.BlobPrefixes.empty()) {
+ // Ensure the returned BlobPrefixes[0] string doesn't contain more
characters than
+ // the requested Prefix. For instance, if we request with
Prefix="dir/abra" and
+ // the container contains "dir/abracadabra/" but not "dir/abra/", we
will get back
+ // "dir/abracadabra/" in the BlobPrefixes list. If "dir/abra/" existed,
+ // it would be returned instead because it comes before
"dir/abracadabra/" in the
+ // lexicographic order guaranteed by ListBlobsByHierarchy.
+ const auto& blob_prefix = list_response.BlobPrefixes[0];
+ if (blob_prefix == internal::EnsureTrailingSlash(location.path)) {
+ info.set_type(FileType::Directory);
return info;
}
- // On flat namespace accounts there are no real directories.
Directories are only
- // implied by using `/` in the blob name.
- Blobs::ListBlobsOptions list_blob_options;
- // If listing the prefix `path.path_to_file` with trailing slash
returns at least
- // one result then `path` refers to an implied directory.
- list_blob_options.Prefix =
internal::EnsureTrailingSlash(location.path);
- // We only need to know if there is at least one result, so minimise
page size
- // for efficiency.
- list_blob_options.PageSizeHint = 1;
-
- try {
- auto paged_list_result =
- blob_service_client_->GetBlobContainerClient(location.container)
- .ListBlobs(list_blob_options);
- auto file_type = paged_list_result.Blobs.size() > 0 ?
FileType::Directory
- :
FileType::NotFound;
- info.set_type(file_type);
+ }
+ if (!list_response.Blobs.empty()) {
+ const auto& blob = list_response.Blobs[0];
+ if (blob.Name == location.path) {
Review Comment:
Do we need this check?
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -1404,11 +1438,76 @@ class AzureFileSystem::Impl {
return stream;
}
- private:
- Status DeleteDirContentsWithoutHierarchicalNamespace(const AzureLocation&
location,
- bool missing_dir_ok) {
- auto container_client =
- blob_service_client_->GetBlobContainerClient(location.container);
+ /// This function assumes the container already exists. So it can only be
+ /// called after that has been verified.
+ ///
+ /// \pre location.container is not empty.
+ /// \pre The location.container container already exists.
+ Status EnsureEmptyDirExists(const Blobs::BlobContainerClient&
container_client,
+ const AzureLocation& location) {
+ DCHECK(!location.container.empty());
+ if (location.path.empty()) {
+ // Nothing to do. The container already exists per the preconditions.
+ return Status::OK();
+ }
+ auto dir_marker_blob_path = internal::EnsureTrailingSlash(location.path);
+ auto block_blob_client =
+
container_client.GetBlobClient(dir_marker_blob_path).AsBlockBlobClient();
+ try {
+ block_blob_client.UploadFrom(nullptr, 0);
+ return Status::OK();
+ } catch (const Storage::StorageException& exception) {
+ return ExceptionToStatus(exception);
+ }
+ }
+
+ /// \pre location.container is not empty.
+ /// \pre location.path is empty.
+ Status DeleteContainer(const Blobs::BlobContainerClient& container_client,
+ const AzureLocation& location) {
+ DCHECK(!location.container.empty());
+ DCHECK(location.path.empty());
+ try {
+ auto response = container_client.Delete();
+ if (response.Value.Deleted) {
+ return Status::OK();
+ } else {
+ return StatusFromErrorResponse(
+ container_client.GetUrl(), *response.RawResponse,
+ "Failed to delete a container: " + location.container);
+ }
+ } catch (const Storage::StorageException& exception) {
+ if (IsContainerNotFound(exception)) {
+ return PathNotFound(location);
+ }
+ return ExceptionToStatus(exception,
+ "Failed to delete a container: ",
location.container, ": ",
+ container_client.GetUrl());
+ }
+ }
+
+ /// Deletes and contents of a directory and possibly the directory itself
Review Comment:
```suggestion
/// Deletes contents of a directory and possibly the directory itself
```
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -1288,97 +1338,81 @@ class AzureFileSystem::Impl {
return ptr;
}
- Status CreateDir(const AzureLocation& location) {
- if (location.container.empty()) {
- return Status::Invalid("CreateDir requires a non-empty path.");
- }
-
- auto container_client =
- blob_service_client_->GetBlobContainerClient(location.container);
- if (location.path.empty()) {
- try {
- auto response = container_client.Create();
- return response.Value.Created
- ? Status::OK()
- : Status::AlreadyExists("Directory already exists: " +
location.all);
- } catch (const Storage::StorageException& exception) {
- return ExceptionToStatus(exception,
- "Failed to create a container: ",
location.container,
- ": ", container_client.GetUrl());
- }
- }
-
- auto adlfs_client =
datalake_service_client_->GetFileSystemClient(location.container);
- ARROW_ASSIGN_OR_RAISE(auto hns_support,
HierarchicalNamespaceSupport(adlfs_client));
- if (hns_support == HNSSupport::kContainerNotFound) {
- return PathNotFound(location);
- }
- if (hns_support == HNSSupport::kDisabled) {
- ARROW_ASSIGN_OR_RAISE(
- auto container_info,
- GetContainerPropsAsFileInfo(location.container, container_client));
- if (container_info.type() == FileType::NotFound) {
- return PathNotFound(location);
- }
- // 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.
- return Status::OK();
- }
-
- auto directory_client = adlfs_client.GetDirectoryClient(location.path);
- try {
- auto response = directory_client.Create();
- if (response.Value.Created) {
- return Status::OK();
- } else {
- return StatusFromErrorResponse(directory_client.GetUrl(),
*response.RawResponse,
- "Failed to create a directory: " +
location.path);
+ private:
+ /// This function cannot assume the filesystem/container already exists.
+ ///
+ /// \pre location.container is not empty.
+ /// \pre location.path is not empty.
+ template <class ContainerClient, class GetDirectoryClient, class
CreateDirIfNotExists>
+ Status CreateDirTemplate(const ContainerClient& container_client,
+ GetDirectoryClient&& get_directory_client,
+ CreateDirIfNotExists&& create_if_not_exists,
+ const AzureLocation& location, bool recursive) {
+ DCHECK(!location.container.empty());
+ DCHECK(!location.path.empty());
+ // Non-recursive CreateDir calls require the parent directory to exist.
+ if (!recursive) {
+ auto parent = location.parent();
+ if (!parent.path.empty()) {
+ RETURN_NOT_OK(CheckDirExists(container_client, parent));
}
- } catch (const Storage::StorageException& exception) {
- return ExceptionToStatus(exception, "Failed to create a directory: ",
location.path,
- ": ", directory_client.GetUrl());
- }
- }
-
- Status CreateDirRecursive(const AzureLocation& location) {
- if (location.container.empty()) {
- return Status::Invalid("CreateDir requires a non-empty path.");
+ // If the parent location is just the container, we don't need to check
if it
+ // exists because the operation we perform below will fail if the
container
+ // doesn't exist and we can handle that error according to the recursive
flag.
}
-
- auto container_client =
- blob_service_client_->GetBlobContainerClient(location.container);
+ auto directory_client = get_directory_client(container_client, location);
try {
- container_client.CreateIfNotExists();
- } catch (const Storage::StorageException& exception) {
- return ExceptionToStatus(exception,
- "Failed to create a container: ",
location.container, " (",
- container_client.GetUrl(), ")");
- }
-
- auto adlfs_client =
datalake_service_client_->GetFileSystemClient(location.container);
- ARROW_ASSIGN_OR_RAISE(auto hns_support,
HierarchicalNamespaceSupport(adlfs_client));
- if (hns_support == HNSSupport::kDisabled) {
- // 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.
+ create_if_not_exists(directory_client);
return Status::OK();
- }
- // Don't handle HNSSupport::kContainerNotFound, just assume it still
exists (because
- // it was created above) and try to create the directory.
-
- if (!location.path.empty()) {
- auto directory_client = adlfs_client.GetDirectoryClient(location.path);
- try {
- directory_client.CreateIfNotExists();
- } catch (const Storage::StorageException& exception) {
- return ExceptionToStatus(exception,
- "Failed to create a directory: ",
location.path, " (",
- directory_client.GetUrl(), ")");
+ } catch (const Storage::StorageException& exception) {
+ if (IsContainerNotFound(exception)) {
+ try {
+ if (recursive) {
+ container_client.CreateIfNotExists();
+ create_if_not_exists(directory_client);
+ return Status::OK();
+ } else {
+ auto parent = location.parent();
+ return PathNotFound(parent);
+ }
+ } catch (const Storage::StorageException& second_exception) {
+ return ExceptionToStatus(second_exception);
+ }
}
+ return ExceptionToStatus(exception);
Review Comment:
ditto.
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -1288,97 +1338,81 @@ class AzureFileSystem::Impl {
return ptr;
}
- Status CreateDir(const AzureLocation& location) {
- if (location.container.empty()) {
- return Status::Invalid("CreateDir requires a non-empty path.");
- }
-
- auto container_client =
- blob_service_client_->GetBlobContainerClient(location.container);
- if (location.path.empty()) {
- try {
- auto response = container_client.Create();
- return response.Value.Created
- ? Status::OK()
- : Status::AlreadyExists("Directory already exists: " +
location.all);
- } catch (const Storage::StorageException& exception) {
- return ExceptionToStatus(exception,
- "Failed to create a container: ",
location.container,
- ": ", container_client.GetUrl());
- }
- }
-
- auto adlfs_client =
datalake_service_client_->GetFileSystemClient(location.container);
- ARROW_ASSIGN_OR_RAISE(auto hns_support,
HierarchicalNamespaceSupport(adlfs_client));
- if (hns_support == HNSSupport::kContainerNotFound) {
- return PathNotFound(location);
- }
- if (hns_support == HNSSupport::kDisabled) {
- ARROW_ASSIGN_OR_RAISE(
- auto container_info,
- GetContainerPropsAsFileInfo(location.container, container_client));
- if (container_info.type() == FileType::NotFound) {
- return PathNotFound(location);
- }
- // 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.
- return Status::OK();
- }
-
- auto directory_client = adlfs_client.GetDirectoryClient(location.path);
- try {
- auto response = directory_client.Create();
- if (response.Value.Created) {
- return Status::OK();
- } else {
- return StatusFromErrorResponse(directory_client.GetUrl(),
*response.RawResponse,
- "Failed to create a directory: " +
location.path);
+ private:
+ /// This function cannot assume the filesystem/container already exists.
+ ///
+ /// \pre location.container is not empty.
+ /// \pre location.path is not empty.
+ template <class ContainerClient, class GetDirectoryClient, class
CreateDirIfNotExists>
+ Status CreateDirTemplate(const ContainerClient& container_client,
+ GetDirectoryClient&& get_directory_client,
+ CreateDirIfNotExists&& create_if_not_exists,
+ const AzureLocation& location, bool recursive) {
+ DCHECK(!location.container.empty());
+ DCHECK(!location.path.empty());
+ // Non-recursive CreateDir calls require the parent directory to exist.
+ if (!recursive) {
+ auto parent = location.parent();
+ if (!parent.path.empty()) {
+ RETURN_NOT_OK(CheckDirExists(container_client, parent));
}
- } catch (const Storage::StorageException& exception) {
- return ExceptionToStatus(exception, "Failed to create a directory: ",
location.path,
- ": ", directory_client.GetUrl());
- }
- }
-
- Status CreateDirRecursive(const AzureLocation& location) {
- if (location.container.empty()) {
- return Status::Invalid("CreateDir requires a non-empty path.");
+ // If the parent location is just the container, we don't need to check
if it
+ // exists because the operation we perform below will fail if the
container
+ // doesn't exist and we can handle that error according to the recursive
flag.
}
-
- auto container_client =
- blob_service_client_->GetBlobContainerClient(location.container);
+ auto directory_client = get_directory_client(container_client, location);
try {
- container_client.CreateIfNotExists();
- } catch (const Storage::StorageException& exception) {
- return ExceptionToStatus(exception,
- "Failed to create a container: ",
location.container, " (",
- container_client.GetUrl(), ")");
- }
-
- auto adlfs_client =
datalake_service_client_->GetFileSystemClient(location.container);
- ARROW_ASSIGN_OR_RAISE(auto hns_support,
HierarchicalNamespaceSupport(adlfs_client));
- if (hns_support == HNSSupport::kDisabled) {
- // 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.
+ create_if_not_exists(directory_client);
return Status::OK();
- }
- // Don't handle HNSSupport::kContainerNotFound, just assume it still
exists (because
- // it was created above) and try to create the directory.
-
- if (!location.path.empty()) {
- auto directory_client = adlfs_client.GetDirectoryClient(location.path);
- try {
- directory_client.CreateIfNotExists();
- } catch (const Storage::StorageException& exception) {
- return ExceptionToStatus(exception,
- "Failed to create a directory: ",
location.path, " (",
- directory_client.GetUrl(), ")");
+ } catch (const Storage::StorageException& exception) {
+ if (IsContainerNotFound(exception)) {
+ try {
+ if (recursive) {
+ container_client.CreateIfNotExists();
+ create_if_not_exists(directory_client);
+ return Status::OK();
+ } else {
+ auto parent = location.parent();
+ return PathNotFound(parent);
+ }
+ } catch (const Storage::StorageException& second_exception) {
+ return ExceptionToStatus(second_exception);
+ }
}
+ return ExceptionToStatus(exception);
}
+ }
- return Status::OK();
+ public:
+ /// This function cannot assume the filesystem already exists.
+ ///
+ /// \pre location.container is not empty.
+ /// \pre location.path is not empty.
+ Status CreateDirOnFileSystem(const DataLake::DataLakeFileSystemClient&
adlfs_client,
+ const AzureLocation& location, bool recursive) {
+ return CreateDirTemplate(
+ adlfs_client,
+ [](auto& adlfs_client, auto& location) {
+ return adlfs_client.GetDirectoryClient(location.path);
+ },
+ [](auto& directory_client) { directory_client.CreateIfNotExists(); },
location,
+ recursive);
+ }
+
+ /// This function cannot assume the container already exists.
+ ///
+ /// \pre location.container is not empty.
+ /// \pre location.path is not empty.
+ Status CreateDirOnContainer(const Blobs::BlobContainerClient&
container_client,
+ const AzureLocation& location, bool recursive) {
+ return CreateDirTemplate(
+ container_client,
+ [](auto& container_client, auto& location) {
+ auto dir_marker_blob_path =
internal::EnsureTrailingSlash(location.path);
+ return
container_client.GetBlobClient(dir_marker_blob_path).AsBlockBlobClient();
+ },
+ [](auto& block_blob_client) { block_blob_client.UploadFrom(nullptr,
0); },
Review Comment:
Can we use `EnsureEmptyDirExists()` here?
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -1288,97 +1338,81 @@ class AzureFileSystem::Impl {
return ptr;
}
- Status CreateDir(const AzureLocation& location) {
- if (location.container.empty()) {
- return Status::Invalid("CreateDir requires a non-empty path.");
- }
-
- auto container_client =
- blob_service_client_->GetBlobContainerClient(location.container);
- if (location.path.empty()) {
- try {
- auto response = container_client.Create();
- return response.Value.Created
- ? Status::OK()
- : Status::AlreadyExists("Directory already exists: " +
location.all);
- } catch (const Storage::StorageException& exception) {
- return ExceptionToStatus(exception,
- "Failed to create a container: ",
location.container,
- ": ", container_client.GetUrl());
- }
- }
-
- auto adlfs_client =
datalake_service_client_->GetFileSystemClient(location.container);
- ARROW_ASSIGN_OR_RAISE(auto hns_support,
HierarchicalNamespaceSupport(adlfs_client));
- if (hns_support == HNSSupport::kContainerNotFound) {
- return PathNotFound(location);
- }
- if (hns_support == HNSSupport::kDisabled) {
- ARROW_ASSIGN_OR_RAISE(
- auto container_info,
- GetContainerPropsAsFileInfo(location.container, container_client));
- if (container_info.type() == FileType::NotFound) {
- return PathNotFound(location);
- }
- // 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.
- return Status::OK();
- }
-
- auto directory_client = adlfs_client.GetDirectoryClient(location.path);
- try {
- auto response = directory_client.Create();
- if (response.Value.Created) {
- return Status::OK();
- } else {
- return StatusFromErrorResponse(directory_client.GetUrl(),
*response.RawResponse,
- "Failed to create a directory: " +
location.path);
+ private:
+ /// This function cannot assume the filesystem/container already exists.
+ ///
+ /// \pre location.container is not empty.
+ /// \pre location.path is not empty.
+ template <class ContainerClient, class GetDirectoryClient, class
CreateDirIfNotExists>
+ Status CreateDirTemplate(const ContainerClient& container_client,
+ GetDirectoryClient&& get_directory_client,
+ CreateDirIfNotExists&& create_if_not_exists,
+ const AzureLocation& location, bool recursive) {
+ DCHECK(!location.container.empty());
+ DCHECK(!location.path.empty());
+ // Non-recursive CreateDir calls require the parent directory to exist.
+ if (!recursive) {
+ auto parent = location.parent();
+ if (!parent.path.empty()) {
+ RETURN_NOT_OK(CheckDirExists(container_client, parent));
}
- } catch (const Storage::StorageException& exception) {
- return ExceptionToStatus(exception, "Failed to create a directory: ",
location.path,
- ": ", directory_client.GetUrl());
- }
- }
-
- Status CreateDirRecursive(const AzureLocation& location) {
- if (location.container.empty()) {
- return Status::Invalid("CreateDir requires a non-empty path.");
+ // If the parent location is just the container, we don't need to check
if it
+ // exists because the operation we perform below will fail if the
container
+ // doesn't exist and we can handle that error according to the recursive
flag.
}
-
- auto container_client =
- blob_service_client_->GetBlobContainerClient(location.container);
+ auto directory_client = get_directory_client(container_client, location);
try {
- container_client.CreateIfNotExists();
- } catch (const Storage::StorageException& exception) {
- return ExceptionToStatus(exception,
- "Failed to create a container: ",
location.container, " (",
- container_client.GetUrl(), ")");
- }
-
- auto adlfs_client =
datalake_service_client_->GetFileSystemClient(location.container);
- ARROW_ASSIGN_OR_RAISE(auto hns_support,
HierarchicalNamespaceSupport(adlfs_client));
- if (hns_support == HNSSupport::kDisabled) {
- // 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.
+ create_if_not_exists(directory_client);
return Status::OK();
- }
- // Don't handle HNSSupport::kContainerNotFound, just assume it still
exists (because
- // it was created above) and try to create the directory.
-
- if (!location.path.empty()) {
- auto directory_client = adlfs_client.GetDirectoryClient(location.path);
- try {
- directory_client.CreateIfNotExists();
- } catch (const Storage::StorageException& exception) {
- return ExceptionToStatus(exception,
- "Failed to create a directory: ",
location.path, " (",
- directory_client.GetUrl(), ")");
+ } catch (const Storage::StorageException& exception) {
+ if (IsContainerNotFound(exception)) {
+ try {
+ if (recursive) {
+ container_client.CreateIfNotExists();
+ create_if_not_exists(directory_client);
+ return Status::OK();
+ } else {
+ auto parent = location.parent();
+ return PathNotFound(parent);
+ }
+ } catch (const Storage::StorageException& second_exception) {
+ return ExceptionToStatus(second_exception);
Review Comment:
Could you add additional error information like other `ExceptionToStatus()`
calls?
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -1288,97 +1338,81 @@ class AzureFileSystem::Impl {
return ptr;
}
- Status CreateDir(const AzureLocation& location) {
- if (location.container.empty()) {
- return Status::Invalid("CreateDir requires a non-empty path.");
- }
-
- auto container_client =
- blob_service_client_->GetBlobContainerClient(location.container);
- if (location.path.empty()) {
- try {
- auto response = container_client.Create();
- return response.Value.Created
- ? Status::OK()
- : Status::AlreadyExists("Directory already exists: " +
location.all);
- } catch (const Storage::StorageException& exception) {
- return ExceptionToStatus(exception,
- "Failed to create a container: ",
location.container,
- ": ", container_client.GetUrl());
- }
- }
-
- auto adlfs_client =
datalake_service_client_->GetFileSystemClient(location.container);
- ARROW_ASSIGN_OR_RAISE(auto hns_support,
HierarchicalNamespaceSupport(adlfs_client));
- if (hns_support == HNSSupport::kContainerNotFound) {
- return PathNotFound(location);
- }
- if (hns_support == HNSSupport::kDisabled) {
- ARROW_ASSIGN_OR_RAISE(
- auto container_info,
- GetContainerPropsAsFileInfo(location.container, container_client));
- if (container_info.type() == FileType::NotFound) {
- return PathNotFound(location);
- }
- // 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.
- return Status::OK();
- }
-
- auto directory_client = adlfs_client.GetDirectoryClient(location.path);
- try {
- auto response = directory_client.Create();
- if (response.Value.Created) {
- return Status::OK();
- } else {
- return StatusFromErrorResponse(directory_client.GetUrl(),
*response.RawResponse,
- "Failed to create a directory: " +
location.path);
+ private:
+ /// This function cannot assume the filesystem/container already exists.
+ ///
+ /// \pre location.container is not empty.
+ /// \pre location.path is not empty.
+ template <class ContainerClient, class GetDirectoryClient, class
CreateDirIfNotExists>
+ Status CreateDirTemplate(const ContainerClient& container_client,
+ GetDirectoryClient&& get_directory_client,
+ CreateDirIfNotExists&& create_if_not_exists,
Review Comment:
Do we need to split them?
It seems that we need only `create_if_not_exists`. (`create_if_not_exists`
can just create a needed directory client and use it internally.)
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -1645,21 +1748,93 @@ Result<FileInfoVector>
AzureFileSystem::GetFileInfo(const FileSelector& select)
Status AzureFileSystem::CreateDir(const std::string& path, bool recursive) {
ARROW_ASSIGN_OR_RAISE(auto location, AzureLocation::FromString(path));
- if (recursive) {
- return impl_->CreateDirRecursive(location);
- } else {
- return impl_->CreateDir(location);
+ if (location.container.empty()) {
+ return Status::Invalid("CreateDir requires a non-empty path.");
}
+
+ auto container_client = impl_->GetBlobContainerClient(location.container);
+ if (location.path.empty()) {
+ // If the path is just the container, the parent (root) trivially exists,
+ // and the CreateDir operation comes down to just creating the container.
+ return CreateContainerIfNotExists(location.container, container_client);
+ }
+
+ auto adlfs_client = impl_->GetFileSystemClient(location.container);
+ ARROW_ASSIGN_OR_RAISE(auto hns_support,
+ impl_->HierarchicalNamespaceSupport(adlfs_client));
+ if (hns_support == HNSSupport::kContainerNotFound) {
+ if (!recursive) {
+ auto parent = location.parent();
+ return PathNotFound(parent);
+ }
+ RETURN_NOT_OK(CreateContainerIfNotExists(location.container,
container_client));
+ // Perform a second check for HNS support after creating the container.
+ ARROW_ASSIGN_OR_RAISE(hns_support,
impl_->HierarchicalNamespaceSupport(adlfs_client));
+ }
+ if (hns_support == HNSSupport::kContainerNotFound) {
+ // We only get kContainerNotFound if we are unable to read the properties
of the
+ // container we just created. This is very unlikely, but theoretically
possible in
+ // a concurrent system, so the error is handled to avoid infinite
recursion.
+ return Status::IOError("Unable to read properties of a newly created
container: ",
+ location.container, ": " +
container_client.GetUrl());
+ }
Review Comment:
How about moving this block to the first `if (hns_support ==
HNSSupport::kContainerNotFound) {` block?
```suggestion
if (hns_support == HNSSupport::kContainerNotFound) {
// We only get kContainerNotFound if we are unable to read the
properties of the
// container we just created. This is very unlikely, but theoretically
possible in
// a concurrent system, so the error is handled to avoid infinite
recursion.
return Status::IOError("Unable to read properties of a newly created
container: ",
location.container, ": " +
container_client.GetUrl());
}
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
