kou commented on code in PR #40075:
URL: https://github.com/apache/arrow/pull/40075#discussion_r1493000478
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -2055,6 +2057,148 @@ class AzureFileSystem::Impl {
static constexpr auto kTimeNeededForFileOrDirectoryRename =
std::chrono::seconds{3};
public:
+ /// \pre location.container is not empty.
+ /// \pre location.path is not empty.
+ Status DeleteFileOnFileSystem(const DataLake::DataLakeFileSystemClient&
adlfs_client,
+ const AzureLocation& location,
+ bool require_file_to_exist) {
+ DCHECK(!location.container.empty());
+ DCHECK(!location.path.empty());
+ auto path_no_trailing_slash =
+ std::string{internal::RemoveTrailingSlash(location.path)};
+ auto file_client = adlfs_client.GetFileClient(path_no_trailing_slash);
+ try {
+ // This is necessary to avoid deletion of directories via DeleteFile.
+ auto properties = file_client.GetProperties();
+ if (properties.Value.IsDirectory) {
+ return internal::NotAFile(location.all);
+ }
+ if (internal::HasTrailingSlash(location.path)) {
+ return internal::NotADir(location.all);
+ }
+ auto response = file_client.Delete();
+ // Only the "*IfExists" functions ever set Deleted to false.
+ // All the others either succeed or throw an exception.
+ DCHECK(response.Value.Deleted);
+ } catch (const Storage::StorageException& exception) {
+ if (exception.StatusCode == Http::HttpStatusCode::NotFound) {
+ // ErrorCode can be "FilesystemNotFound", "PathNotFound"...
+ if (require_file_to_exist) {
+ return PathNotFound(location);
+ }
+ return Status::OK();
+ }
+ return ExceptionToStatus(exception, "Failed to delete a file: ",
location.path,
+ ": ", file_client.GetUrl());
+ }
+ return Status::OK();
+ }
+
+ /// \pre location.container is not empty.
+ /// \pre location.path is not empty.
+ Status DeleteFileOnContainer(const Blobs::BlobContainerClient&
container_client,
+ const AzureLocation& location, bool
require_file_to_exist,
+ const char* operation) {
+ DCHECK(!location.container.empty());
+ DCHECK(!location.path.empty());
+ constexpr auto kFileBlobLeaseTime = std::chrono::seconds{15};
+ auto no_trailing_slash_location = location.RemoveTrailingSlash(
+ /*preserve_original=*/true);
Review Comment:
It seems that `no_trailing_slash_location.all` isn't used by the user:
```cpp
ARROW_ASSIGN_OR_RAISE(auto file_info,
GetFileInfo(container_client,
no_trailing_slash_location));
```
`GetFileInfo()` in the above code may return an error to the user. But
`GetFileInfo()` doesn't use `no_trailing_slash_location.all` for an error
message:
https://github.com/apache/arrow/blob/a03d957b5b8d0425f9d5b6c98b6ee1efa56a1248/cpp/src/arrow/filesystem/azurefs.cc#L1275-L1277
It uses `file_client.GetUrl()` and it's based on
`no_trailing_slash_location.path` not `.all`.
And it seems that `FileInfo::path()` of `file_info` returned by
`GetFileInfo()` isn't used in this lambda. (`file_info.path()` uses
`no_trailing_slash_location.all` but it's not used.)
--
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]
