You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "felipecrv (via GitHub)" <gi...@apache.org> on 2024/02/14 01:32:14 UTC
[PR] GH-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
felipecrv opened a new pull request, #40075:
URL: https://github.com/apache/arrow/pull/40075
### Rationale for this change
It was not implemented yet.
### What changes are included in this PR?
- An implementation of `DeleteFile()` that is specialized to storage accounts that don't have HNS support enabled
- This fixes a semantic issue: deleting a file should not delete the parent directory when the file deleted was the last one
- Increased test coverage
- Fix of a bug in the version that deletes files in HNS-enabled accounts (we shouldn't let `DeleteFile` delete directories even if they are empty)
### Are these changes tested?
Yes. Tests were re-written and moved to `TestAzureFileSystemOnAllScenarios`.
--
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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #40075:
URL: https://github.com/apache/arrow/pull/40075#issuecomment-1942958064
@Tom-Newton
--
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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #40075:
URL: https://github.com/apache/arrow/pull/40075#discussion_r1496566444
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -2055,6 +2039,132 @@ 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) {
Review Comment:
It seems that we don't need to receive `operation` as an argument. How about defining it as a local variable instead of an argument?
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -2055,6 +2039,132 @@ 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};
+
+ // When it's known that the blob doesn't exist as a file, check if it exists as a
+ // directory to generate the appropriate error message.
+ auto check_if_location_exists_as_dir = [&]() -> Status {
+ auto no_trailing_slash = location;
+ no_trailing_slash.path = internal::RemoveTrailingSlash(location.path);
+ no_trailing_slash.all = internal::RemoveTrailingSlash(location.all);
+ ARROW_ASSIGN_OR_RAISE(auto file_info,
+ GetFileInfo(container_client, no_trailing_slash));
+ if (file_info.type() == FileType::NotFound) {
+ return require_file_to_exist ? PathNotFound(location) : Status::OK();
+ }
+ if (file_info.type() == FileType::Directory) {
+ return internal::NotAFile(location.all);
+ }
+ return internal::HasTrailingSlash(location.path) ? internal::NotADir(location.all)
+ : internal::NotAFile(location.all);
+ };
+
+ // Paths ending with trailing slashes are never leading to a deletion,
+ // but the correct error message requires a check of the path.
+ if (internal::HasTrailingSlash(location.path)) {
+ return check_if_location_exists_as_dir();
+ }
+
+ // If the parent directory of a file is not the container itself, there is a
+ // risk that deleting the file also deletes the *implied directory* -- the
+ // directory that is implied by the existence of the file path.
+ //
+ // In this case, we must ensure that the deletion is not semantically
+ // equivalent to also deleting the directory. This is done by ensuring the
+ // directory marker blob exists before the file is deleted.
+ std::optional<LeaseGuard> file_blob_lease_guard;
+ const auto parent = location.parent();
+ if (!parent.path.empty()) {
+ // We have to check the existence of the file before checking the
+ // existence of the parent directory marker, so we acquire a lease on the
+ // file first.
+ ARROW_ASSIGN_OR_RAISE(auto file_blob_lease_client,
+ AcquireBlobLease(location, kFileBlobLeaseTime,
+ /*allow_missing=*/true));
+ if (file_blob_lease_client) {
+ file_blob_lease_guard.emplace(std::move(file_blob_lease_client),
+ kFileBlobLeaseTime);
+ // Ensure the empty directory marker blob of the parent exists before the file is
+ // deleted.
+ //
+ // There is not need to hold a lease on the directory marker because if
+ // a concurrent client deletes the directory marker right after we
+ // create it, the file deletion itself won't be the cause of the directory
+ // deletion. Additionally, the fact that a lease is held on the blob path
+ // semantically preserves the directory -- its existence is implied
+ // until the blob representing the file is deleted -- even if another
+ // client deletes the directory marker.
+ RETURN_NOT_OK(EnsureEmptyDirExists(container_client, parent, operation));
+ } else {
+ return check_if_location_exists_as_dir();
+ }
+ }
+
+ DCHECK(!internal::HasTrailingSlash(location.path));
Review Comment:
Can we remove this because we have `if (internal::HasTrailingSlash(location.path)) {` above?
--
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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #40075:
URL: https://github.com/apache/arrow/pull/40075#discussion_r1491394837
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -2544,7 +2635,28 @@ Status AzureFileSystem::DeleteRootDirContents() {
Status AzureFileSystem::DeleteFile(const std::string& path) {
ARROW_ASSIGN_OR_RAISE(auto location, AzureLocation::FromString(path));
- return impl_->DeleteFile(location);
+ if (location.container.empty()) {
+ return Status::Invalid("DeleteFile requires a non-empty path.");
+ }
+ if (internal::HasTrailingSlash(location.path) || location.path.empty()) {
+ // Container paths (locations w/o path) or paths ending with a slash are not file
+ // paths.
+ return NotAFile(location);
+ }
Review Comment:
It's not the same logic I'm implementing here and it obscures the pre-conditions I'm guaranteeing before delegating to `DeleteFileOnFileSystem/DeleteFileOnContainer`.
--
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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #40075:
URL: https://github.com/apache/arrow/pull/40075#discussion_r1491989846
##########
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:
Hmm. I think that `preserve_original` isn't a good approach. It will confuse us. Is it really needed?
--
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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #40075:
URL: https://github.com/apache/arrow/pull/40075#discussion_r1491402273
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -889,6 +889,117 @@ class TestAzureFileSystem : public ::testing::Test {
ASSERT_RAISES(IOError, fs()->DeleteDirContents(directory_path, false));
}
+ void TestDeleteFileAtRoot() {
+ ASSERT_RAISES(Invalid, fs()->DeleteFile("file0"));
+ ASSERT_RAISES(Invalid, fs()->DeleteFile("file1/"));
+ const auto container_name = PreexistingData::RandomContainerName(rng_);
+ if (WithHierarchicalNamespace()) {
+ ARROW_UNUSED(CreateFilesystem(container_name));
+ } else {
+ ARROW_UNUSED(CreateContainer(container_name));
+ }
+ arrow::fs::AssertFileInfo(fs(), container_name, FileType::Directory);
+ ASSERT_RAISES(IOError, fs()->DeleteFile(container_name));
+ ASSERT_RAISES(IOError, fs()->DeleteFile(container_name + "/"));
+ }
+
+ void TestDeleteFileAtContainerRoot() {
+ auto data = SetUpPreexistingData();
+
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Path does not exist '" + data.Path("nonexistent-file") +
+ "'"),
+ fs()->DeleteFile(data.Path("nonexistent-file")));
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Not a regular file: '" + data.Path("nonexistent-file/") +
+ "'"),
+ fs()->DeleteFile(data.Path("nonexistent-file/")));
+
+ arrow::fs::AssertFileInfo(fs(), data.ObjectPath(), FileType::File);
+ ASSERT_OK(fs()->DeleteFile(data.ObjectPath()));
+ arrow::fs::AssertFileInfo(fs(), data.ObjectPath(), FileType::NotFound);
+
+ if (WithHierarchicalNamespace()) {
+ auto adlfs_client =
+ datalake_service_client_->GetFileSystemClient(data.container_name);
+ CreateFile(adlfs_client, data.kObjectName, PreexistingData::kLoremIpsum);
+ } else {
+ auto container_client = CreateContainer(data.container_name);
+ CreateBlob(container_client, data.kObjectName, PreexistingData::kLoremIpsum);
+ }
+
+ arrow::fs::AssertFileInfo(fs(), data.ObjectPath(), FileType::File);
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError, ::testing::HasSubstr("Not a regular file: '" + data.ObjectPath() + "/'"),
+ fs()->DeleteFile(data.ObjectPath() + "/"));
+ ASSERT_RAISES(IOError, fs()->DeleteFile(data.ObjectPath() + "/"));
+ }
+
+ void TestDeleteFileAtSubdirectory(bool create_empty_dir_marker_first) {
+ auto data = SetUpPreexistingData();
+
+ auto setup_dir_file0 = [this, create_empty_dir_marker_first, &data]() {
+ if (WithHierarchicalNamespace()) {
+ ASSERT_FALSE(create_empty_dir_marker_first);
+ auto adlfs_client =
+ datalake_service_client_->GetFileSystemClient(data.container_name);
+ CreateFile(adlfs_client, "dir/file0", PreexistingData::kLoremIpsum);
+ } else {
+ auto container_client = CreateContainer(data.container_name);
+ if (create_empty_dir_marker_first) {
+ CreateBlob(container_client, "dir/", "");
+ }
+ CreateBlob(container_client, "dir/file0", PreexistingData::kLoremIpsum);
+ }
+ };
+ setup_dir_file0();
+
+ // Trying to delete a non-existing file in an existing directory should fail
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Path does not exist '" + data.Path("dir/nonexistent-file") +
+ "'"),
+ fs()->DeleteFile(data.Path("dir/nonexistent-file")));
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Not a regular file: '" +
+ data.Path("dir/nonexistent-file/") + "'"),
+ fs()->DeleteFile(data.Path("dir/nonexistent-file/")));
+
+ // Trying to delete the directory with DeleteFile should fail
+ if (WithHierarchicalNamespace()) {
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError, ::testing::HasSubstr("Not a regular file: '" + data.Path("dir") + "'"),
+ fs()->DeleteFile(data.Path("dir")));
+ } else {
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError, ::testing::HasSubstr("Path does not exist '" + data.Path("dir") + "'"),
+ fs()->DeleteFile(data.Path("dir")));
+ }
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError, ::testing::HasSubstr("Not a regular file: '" + data.Path("dir/") + "'"),
+ fs()->DeleteFile(data.Path("dir/")));
+
+ arrow::fs::AssertFileInfo(fs(), data.Path("dir"), FileType::Directory);
+ arrow::fs::AssertFileInfo(fs(), data.Path("dir/"), FileType::Directory);
+ arrow::fs::AssertFileInfo(fs(), data.Path("dir/file0"), FileType::File);
+ ASSERT_OK(fs()->DeleteFile(data.Path("dir/file0")));
+ arrow::fs::AssertFileInfo(fs(), data.Path("dir"), FileType::Directory);
+ arrow::fs::AssertFileInfo(fs(), data.Path("dir/"), FileType::Directory);
+ arrow::fs::AssertFileInfo(fs(), data.Path("dir/file0"), FileType::NotFound);
+
+ // Recreating the file on the same path gurantees leases were properly released/broken
+ setup_dir_file0();
+
+ arrow::fs::AssertFileInfo(fs(), data.Path("dir/file0"), FileType::File);
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Not a regular file: '" + data.Path("dir/file0/") + "'"),
+ fs()->DeleteFile(data.Path("dir/file0/")));
Review Comment:
Done.
--
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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #40075:
URL: https://github.com/apache/arrow/pull/40075#issuecomment-1947647753
@kou I added a commit making error messages a bit more detailed now and removed the `WithHierarchicalNamespace()` check splitting the tests because the different version were behaving differently in some situations.
--
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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #40075:
URL: https://github.com/apache/arrow/pull/40075#discussion_r1496736135
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -2055,6 +2039,132 @@ 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};
+
+ // When it's known that the blob doesn't exist as a file, check if it exists as a
+ // directory to generate the appropriate error message.
+ auto check_if_location_exists_as_dir = [&]() -> Status {
+ auto no_trailing_slash = location;
+ no_trailing_slash.path = internal::RemoveTrailingSlash(location.path);
+ no_trailing_slash.all = internal::RemoveTrailingSlash(location.all);
+ ARROW_ASSIGN_OR_RAISE(auto file_info,
+ GetFileInfo(container_client, no_trailing_slash));
+ if (file_info.type() == FileType::NotFound) {
+ return require_file_to_exist ? PathNotFound(location) : Status::OK();
+ }
+ if (file_info.type() == FileType::Directory) {
+ return internal::NotAFile(location.all);
+ }
+ return internal::HasTrailingSlash(location.path) ? internal::NotADir(location.all)
+ : internal::NotAFile(location.all);
+ };
+
+ // Paths ending with trailing slashes are never leading to a deletion,
+ // but the correct error message requires a check of the path.
+ if (internal::HasTrailingSlash(location.path)) {
+ return check_if_location_exists_as_dir();
+ }
+
+ // If the parent directory of a file is not the container itself, there is a
+ // risk that deleting the file also deletes the *implied directory* -- the
+ // directory that is implied by the existence of the file path.
+ //
+ // In this case, we must ensure that the deletion is not semantically
+ // equivalent to also deleting the directory. This is done by ensuring the
+ // directory marker blob exists before the file is deleted.
+ std::optional<LeaseGuard> file_blob_lease_guard;
+ const auto parent = location.parent();
+ if (!parent.path.empty()) {
+ // We have to check the existence of the file before checking the
+ // existence of the parent directory marker, so we acquire a lease on the
+ // file first.
+ ARROW_ASSIGN_OR_RAISE(auto file_blob_lease_client,
+ AcquireBlobLease(location, kFileBlobLeaseTime,
+ /*allow_missing=*/true));
+ if (file_blob_lease_client) {
+ file_blob_lease_guard.emplace(std::move(file_blob_lease_client),
+ kFileBlobLeaseTime);
+ // Ensure the empty directory marker blob of the parent exists before the file is
+ // deleted.
+ //
+ // There is not need to hold a lease on the directory marker because if
+ // a concurrent client deletes the directory marker right after we
+ // create it, the file deletion itself won't be the cause of the directory
+ // deletion. Additionally, the fact that a lease is held on the blob path
+ // semantically preserves the directory -- its existence is implied
+ // until the blob representing the file is deleted -- even if another
+ // client deletes the directory marker.
+ RETURN_NOT_OK(EnsureEmptyDirExists(container_client, parent, operation));
+ } else {
+ return check_if_location_exists_as_dir();
+ }
+ }
+
+ DCHECK(!internal::HasTrailingSlash(location.path));
Review Comment:
Removing.
--
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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #40075:
URL: https://github.com/apache/arrow/pull/40075#discussion_r1491400517
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -889,6 +889,117 @@ class TestAzureFileSystem : public ::testing::Test {
ASSERT_RAISES(IOError, fs()->DeleteDirContents(directory_path, false));
}
+ void TestDeleteFileAtRoot() {
+ ASSERT_RAISES(Invalid, fs()->DeleteFile("file0"));
+ ASSERT_RAISES(Invalid, fs()->DeleteFile("file1/"));
+ const auto container_name = PreexistingData::RandomContainerName(rng_);
+ if (WithHierarchicalNamespace()) {
+ ARROW_UNUSED(CreateFilesystem(container_name));
+ } else {
+ ARROW_UNUSED(CreateContainer(container_name));
+ }
+ arrow::fs::AssertFileInfo(fs(), container_name, FileType::Directory);
+ ASSERT_RAISES(IOError, fs()->DeleteFile(container_name));
+ ASSERT_RAISES(IOError, fs()->DeleteFile(container_name + "/"));
+ }
+
+ void TestDeleteFileAtContainerRoot() {
+ auto data = SetUpPreexistingData();
+
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Path does not exist '" + data.Path("nonexistent-file") +
+ "'"),
+ fs()->DeleteFile(data.Path("nonexistent-file")));
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Not a regular file: '" + data.Path("nonexistent-file/") +
+ "'"),
+ fs()->DeleteFile(data.Path("nonexistent-file/")));
+
+ arrow::fs::AssertFileInfo(fs(), data.ObjectPath(), FileType::File);
+ ASSERT_OK(fs()->DeleteFile(data.ObjectPath()));
+ arrow::fs::AssertFileInfo(fs(), data.ObjectPath(), FileType::NotFound);
+
+ if (WithHierarchicalNamespace()) {
+ auto adlfs_client =
+ datalake_service_client_->GetFileSystemClient(data.container_name);
+ CreateFile(adlfs_client, data.kObjectName, PreexistingData::kLoremIpsum);
+ } else {
+ auto container_client = CreateContainer(data.container_name);
+ CreateBlob(container_client, data.kObjectName, PreexistingData::kLoremIpsum);
+ }
+
+ arrow::fs::AssertFileInfo(fs(), data.ObjectPath(), FileType::File);
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError, ::testing::HasSubstr("Not a regular file: '" + data.ObjectPath() + "/'"),
+ fs()->DeleteFile(data.ObjectPath() + "/"));
+ ASSERT_RAISES(IOError, fs()->DeleteFile(data.ObjectPath() + "/"));
+ }
+
+ void TestDeleteFileAtSubdirectory(bool create_empty_dir_marker_first) {
+ auto data = SetUpPreexistingData();
+
+ auto setup_dir_file0 = [this, create_empty_dir_marker_first, &data]() {
+ if (WithHierarchicalNamespace()) {
+ ASSERT_FALSE(create_empty_dir_marker_first);
+ auto adlfs_client =
+ datalake_service_client_->GetFileSystemClient(data.container_name);
+ CreateFile(adlfs_client, "dir/file0", PreexistingData::kLoremIpsum);
+ } else {
+ auto container_client = CreateContainer(data.container_name);
+ if (create_empty_dir_marker_first) {
+ CreateBlob(container_client, "dir/", "");
+ }
+ CreateBlob(container_client, "dir/file0", PreexistingData::kLoremIpsum);
+ }
+ };
+ setup_dir_file0();
+
+ // Trying to delete a non-existing file in an existing directory should fail
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Path does not exist '" + data.Path("dir/nonexistent-file") +
+ "'"),
+ fs()->DeleteFile(data.Path("dir/nonexistent-file")));
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Not a regular file: '" +
+ data.Path("dir/nonexistent-file/") + "'"),
+ fs()->DeleteFile(data.Path("dir/nonexistent-file/")));
Review Comment:
Same here.
--
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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #40075:
URL: https://github.com/apache/arrow/pull/40075#discussion_r1491877474
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -2055,6 +2035,116 @@ class AzureFileSystem::Impl {
static constexpr auto kTimeNeededForFileOrDirectoryRename = std::chrono::seconds{3};
public:
+ /// \pre location.container is not empty.
+ /// \pre location.path is not empty.
+ /// \pre location.path does not end with a trailing slash.
+ Status DeleteFileOnFileSystem(const DataLake::DataLakeFileSystemClient& adlfs_client,
+ const AzureLocation& location, bool require_file_to_exist,
+ Azure::Nullable<std::string> lease_id = {}) {
+ DCHECK(!location.container.empty());
+ DCHECK(!location.path.empty());
+ DCHECK(!internal::HasTrailingSlash(location.path));
+ auto file_client = adlfs_client.GetFileClient(location.path);
+ DataLake::GetPathPropertiesOptions get_options;
+ get_options.AccessConditions.LeaseId = lease_id;
+ DataLake::DeleteFileOptions delete_options;
+ delete_options.AccessConditions.LeaseId = std::move(lease_id);
+ try {
+ // This is necessary to avoid deletion of directories via DeleteFile.
+ auto properties = file_client.GetProperties(get_options);
+ if (properties.Value.IsDirectory) {
+ return internal::NotAFile(location.all);
+ }
+ auto response = file_client.Delete(delete_options);
+ // 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.
+ /// \pre location.path does not end with a trailing slash.
+ 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());
+ DCHECK(!internal::HasTrailingSlash(location.path));
+ constexpr auto kFileBlobLeaseTime = std::chrono::seconds{15};
+
+ // If the parent directory of a file is not the container itself, there is a
+ // risk that deleting the file also deletes the *implied directory* -- the
+ // directory that is implied by the existence of the file path.
+ //
+ // In this case, we must ensure that the deletion is not semantically
+ // equivalent to also deleting the directory. This is done by ensuring the
+ // directory marker blob exists before the file is deleted.
+ std::optional<LeaseGuard> file_blob_lease_guard;
+ const auto parent = location.parent();
+ if (!parent.path.empty()) {
+ // We have to check the existence of the file before checking the
+ // existence of the parent directory marker, so we acquire a lease on the
+ // file first.
+ ARROW_ASSIGN_OR_RAISE(auto file_blob_lease_client,
+ AcquireBlobLease(location, kFileBlobLeaseTime,
+ /*allow_missing=*/!require_file_to_exist));
+ if (file_blob_lease_client) {
+ file_blob_lease_guard.emplace(std::move(file_blob_lease_client),
+ kFileBlobLeaseTime);
+ // Ensure the empty directory marker blob exists before the file is deleted.
+ //
+ // There is not need to hold a lease on the directory marker because if
+ // a concurrent client deletes the directory marker right after we
+ // create it, the file deletion itself won't be the cause of the directory
+ // deletion. Additionally, the fact that a lease is held on the blob path
+ // semantically preserves the directory -- its existence is implied
+ // until the blob representing the file is deleted -- even if another
+ // client deletes the directory marker.
+ RETURN_NOT_OK(EnsureEmptyDirExists(container_client, parent, operation));
+ } else {
+ // File blob doesn't exist and require_file_to_exists is false, so just return OK.
+ // Trying to delete a file that doesn't exist, costs only one request to the
+ // server.
+ DCHECK(!require_file_to_exist);
+ return Status::OK();
+ }
+ }
+
+ auto blob_client = container_client.GetBlobClient(location.path);
+ Blobs::DeleteBlobOptions options;
+ if (file_blob_lease_guard) {
+ options.AccessConditions.LeaseId = file_blob_lease_guard->LeaseId();
+ }
+ try {
+ auto response = blob_client.Delete(options);
Review Comment:
Alright. Adding a comment on `BreakBeforeDeletion`.
--
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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #40075:
URL: https://github.com/apache/arrow/pull/40075#discussion_r1494493237
##########
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:
Alright. I'm fixing `GetFileInfo`, then.
--
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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #40075:
URL: https://github.com/apache/arrow/pull/40075#discussion_r1492550021
##########
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:
What about `RemoveTrailingSlashFromPath()`?
--
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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #40075:
URL: https://github.com/apache/arrow/pull/40075#issuecomment-1942954043
@av8or1
--
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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #40075:
URL: https://github.com/apache/arrow/pull/40075#issuecomment-1956257877
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit a2d072929fea956a77775671e2eaf20f9d0ed5fa.
There were 2 benchmark results with an error:
- Commit Run on `ursa-i9-9960x` at [2024-02-21 01:17:24Z](https://conbench.ursa.dev/compare/runs/474505c713034247be655f66f887d00e...15e50e09be094fc6bbc44e4af3c896c4/)
- [`tpch` (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-06, scale_factor=10](https://conbench.ursa.dev/benchmark-results/065d5635e97a7c768000e865f2b94edc)
- [`tpch` (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-04, scale_factor=10](https://conbench.ursa.dev/benchmark-results/065d562dc59a78898000973e8b1ecf05)
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/21808534806) has more details. It also includes information about 2 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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #40075:
URL: https://github.com/apache/arrow/pull/40075#issuecomment-1942951273
:warning: GitHub issue #40074 **has been automatically assigned in GitHub** to PR creator.
--
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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #40075:
URL: https://github.com/apache/arrow/pull/40075#discussion_r1494493237
##########
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:
Alright. I'm fixing `GetFileInfo`, then.
--
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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #40075:
URL: https://github.com/apache/arrow/pull/40075#discussion_r1491878105
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -2544,7 +2635,28 @@ Status AzureFileSystem::DeleteRootDirContents() {
Status AzureFileSystem::DeleteFile(const std::string& path) {
ARROW_ASSIGN_OR_RAISE(auto location, AzureLocation::FromString(path));
- return impl_->DeleteFile(location);
+ if (location.container.empty()) {
+ return Status::Invalid("DeleteFile requires a non-empty path.");
+ }
+ if (internal::HasTrailingSlash(location.path) || location.path.empty()) {
+ // Container paths (locations w/o path) or paths ending with a slash are not file
+ // paths.
+ return NotAFile(location);
+ }
Review Comment:
Same checks, but different errors returned. Anyways, I changed the logic even more now and the trailing slashes are handled inside `DeleteFileOnFileSystem/DeleteFileOnContainer` instead.
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -2055,6 +2035,116 @@ class AzureFileSystem::Impl {
static constexpr auto kTimeNeededForFileOrDirectoryRename = std::chrono::seconds{3};
public:
+ /// \pre location.container is not empty.
+ /// \pre location.path is not empty.
+ /// \pre location.path does not end with a trailing slash.
+ Status DeleteFileOnFileSystem(const DataLake::DataLakeFileSystemClient& adlfs_client,
+ const AzureLocation& location, bool require_file_to_exist,
+ Azure::Nullable<std::string> lease_id = {}) {
Review Comment:
I removed it in the latest commits.
--
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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #40075:
URL: https://github.com/apache/arrow/pull/40075#discussion_r1491398538
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -2055,6 +2035,116 @@ class AzureFileSystem::Impl {
static constexpr auto kTimeNeededForFileOrDirectoryRename = std::chrono::seconds{3};
public:
+ /// \pre location.container is not empty.
+ /// \pre location.path is not empty.
+ /// \pre location.path does not end with a trailing slash.
+ Status DeleteFileOnFileSystem(const DataLake::DataLakeFileSystemClient& adlfs_client,
+ const AzureLocation& location, bool require_file_to_exist,
+ Azure::Nullable<std::string> lease_id = {}) {
+ DCHECK(!location.container.empty());
+ DCHECK(!location.path.empty());
+ DCHECK(!internal::HasTrailingSlash(location.path));
+ auto file_client = adlfs_client.GetFileClient(location.path);
+ DataLake::GetPathPropertiesOptions get_options;
+ get_options.AccessConditions.LeaseId = lease_id;
+ DataLake::DeleteFileOptions delete_options;
+ delete_options.AccessConditions.LeaseId = std::move(lease_id);
+ try {
+ // This is necessary to avoid deletion of directories via DeleteFile.
+ auto properties = file_client.GetProperties(get_options);
+ if (properties.Value.IsDirectory) {
+ return internal::NotAFile(location.all);
+ }
+ auto response = file_client.Delete(delete_options);
+ // 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.
+ /// \pre location.path does not end with a trailing slash.
+ 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());
+ DCHECK(!internal::HasTrailingSlash(location.path));
+ constexpr auto kFileBlobLeaseTime = std::chrono::seconds{15};
+
+ // If the parent directory of a file is not the container itself, there is a
+ // risk that deleting the file also deletes the *implied directory* -- the
+ // directory that is implied by the existence of the file path.
+ //
+ // In this case, we must ensure that the deletion is not semantically
+ // equivalent to also deleting the directory. This is done by ensuring the
+ // directory marker blob exists before the file is deleted.
+ std::optional<LeaseGuard> file_blob_lease_guard;
+ const auto parent = location.parent();
+ if (!parent.path.empty()) {
+ // We have to check the existence of the file before checking the
+ // existence of the parent directory marker, so we acquire a lease on the
+ // file first.
+ ARROW_ASSIGN_OR_RAISE(auto file_blob_lease_client,
+ AcquireBlobLease(location, kFileBlobLeaseTime,
+ /*allow_missing=*/!require_file_to_exist));
+ if (file_blob_lease_client) {
+ file_blob_lease_guard.emplace(std::move(file_blob_lease_client),
+ kFileBlobLeaseTime);
+ // Ensure the empty directory marker blob exists before the file is deleted.
+ //
+ // There is not need to hold a lease on the directory marker because if
+ // a concurrent client deletes the directory marker right after we
+ // create it, the file deletion itself won't be the cause of the directory
+ // deletion. Additionally, the fact that a lease is held on the blob path
+ // semantically preserves the directory -- its existence is implied
+ // until the blob representing the file is deleted -- even if another
+ // client deletes the directory marker.
+ RETURN_NOT_OK(EnsureEmptyDirExists(container_client, parent, operation));
+ } else {
+ // File blob doesn't exist and require_file_to_exists is false, so just return OK.
+ // Trying to delete a file that doesn't exist, costs only one request to the
+ // server.
+ DCHECK(!require_file_to_exist);
+ return Status::OK();
+ }
+ }
+
+ auto blob_client = container_client.GetBlobClient(location.path);
+ Blobs::DeleteBlobOptions options;
+ if (file_blob_lease_guard) {
+ options.AccessConditions.LeaseId = file_blob_lease_guard->LeaseId();
+ }
+ try {
+ auto response = blob_client.Delete(options);
Review Comment:
This was surprising to me as well. In the Azure Blobs API if I delete a blob that’s holding a lease I can release the lease on that path just fine. Re-creating a Blob on the same path immediately after deletion+release works just fine. This was a mess in the DataLake API, but is fine in the Blobs API.
--
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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #40075:
URL: https://github.com/apache/arrow/pull/40075#discussion_r1492535545
##########
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:
I want `all` to be path the user used so error messages make sense.
--
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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #40075:
URL: https://github.com/apache/arrow/pull/40075#discussion_r1491402918
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -889,6 +889,117 @@ class TestAzureFileSystem : public ::testing::Test {
ASSERT_RAISES(IOError, fs()->DeleteDirContents(directory_path, false));
}
+ void TestDeleteFileAtRoot() {
+ ASSERT_RAISES(Invalid, fs()->DeleteFile("file0"));
+ ASSERT_RAISES(Invalid, fs()->DeleteFile("file1/"));
+ const auto container_name = PreexistingData::RandomContainerName(rng_);
+ if (WithHierarchicalNamespace()) {
+ ARROW_UNUSED(CreateFilesystem(container_name));
+ } else {
+ ARROW_UNUSED(CreateContainer(container_name));
+ }
+ arrow::fs::AssertFileInfo(fs(), container_name, FileType::Directory);
+ ASSERT_RAISES(IOError, fs()->DeleteFile(container_name));
+ ASSERT_RAISES(IOError, fs()->DeleteFile(container_name + "/"));
+ }
+
+ void TestDeleteFileAtContainerRoot() {
+ auto data = SetUpPreexistingData();
+
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Path does not exist '" + data.Path("nonexistent-file") +
+ "'"),
+ fs()->DeleteFile(data.Path("nonexistent-file")));
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Not a regular file: '" + data.Path("nonexistent-file/") +
+ "'"),
+ fs()->DeleteFile(data.Path("nonexistent-file/")));
+
+ arrow::fs::AssertFileInfo(fs(), data.ObjectPath(), FileType::File);
+ ASSERT_OK(fs()->DeleteFile(data.ObjectPath()));
+ arrow::fs::AssertFileInfo(fs(), data.ObjectPath(), FileType::NotFound);
+
+ if (WithHierarchicalNamespace()) {
+ auto adlfs_client =
+ datalake_service_client_->GetFileSystemClient(data.container_name);
+ CreateFile(adlfs_client, data.kObjectName, PreexistingData::kLoremIpsum);
+ } else {
+ auto container_client = CreateContainer(data.container_name);
+ CreateBlob(container_client, data.kObjectName, PreexistingData::kLoremIpsum);
+ }
+
+ arrow::fs::AssertFileInfo(fs(), data.ObjectPath(), FileType::File);
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError, ::testing::HasSubstr("Not a regular file: '" + data.ObjectPath() + "/'"),
+ fs()->DeleteFile(data.ObjectPath() + "/"));
+ ASSERT_RAISES(IOError, fs()->DeleteFile(data.ObjectPath() + "/"));
+ }
+
+ void TestDeleteFileAtSubdirectory(bool create_empty_dir_marker_first) {
+ auto data = SetUpPreexistingData();
+
+ auto setup_dir_file0 = [this, create_empty_dir_marker_first, &data]() {
+ if (WithHierarchicalNamespace()) {
+ ASSERT_FALSE(create_empty_dir_marker_first);
+ auto adlfs_client =
+ datalake_service_client_->GetFileSystemClient(data.container_name);
+ CreateFile(adlfs_client, "dir/file0", PreexistingData::kLoremIpsum);
+ } else {
+ auto container_client = CreateContainer(data.container_name);
+ if (create_empty_dir_marker_first) {
+ CreateBlob(container_client, "dir/", "");
+ }
+ CreateBlob(container_client, "dir/file0", PreexistingData::kLoremIpsum);
+ }
+ };
+ setup_dir_file0();
+
+ // Trying to delete a non-existing file in an existing directory should fail
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Path does not exist '" + data.Path("dir/nonexistent-file") +
+ "'"),
+ fs()->DeleteFile(data.Path("dir/nonexistent-file")));
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Not a regular file: '" +
+ data.Path("dir/nonexistent-file/") + "'"),
+ fs()->DeleteFile(data.Path("dir/nonexistent-file/")));
+
+ // Trying to delete the directory with DeleteFile should fail
+ if (WithHierarchicalNamespace()) {
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError, ::testing::HasSubstr("Not a regular file: '" + data.Path("dir") + "'"),
+ fs()->DeleteFile(data.Path("dir")));
+ } else {
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError, ::testing::HasSubstr("Path does not exist '" + data.Path("dir") + "'"),
+ fs()->DeleteFile(data.Path("dir")));
+ }
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError, ::testing::HasSubstr("Not a regular file: '" + data.Path("dir/") + "'"),
+ fs()->DeleteFile(data.Path("dir/")));
+
+ arrow::fs::AssertFileInfo(fs(), data.Path("dir"), FileType::Directory);
+ arrow::fs::AssertFileInfo(fs(), data.Path("dir/"), FileType::Directory);
+ arrow::fs::AssertFileInfo(fs(), data.Path("dir/file0"), FileType::File);
+ ASSERT_OK(fs()->DeleteFile(data.Path("dir/file0")));
+ arrow::fs::AssertFileInfo(fs(), data.Path("dir"), FileType::Directory);
+ arrow::fs::AssertFileInfo(fs(), data.Path("dir/"), FileType::Directory);
+ arrow::fs::AssertFileInfo(fs(), data.Path("dir/file0"), FileType::NotFound);
+
+ // Recreating the file on the same path gurantees leases were properly released/broken
Review Comment:
But I'm using the same path to test the lease management code.
--
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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #40075:
URL: https://github.com/apache/arrow/pull/40075#discussion_r1491399770
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -889,6 +889,117 @@ class TestAzureFileSystem : public ::testing::Test {
ASSERT_RAISES(IOError, fs()->DeleteDirContents(directory_path, false));
}
+ void TestDeleteFileAtRoot() {
+ ASSERT_RAISES(Invalid, fs()->DeleteFile("file0"));
+ ASSERT_RAISES(Invalid, fs()->DeleteFile("file1/"));
+ const auto container_name = PreexistingData::RandomContainerName(rng_);
+ if (WithHierarchicalNamespace()) {
+ ARROW_UNUSED(CreateFilesystem(container_name));
+ } else {
+ ARROW_UNUSED(CreateContainer(container_name));
+ }
+ arrow::fs::AssertFileInfo(fs(), container_name, FileType::Directory);
+ ASSERT_RAISES(IOError, fs()->DeleteFile(container_name));
+ ASSERT_RAISES(IOError, fs()->DeleteFile(container_name + "/"));
+ }
+
+ void TestDeleteFileAtContainerRoot() {
+ auto data = SetUpPreexistingData();
+
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Path does not exist '" + data.Path("nonexistent-file") +
+ "'"),
+ fs()->DeleteFile(data.Path("nonexistent-file")));
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Not a regular file: '" + data.Path("nonexistent-file/") +
+ "'"),
+ fs()->DeleteFile(data.Path("nonexistent-file/")));
Review Comment:
This is supposed to be just like the test above, but with a slash in the path. I will replace "file" with "path" so I can keep them the same.
--
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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv merged PR #40075:
URL: https://github.com/apache/arrow/pull/40075
--
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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #40075:
URL: https://github.com/apache/arrow/pull/40075#discussion_r1494515619
##########
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:
Test failures when I remove this:
```cpp
[ RUN ] TestAzureFileSystemOnAllScenarios/2.DeleteFileAtContainerRoot
/Users/felipe/code/arrow/cpp/src/arrow/filesystem/azurefs_test.cc:942: Failure
Failed
'fs()->DeleteFile(data.ObjectPath() + "/")' did not fail with errno=ENOTDIR: IOError: Path does not exist 'w3ay9l35qoxg0i0lqbfababvwbrrrhxq/test-object-name/'. Detail: [errno 2] No such file or directory
[ FAILED ] TestAzureFileSystemOnAllScenarios/2.DeleteFileAtContainerRoot, where TypeParam = arrow::fs::TestingScenario<arrow::fs::AzureFlatNSEnv,true> (3185 ms)
[ RUN ] TestAzureFileSystemOnAllScenarios/2.DeleteFileAtSubdirectory
/Users/felipe/code/arrow/cpp/src/arrow/filesystem/azurefs_test.cc:1000: Failure
Value of: _st.ToStringWithoutContextLines()
Expected: has substring "Not a directory: 'sco3n8q67r3cd2mas6lx53nlc36uw7f9/dir/file0/'"
Actual: "IOError: Path does not exist 'sco3n8q67r3cd2mas6lx53nlc36uw7f9/dir/file0/'. Detail: [errno 2] No such file or directory"
/Users/felipe/code/arrow/cpp/src/arrow/filesystem/azurefs_test.cc:1000: Failure
Value of: _st.ToStringWithoutContextLines()
Expected: has substring "Not a directory: '3nw7fyp8whjb3r8e6g7eoyg2zav8nr8y/dir/file0/'"
Actual: "IOError: Path does not exist '3nw7fyp8whjb3r8e6g7eoyg2zav8nr8y/dir/file0/'. Detail: [errno 2] No such file or directory"
[ FAILED ] TestAzureFileSystemOnAllScenarios/2.DeleteFileAtSubdirectory, where TypeParam = arrow::fs::TestingScenario<arrow::fs::AzureFlatNSEnv,true> (12108 ms)
```
I'm going to implement this by copying `location` and removing the trailing slashes manually.
--
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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #40075:
URL: https://github.com/apache/arrow/pull/40075#discussion_r1496736887
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -2055,6 +2039,132 @@ 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) {
Review Comment:
I might use `Move()` with it, so I will keep 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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #40075:
URL: https://github.com/apache/arrow/pull/40075#discussion_r1490326513
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -2055,6 +2035,116 @@ class AzureFileSystem::Impl {
static constexpr auto kTimeNeededForFileOrDirectoryRename = std::chrono::seconds{3};
public:
+ /// \pre location.container is not empty.
+ /// \pre location.path is not empty.
+ /// \pre location.path does not end with a trailing slash.
+ Status DeleteFileOnFileSystem(const DataLake::DataLakeFileSystemClient& adlfs_client,
+ const AzureLocation& location, bool require_file_to_exist,
+ Azure::Nullable<std::string> lease_id = {}) {
+ DCHECK(!location.container.empty());
+ DCHECK(!location.path.empty());
+ DCHECK(!internal::HasTrailingSlash(location.path));
+ auto file_client = adlfs_client.GetFileClient(location.path);
+ DataLake::GetPathPropertiesOptions get_options;
+ get_options.AccessConditions.LeaseId = lease_id;
+ DataLake::DeleteFileOptions delete_options;
+ delete_options.AccessConditions.LeaseId = std::move(lease_id);
+ try {
+ // This is necessary to avoid deletion of directories via DeleteFile.
+ auto properties = file_client.GetProperties(get_options);
+ if (properties.Value.IsDirectory) {
+ return internal::NotAFile(location.all);
+ }
+ auto response = file_client.Delete(delete_options);
+ // 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.
+ /// \pre location.path does not end with a trailing slash.
+ 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());
+ DCHECK(!internal::HasTrailingSlash(location.path));
+ constexpr auto kFileBlobLeaseTime = std::chrono::seconds{15};
+
+ // If the parent directory of a file is not the container itself, there is a
+ // risk that deleting the file also deletes the *implied directory* -- the
+ // directory that is implied by the existence of the file path.
+ //
+ // In this case, we must ensure that the deletion is not semantically
+ // equivalent to also deleting the directory. This is done by ensuring the
+ // directory marker blob exists before the file is deleted.
+ std::optional<LeaseGuard> file_blob_lease_guard;
+ const auto parent = location.parent();
+ if (!parent.path.empty()) {
+ // We have to check the existence of the file before checking the
+ // existence of the parent directory marker, so we acquire a lease on the
+ // file first.
+ ARROW_ASSIGN_OR_RAISE(auto file_blob_lease_client,
+ AcquireBlobLease(location, kFileBlobLeaseTime,
+ /*allow_missing=*/!require_file_to_exist));
+ if (file_blob_lease_client) {
+ file_blob_lease_guard.emplace(std::move(file_blob_lease_client),
+ kFileBlobLeaseTime);
+ // Ensure the empty directory marker blob exists before the file is deleted.
+ //
+ // There is not need to hold a lease on the directory marker because if
+ // a concurrent client deletes the directory marker right after we
+ // create it, the file deletion itself won't be the cause of the directory
+ // deletion. Additionally, the fact that a lease is held on the blob path
+ // semantically preserves the directory -- its existence is implied
+ // until the blob representing the file is deleted -- even if another
+ // client deletes the directory marker.
+ RETURN_NOT_OK(EnsureEmptyDirExists(container_client, parent, operation));
+ } else {
+ // File blob doesn't exist and require_file_to_exists is false, so just return OK.
+ // Trying to delete a file that doesn't exist, costs only one request to the
+ // server.
+ DCHECK(!require_file_to_exist);
+ return Status::OK();
+ }
+ }
+
+ auto blob_client = container_client.GetBlobClient(location.path);
+ Blobs::DeleteBlobOptions options;
+ if (file_blob_lease_guard) {
+ options.AccessConditions.LeaseId = file_blob_lease_guard->LeaseId();
+ }
+ try {
+ auto response = blob_client.Delete(options);
Review Comment:
Do we need to call `file_blob_lease_guard->BreakBeforeDeletion()` before `Delete()`?
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -889,6 +889,117 @@ class TestAzureFileSystem : public ::testing::Test {
ASSERT_RAISES(IOError, fs()->DeleteDirContents(directory_path, false));
}
+ void TestDeleteFileAtRoot() {
+ ASSERT_RAISES(Invalid, fs()->DeleteFile("file0"));
+ ASSERT_RAISES(Invalid, fs()->DeleteFile("file1/"));
+ const auto container_name = PreexistingData::RandomContainerName(rng_);
+ if (WithHierarchicalNamespace()) {
+ ARROW_UNUSED(CreateFilesystem(container_name));
+ } else {
+ ARROW_UNUSED(CreateContainer(container_name));
+ }
+ arrow::fs::AssertFileInfo(fs(), container_name, FileType::Directory);
+ ASSERT_RAISES(IOError, fs()->DeleteFile(container_name));
+ ASSERT_RAISES(IOError, fs()->DeleteFile(container_name + "/"));
+ }
+
+ void TestDeleteFileAtContainerRoot() {
+ auto data = SetUpPreexistingData();
+
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Path does not exist '" + data.Path("nonexistent-file") +
+ "'"),
+ fs()->DeleteFile(data.Path("nonexistent-file")));
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Not a regular file: '" + data.Path("nonexistent-file/") +
+ "'"),
+ fs()->DeleteFile(data.Path("nonexistent-file/")));
+
+ arrow::fs::AssertFileInfo(fs(), data.ObjectPath(), FileType::File);
+ ASSERT_OK(fs()->DeleteFile(data.ObjectPath()));
+ arrow::fs::AssertFileInfo(fs(), data.ObjectPath(), FileType::NotFound);
+
+ if (WithHierarchicalNamespace()) {
+ auto adlfs_client =
+ datalake_service_client_->GetFileSystemClient(data.container_name);
+ CreateFile(adlfs_client, data.kObjectName, PreexistingData::kLoremIpsum);
+ } else {
+ auto container_client = CreateContainer(data.container_name);
+ CreateBlob(container_client, data.kObjectName, PreexistingData::kLoremIpsum);
+ }
+
+ arrow::fs::AssertFileInfo(fs(), data.ObjectPath(), FileType::File);
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError, ::testing::HasSubstr("Not a regular file: '" + data.ObjectPath() + "/'"),
+ fs()->DeleteFile(data.ObjectPath() + "/"));
+ ASSERT_RAISES(IOError, fs()->DeleteFile(data.ObjectPath() + "/"));
+ }
+
+ void TestDeleteFileAtSubdirectory(bool create_empty_dir_marker_first) {
+ auto data = SetUpPreexistingData();
+
+ auto setup_dir_file0 = [this, create_empty_dir_marker_first, &data]() {
+ if (WithHierarchicalNamespace()) {
+ ASSERT_FALSE(create_empty_dir_marker_first);
+ auto adlfs_client =
+ datalake_service_client_->GetFileSystemClient(data.container_name);
+ CreateFile(adlfs_client, "dir/file0", PreexistingData::kLoremIpsum);
+ } else {
+ auto container_client = CreateContainer(data.container_name);
+ if (create_empty_dir_marker_first) {
+ CreateBlob(container_client, "dir/", "");
+ }
+ CreateBlob(container_client, "dir/file0", PreexistingData::kLoremIpsum);
+ }
+ };
+ setup_dir_file0();
+
+ // Trying to delete a non-existing file in an existing directory should fail
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Path does not exist '" + data.Path("dir/nonexistent-file") +
+ "'"),
+ fs()->DeleteFile(data.Path("dir/nonexistent-file")));
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Not a regular file: '" +
+ data.Path("dir/nonexistent-file/") + "'"),
+ fs()->DeleteFile(data.Path("dir/nonexistent-file/")));
Review Comment:
```suggestion
data.Path("dir/nonexistent-dir/") + "'"),
fs()->DeleteFile(data.Path("dir/nonexistent-dir/")));
```
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -2544,7 +2635,28 @@ Status AzureFileSystem::DeleteRootDirContents() {
Status AzureFileSystem::DeleteFile(const std::string& path) {
ARROW_ASSIGN_OR_RAISE(auto location, AzureLocation::FromString(path));
- return impl_->DeleteFile(location);
+ if (location.container.empty()) {
+ return Status::Invalid("DeleteFile requires a non-empty path.");
+ }
+ if (internal::HasTrailingSlash(location.path) || location.path.empty()) {
+ // Container paths (locations w/o path) or paths ending with a slash are not file
+ // paths.
+ return NotAFile(location);
+ }
Review Comment:
Can we use `ValidateFileLocation()`?
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -2055,6 +2035,116 @@ class AzureFileSystem::Impl {
static constexpr auto kTimeNeededForFileOrDirectoryRename = std::chrono::seconds{3};
public:
+ /// \pre location.container is not empty.
+ /// \pre location.path is not empty.
+ /// \pre location.path does not end with a trailing slash.
+ Status DeleteFileOnFileSystem(const DataLake::DataLakeFileSystemClient& adlfs_client,
+ const AzureLocation& location, bool require_file_to_exist,
+ Azure::Nullable<std::string> lease_id = {}) {
Review Comment:
When do we need `lease_id`?
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -889,6 +889,117 @@ class TestAzureFileSystem : public ::testing::Test {
ASSERT_RAISES(IOError, fs()->DeleteDirContents(directory_path, false));
}
+ void TestDeleteFileAtRoot() {
+ ASSERT_RAISES(Invalid, fs()->DeleteFile("file0"));
+ ASSERT_RAISES(Invalid, fs()->DeleteFile("file1/"));
+ const auto container_name = PreexistingData::RandomContainerName(rng_);
+ if (WithHierarchicalNamespace()) {
+ ARROW_UNUSED(CreateFilesystem(container_name));
+ } else {
+ ARROW_UNUSED(CreateContainer(container_name));
+ }
+ arrow::fs::AssertFileInfo(fs(), container_name, FileType::Directory);
+ ASSERT_RAISES(IOError, fs()->DeleteFile(container_name));
+ ASSERT_RAISES(IOError, fs()->DeleteFile(container_name + "/"));
+ }
+
+ void TestDeleteFileAtContainerRoot() {
+ auto data = SetUpPreexistingData();
+
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Path does not exist '" + data.Path("nonexistent-file") +
+ "'"),
+ fs()->DeleteFile(data.Path("nonexistent-file")));
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Not a regular file: '" + data.Path("nonexistent-file/") +
+ "'"),
+ fs()->DeleteFile(data.Path("nonexistent-file/")));
Review Comment:
```suggestion
::testing::HasSubstr("Not a regular file: '" + data.Path("nonexistent-dir/") +
"'"),
fs()->DeleteFile(data.Path("nonexistent-dir/")));
```
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -1528,6 +1639,21 @@ TYPED_TEST(TestAzureFileSystemOnAllScenarios, DeleteDirContentsFailureNonexisten
this->TestDeleteDirContentsFailureNonexistent();
}
+TYPED_TEST(TestAzureFileSystemOnAllScenarios, DeleteFileAtRoot) {
+ void TestDeleteFileAtRoot();
Review Comment:
```suggestion
this->TestDeleteFileAtRoot();
```
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -889,6 +889,117 @@ class TestAzureFileSystem : public ::testing::Test {
ASSERT_RAISES(IOError, fs()->DeleteDirContents(directory_path, false));
}
+ void TestDeleteFileAtRoot() {
+ ASSERT_RAISES(Invalid, fs()->DeleteFile("file0"));
+ ASSERT_RAISES(Invalid, fs()->DeleteFile("file1/"));
+ const auto container_name = PreexistingData::RandomContainerName(rng_);
+ if (WithHierarchicalNamespace()) {
+ ARROW_UNUSED(CreateFilesystem(container_name));
+ } else {
+ ARROW_UNUSED(CreateContainer(container_name));
+ }
+ arrow::fs::AssertFileInfo(fs(), container_name, FileType::Directory);
+ ASSERT_RAISES(IOError, fs()->DeleteFile(container_name));
+ ASSERT_RAISES(IOError, fs()->DeleteFile(container_name + "/"));
+ }
+
+ void TestDeleteFileAtContainerRoot() {
+ auto data = SetUpPreexistingData();
+
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Path does not exist '" + data.Path("nonexistent-file") +
+ "'"),
+ fs()->DeleteFile(data.Path("nonexistent-file")));
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Not a regular file: '" + data.Path("nonexistent-file/") +
+ "'"),
+ fs()->DeleteFile(data.Path("nonexistent-file/")));
+
+ arrow::fs::AssertFileInfo(fs(), data.ObjectPath(), FileType::File);
+ ASSERT_OK(fs()->DeleteFile(data.ObjectPath()));
+ arrow::fs::AssertFileInfo(fs(), data.ObjectPath(), FileType::NotFound);
+
+ if (WithHierarchicalNamespace()) {
+ auto adlfs_client =
+ datalake_service_client_->GetFileSystemClient(data.container_name);
+ CreateFile(adlfs_client, data.kObjectName, PreexistingData::kLoremIpsum);
+ } else {
+ auto container_client = CreateContainer(data.container_name);
+ CreateBlob(container_client, data.kObjectName, PreexistingData::kLoremIpsum);
+ }
+
+ arrow::fs::AssertFileInfo(fs(), data.ObjectPath(), FileType::File);
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError, ::testing::HasSubstr("Not a regular file: '" + data.ObjectPath() + "/'"),
+ fs()->DeleteFile(data.ObjectPath() + "/"));
+ ASSERT_RAISES(IOError, fs()->DeleteFile(data.ObjectPath() + "/"));
+ }
+
+ void TestDeleteFileAtSubdirectory(bool create_empty_dir_marker_first) {
+ auto data = SetUpPreexistingData();
+
+ auto setup_dir_file0 = [this, create_empty_dir_marker_first, &data]() {
+ if (WithHierarchicalNamespace()) {
+ ASSERT_FALSE(create_empty_dir_marker_first);
+ auto adlfs_client =
+ datalake_service_client_->GetFileSystemClient(data.container_name);
+ CreateFile(adlfs_client, "dir/file0", PreexistingData::kLoremIpsum);
+ } else {
+ auto container_client = CreateContainer(data.container_name);
+ if (create_empty_dir_marker_first) {
+ CreateBlob(container_client, "dir/", "");
+ }
+ CreateBlob(container_client, "dir/file0", PreexistingData::kLoremIpsum);
+ }
+ };
+ setup_dir_file0();
+
+ // Trying to delete a non-existing file in an existing directory should fail
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Path does not exist '" + data.Path("dir/nonexistent-file") +
+ "'"),
+ fs()->DeleteFile(data.Path("dir/nonexistent-file")));
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Not a regular file: '" +
+ data.Path("dir/nonexistent-file/") + "'"),
+ fs()->DeleteFile(data.Path("dir/nonexistent-file/")));
+
+ // Trying to delete the directory with DeleteFile should fail
+ if (WithHierarchicalNamespace()) {
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError, ::testing::HasSubstr("Not a regular file: '" + data.Path("dir") + "'"),
+ fs()->DeleteFile(data.Path("dir")));
+ } else {
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError, ::testing::HasSubstr("Path does not exist '" + data.Path("dir") + "'"),
+ fs()->DeleteFile(data.Path("dir")));
+ }
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError, ::testing::HasSubstr("Not a regular file: '" + data.Path("dir/") + "'"),
+ fs()->DeleteFile(data.Path("dir/")));
+
+ arrow::fs::AssertFileInfo(fs(), data.Path("dir"), FileType::Directory);
+ arrow::fs::AssertFileInfo(fs(), data.Path("dir/"), FileType::Directory);
+ arrow::fs::AssertFileInfo(fs(), data.Path("dir/file0"), FileType::File);
+ ASSERT_OK(fs()->DeleteFile(data.Path("dir/file0")));
+ arrow::fs::AssertFileInfo(fs(), data.Path("dir"), FileType::Directory);
+ arrow::fs::AssertFileInfo(fs(), data.Path("dir/"), FileType::Directory);
+ arrow::fs::AssertFileInfo(fs(), data.Path("dir/file0"), FileType::NotFound);
+
+ // Recreating the file on the same path gurantees leases were properly released/broken
Review Comment:
How about using different directory instead for it? It'll be cleaner.
##########
cpp/src/arrow/filesystem/azurefs_test.cc:
##########
@@ -889,6 +889,117 @@ class TestAzureFileSystem : public ::testing::Test {
ASSERT_RAISES(IOError, fs()->DeleteDirContents(directory_path, false));
}
+ void TestDeleteFileAtRoot() {
+ ASSERT_RAISES(Invalid, fs()->DeleteFile("file0"));
+ ASSERT_RAISES(Invalid, fs()->DeleteFile("file1/"));
+ const auto container_name = PreexistingData::RandomContainerName(rng_);
+ if (WithHierarchicalNamespace()) {
+ ARROW_UNUSED(CreateFilesystem(container_name));
+ } else {
+ ARROW_UNUSED(CreateContainer(container_name));
+ }
+ arrow::fs::AssertFileInfo(fs(), container_name, FileType::Directory);
+ ASSERT_RAISES(IOError, fs()->DeleteFile(container_name));
+ ASSERT_RAISES(IOError, fs()->DeleteFile(container_name + "/"));
+ }
+
+ void TestDeleteFileAtContainerRoot() {
+ auto data = SetUpPreexistingData();
+
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Path does not exist '" + data.Path("nonexistent-file") +
+ "'"),
+ fs()->DeleteFile(data.Path("nonexistent-file")));
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Not a regular file: '" + data.Path("nonexistent-file/") +
+ "'"),
+ fs()->DeleteFile(data.Path("nonexistent-file/")));
+
+ arrow::fs::AssertFileInfo(fs(), data.ObjectPath(), FileType::File);
+ ASSERT_OK(fs()->DeleteFile(data.ObjectPath()));
+ arrow::fs::AssertFileInfo(fs(), data.ObjectPath(), FileType::NotFound);
+
+ if (WithHierarchicalNamespace()) {
+ auto adlfs_client =
+ datalake_service_client_->GetFileSystemClient(data.container_name);
+ CreateFile(adlfs_client, data.kObjectName, PreexistingData::kLoremIpsum);
+ } else {
+ auto container_client = CreateContainer(data.container_name);
+ CreateBlob(container_client, data.kObjectName, PreexistingData::kLoremIpsum);
+ }
+
+ arrow::fs::AssertFileInfo(fs(), data.ObjectPath(), FileType::File);
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError, ::testing::HasSubstr("Not a regular file: '" + data.ObjectPath() + "/'"),
+ fs()->DeleteFile(data.ObjectPath() + "/"));
+ ASSERT_RAISES(IOError, fs()->DeleteFile(data.ObjectPath() + "/"));
+ }
+
+ void TestDeleteFileAtSubdirectory(bool create_empty_dir_marker_first) {
+ auto data = SetUpPreexistingData();
+
+ auto setup_dir_file0 = [this, create_empty_dir_marker_first, &data]() {
+ if (WithHierarchicalNamespace()) {
+ ASSERT_FALSE(create_empty_dir_marker_first);
+ auto adlfs_client =
+ datalake_service_client_->GetFileSystemClient(data.container_name);
+ CreateFile(adlfs_client, "dir/file0", PreexistingData::kLoremIpsum);
+ } else {
+ auto container_client = CreateContainer(data.container_name);
+ if (create_empty_dir_marker_first) {
+ CreateBlob(container_client, "dir/", "");
+ }
+ CreateBlob(container_client, "dir/file0", PreexistingData::kLoremIpsum);
+ }
+ };
+ setup_dir_file0();
+
+ // Trying to delete a non-existing file in an existing directory should fail
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Path does not exist '" + data.Path("dir/nonexistent-file") +
+ "'"),
+ fs()->DeleteFile(data.Path("dir/nonexistent-file")));
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Not a regular file: '" +
+ data.Path("dir/nonexistent-file/") + "'"),
+ fs()->DeleteFile(data.Path("dir/nonexistent-file/")));
+
+ // Trying to delete the directory with DeleteFile should fail
+ if (WithHierarchicalNamespace()) {
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError, ::testing::HasSubstr("Not a regular file: '" + data.Path("dir") + "'"),
+ fs()->DeleteFile(data.Path("dir")));
+ } else {
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError, ::testing::HasSubstr("Path does not exist '" + data.Path("dir") + "'"),
+ fs()->DeleteFile(data.Path("dir")));
+ }
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError, ::testing::HasSubstr("Not a regular file: '" + data.Path("dir/") + "'"),
+ fs()->DeleteFile(data.Path("dir/")));
+
+ arrow::fs::AssertFileInfo(fs(), data.Path("dir"), FileType::Directory);
+ arrow::fs::AssertFileInfo(fs(), data.Path("dir/"), FileType::Directory);
+ arrow::fs::AssertFileInfo(fs(), data.Path("dir/file0"), FileType::File);
+ ASSERT_OK(fs()->DeleteFile(data.Path("dir/file0")));
+ arrow::fs::AssertFileInfo(fs(), data.Path("dir"), FileType::Directory);
+ arrow::fs::AssertFileInfo(fs(), data.Path("dir/"), FileType::Directory);
+ arrow::fs::AssertFileInfo(fs(), data.Path("dir/file0"), FileType::NotFound);
+
+ // Recreating the file on the same path gurantees leases were properly released/broken
+ setup_dir_file0();
+
+ arrow::fs::AssertFileInfo(fs(), data.Path("dir/file0"), FileType::File);
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ IOError,
+ ::testing::HasSubstr("Not a regular file: '" + data.Path("dir/file0/") + "'"),
+ fs()->DeleteFile(data.Path("dir/file0/")));
Review Comment:
How about swapping these assertions to check `DeleteFile("dir/file0")` while `dir/file0` exists?
```suggestion
EXPECT_RAISES_WITH_MESSAGE_THAT(
IOError,
::testing::HasSubstr("Not a regular file: '" + data.Path("dir/file0/") + "'"),
fs()->DeleteFile(data.Path("dir/file0/")));
arrow::fs::AssertFileInfo(fs(), data.Path("dir/file0"), FileType::File);
```
--
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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #40075:
URL: https://github.com/apache/arrow/pull/40075#discussion_r1491396835
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -2055,6 +2035,116 @@ class AzureFileSystem::Impl {
static constexpr auto kTimeNeededForFileOrDirectoryRename = std::chrono::seconds{3};
public:
+ /// \pre location.container is not empty.
+ /// \pre location.path is not empty.
+ /// \pre location.path does not end with a trailing slash.
+ Status DeleteFileOnFileSystem(const DataLake::DataLakeFileSystemClient& adlfs_client,
+ const AzureLocation& location, bool require_file_to_exist,
+ Azure::Nullable<std::string> lease_id = {}) {
Review Comment:
This was needed for an experiment I did with the API before, but I'm removing this together with the implementation of `MovePaths` for flat-namespace accounts.
--
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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #40075:
URL: https://github.com/apache/arrow/pull/40075#discussion_r1491610498
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -2055,6 +2035,116 @@ class AzureFileSystem::Impl {
static constexpr auto kTimeNeededForFileOrDirectoryRename = std::chrono::seconds{3};
public:
+ /// \pre location.container is not empty.
+ /// \pre location.path is not empty.
+ /// \pre location.path does not end with a trailing slash.
+ Status DeleteFileOnFileSystem(const DataLake::DataLakeFileSystemClient& adlfs_client,
+ const AzureLocation& location, bool require_file_to_exist,
+ Azure::Nullable<std::string> lease_id = {}) {
+ DCHECK(!location.container.empty());
+ DCHECK(!location.path.empty());
+ DCHECK(!internal::HasTrailingSlash(location.path));
+ auto file_client = adlfs_client.GetFileClient(location.path);
+ DataLake::GetPathPropertiesOptions get_options;
+ get_options.AccessConditions.LeaseId = lease_id;
+ DataLake::DeleteFileOptions delete_options;
+ delete_options.AccessConditions.LeaseId = std::move(lease_id);
+ try {
+ // This is necessary to avoid deletion of directories via DeleteFile.
+ auto properties = file_client.GetProperties(get_options);
+ if (properties.Value.IsDirectory) {
+ return internal::NotAFile(location.all);
+ }
+ auto response = file_client.Delete(delete_options);
+ // 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.
+ /// \pre location.path does not end with a trailing slash.
+ 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());
+ DCHECK(!internal::HasTrailingSlash(location.path));
+ constexpr auto kFileBlobLeaseTime = std::chrono::seconds{15};
+
+ // If the parent directory of a file is not the container itself, there is a
+ // risk that deleting the file also deletes the *implied directory* -- the
+ // directory that is implied by the existence of the file path.
+ //
+ // In this case, we must ensure that the deletion is not semantically
+ // equivalent to also deleting the directory. This is done by ensuring the
+ // directory marker blob exists before the file is deleted.
+ std::optional<LeaseGuard> file_blob_lease_guard;
+ const auto parent = location.parent();
+ if (!parent.path.empty()) {
+ // We have to check the existence of the file before checking the
+ // existence of the parent directory marker, so we acquire a lease on the
+ // file first.
+ ARROW_ASSIGN_OR_RAISE(auto file_blob_lease_client,
+ AcquireBlobLease(location, kFileBlobLeaseTime,
+ /*allow_missing=*/!require_file_to_exist));
+ if (file_blob_lease_client) {
+ file_blob_lease_guard.emplace(std::move(file_blob_lease_client),
+ kFileBlobLeaseTime);
+ // Ensure the empty directory marker blob exists before the file is deleted.
+ //
+ // There is not need to hold a lease on the directory marker because if
+ // a concurrent client deletes the directory marker right after we
+ // create it, the file deletion itself won't be the cause of the directory
+ // deletion. Additionally, the fact that a lease is held on the blob path
+ // semantically preserves the directory -- its existence is implied
+ // until the blob representing the file is deleted -- even if another
+ // client deletes the directory marker.
+ RETURN_NOT_OK(EnsureEmptyDirExists(container_client, parent, operation));
+ } else {
+ // File blob doesn't exist and require_file_to_exists is false, so just return OK.
+ // Trying to delete a file that doesn't exist, costs only one request to the
+ // server.
+ DCHECK(!require_file_to_exist);
+ return Status::OK();
+ }
+ }
+
+ auto blob_client = container_client.GetBlobClient(location.path);
+ Blobs::DeleteBlobOptions options;
+ if (file_blob_lease_guard) {
+ options.AccessConditions.LeaseId = file_blob_lease_guard->LeaseId();
+ }
+ try {
+ auto response = blob_client.Delete(options);
Review Comment:
Wow. Could you add the information as a comment of here or `LeaseGuard::BreakBeforeDeletion()`?
--
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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #40075:
URL: https://github.com/apache/arrow/pull/40075#discussion_r1491599653
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -2544,7 +2635,28 @@ Status AzureFileSystem::DeleteRootDirContents() {
Status AzureFileSystem::DeleteFile(const std::string& path) {
ARROW_ASSIGN_OR_RAISE(auto location, AzureLocation::FromString(path));
- return impl_->DeleteFile(location);
+ if (location.container.empty()) {
+ return Status::Invalid("DeleteFile requires a non-empty path.");
+ }
+ if (internal::HasTrailingSlash(location.path) || location.path.empty()) {
+ // Container paths (locations w/o path) or paths ending with a slash are not file
+ // paths.
+ return NotAFile(location);
+ }
Review Comment:
Hmm. It seems that both of `ValidateFileLocation()` and here check `location.container.empty()`, `location.path.empty()` and `HasTrailingSlash()`.
--
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-40074: [C++][FS][Azure] Implement `DeleteFile()` for flat-namespace storage accounts [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #40075:
URL: https://github.com/apache/arrow/pull/40075#discussion_r1492550021
##########
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:
What about `RemoveTrailingSlashFromPath()`?
UPDATE: I added a commit renaming it and the docstring now explains why `all` is always preserved. I don't think there will be any use-case for changing `all`.
--
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