GH-41034: [C++][FS][Azure] Adjust DeleteDir/DeleteDirContents/GetFileInfoSelector behaviors against Azure for generic filesystem tests

[kou]

Rationale for this change

They are failing:

[  FAILED  ] TestAzureHierarchicalNSGeneric.DeleteDir
[  FAILED  ] TestAzureHierarchicalNSGeneric.DeleteDirContents
[  FAILED  ] TestAzureHierarchicalNSGeneric.GetFileInfoSelector

What changes are included in this PR?

• DeleteDir(): Check not a directory case
• DeleteDirContents(): Check not a directory case
• GetFileInfoSelector():
  • Add not a directory check for input
  • Add support for returning metadata for directory

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

• GitHub Issue: [C++][FS][Azure] Generic filesystem tests for DeleteDir/DeleteDirContents/GetFileInfoSelector failed with Azure #41034

[github-actions]

⚠️ GitHub issue #41034 has been automatically assigned in GitHub to PR creator.

[kou]

@felipecrv It seems that the current CopyFile implementation doesn't work with Azure hierarchical namespace support. I'll split it to a separated issue.

[felipecrv]

This fix is very confusing to me. This function is GetFileInfoWithSelectorFromContainer, it's meant to be called when HNS support has been detected as disabled.

The right fix (IMO) is to create GetFileInfoWithSelectorFromFileSystem(adlfs_client, ...) and then in GetFileInfoWithSelector dispatch to the different implementations according to the HNS support detection.

[kou]

I can understand what you say but the current implementation doesn't:

GetFileInfoWithSelector() isn't dispatched based on HNS support:

arrow/cpp/src/arrow/filesystem/azurefs.cc

Lines 2859 to 2866 in cd607d0

	Result<FileInfoVector> AzureFileSystem::GetFileInfo(const FileSelector& select) {
	Core::Context context;
	Azure::Nullable<int32_t> page_size_hint; // unspecified
	FileInfoVector results;
	RETURN_NOT_OK(
	impl_->GetFileInfoWithSelector(context, page_size_hint, select, &results));
	return {std::move(results)};
	}

arrow/cpp/src/arrow/filesystem/azurefs.cc

Lines 1740 to 1779 in cd607d0

	Status GetFileInfoWithSelector(const Core::Context& context,
	Azure::Nullable<int32_t> page_size_hint,
	const FileSelector& select,
	FileInfoVector* acc_results) {
	ARROW_ASSIGN_OR_RAISE(auto base_location, AzureLocation::FromString(select.base_dir));
	if (base_location.container.empty()) {
	// Without a container, the base_location is equivalent to the filesystem
	// root -- `/`. FileSelector::allow_not_found doesn't matter in this case
	// because the root always exists.
	auto on_container = [&](const Blobs::Models::BlobContainerItem& container) {
	// Deleted containers are not listed by ListContainers.
	DCHECK(!container.IsDeleted);
	// Every container is considered a directory.
	FileInfo info{container.Name, FileType::Directory};
	info.set_mtime(
	std::chrono::system_clock::time_point{container.Details.LastModified});
	acc_results->push_back(std::move(info));
	// Recurse into containers (subdirectories) if requested.
	if (select.recursive && select.max_recursion > 0) {
	FileSelector sub_select;
	sub_select.base_dir = container.Name;
	sub_select.allow_not_found = true;
	sub_select.recursive = true;
	sub_select.max_recursion = select.max_recursion - 1;
	ARROW_RETURN_NOT_OK(
	GetFileInfoWithSelector(context, page_size_hint, sub_select, acc_results));
	}
	return Status::OK();
	};
	return VisitContainers(context, std::move(on_container));
	}
	auto container_client =
	blob_service_client_->GetBlobContainerClient(base_location.container);
	return GetFileInfoWithSelectorFromContainer(container_client, context, page_size_hint,
	select, acc_results);
	}

The FromContainer suffix doesn't mean only for no HNS support. We use OnContainer/OnFileSystem suffix for it. FromContainer just means that it's for one container and it doesn't work with multiple containers.

---

Should we create GetFileInfoWithSelectorFromContainerOn{Container,FileSystem}()? But their names are too long...

[felipecrv]

I can understand what you say but the current implementation doesn't:

GetFileInfoWithSelector() isn't dispatched based on HNS support:

But what I meant was changing/fixing GetFileInfoWithSelector to do that.

Should we create GetFileInfoWithSelectorFromContainerOn{Container,FileSystem}()? But their names are too long...

ADLFS calls "filesystem" what "blobs" calls "container", so you replace the last word of the name:

  Status GetFileInfoWithSelectorFromContainer(const Blobs::BlobContainerClient& container_client, ...);
  Status GetFileInfoWithSelecitonFromFilesystem(const DataLake::DataLakeFileSystemClient& adlfs_client, ...);

[kou]

ADLFS calls "filesystem" what "blobs" calls "container"

Wow! I didn't notice it!

[kou]

Implemented.

[felipecrv]

The fact that you check existence above doesn't mean these errors can't happen down here.

To use terms that @pitrou once shared with me [1], you're moving from EAFP [2] "easier to ask for permission" to LBYL [3] "look before you leap".

From [3]:

LBYL

Look before you leap. This coding style explicitly tests for pre-conditions before making calls or lookups. This style contrasts with the EAFP approach and is characterized by the presence of many if statements.

In a multi-threaded environment, the LBYL approach can risk introducing a race condition between “the looking” and “the leaping”. For example, the code, if key in mapping: return mapping[key] can fail if another thread removes key from mapping after the test, but before the lookup. This issue can be solved with locks or by using the EAFP approach.

[1] https://devblogs.microsoft.com/python/idiomatic-python-eafp-versus-lbyl/
[2] https://docs.python.org/3.5/glossary.html#term-eafp
[3] https://docs.python.org/3.5/glossary.html#term-lbyl

[felipecrv]

Given the TZ differences, I will give myself a chance of fixing these bugs to see if I can come up with something that doesn't introduce pessimistic round trips to the backend.

[kou]

OK. I'll revert this for now.
You can introduce better approach later if you can find it.

FYI: We want to report a NotADir() for file location. But DeleteRecursive()/DeleteEmpty() can delete both of file and directory. See also: https://learn.microsoft.com/en-us/rest/api/storageservices/datalakestoragegen2/path/delete?view=rest-storageservices-datalakestoragegen2-2019-12-12

Delete the file or directory.

arrow/cpp/src/arrow/filesystem/test_util.cc

Lines 274 to 276 in cd607d0

	// Not a directory
	CreateFile(fs, "AB/def", "");
	ASSERT_RAISES(IOError, fs->DeleteDir("AB/def"));

is a test for this case.

[kou]

Can we block changing the target location from other connections by using lease_id for GetFileInfo() too?

diff --git a/cpp/src/arrow/filesystem/azurefs.cc b/cpp/src/arrow/filesystem/azurefs.cc
index df6db3d959..67232b3156 100644
--- a/cpp/src/arrow/filesystem/azurefs.cc
+++ b/cpp/src/arrow/filesystem/azurefs.cc
@@ -2181,7 +2181,7 @@ class AzureFileSystem::Impl {
                                Azure::Nullable<std::string> lease_id = {}) {
     DCHECK(!location.container.empty());
     DCHECK(!location.path.empty());
-    ARROW_ASSIGN_OR_RAISE(auto file_info, GetFileInfo(adlfs_client, location));
+    ARROW_ASSIGN_OR_RAISE(auto file_info, GetFileInfo(adlfs_client, location, lease_id));
     if (file_info.type() == FileType::NotFound) {
       if (require_dir_to_exist) {
         return PathNotFound(location);

[felipecrv]

But DeleteRecursive()/DeleteEmpty() can delete both of file and directory. See also: https://learn.microsoft.com/en-us/rest/api/storageservices/datalakestoragegen2/path/delete?view=rest-storageservices-datalakestoragegen2-2019-12-12

This scenario sounds familiar. 🤔 Does that happen even if you append a trailing slash to the path?

[kou]

If we add a trailing slash, API returns an error...:

'fs->DeleteDir("AB/CD")' failed with IOError: Failed to delete a directory: AB/CD: https://clearcodearrow.blob.core.windows.net/aeouspyxzsz16iliz71ya6yt5qrur6b9/AB/CD/ Azure Error: [InvalidUri] 400 The request URI is invalid.

[kou]

@felipecrv Do you want to review this before we merge this? I want to add this to 16.0.0.

[felipecrv]

Thank you! This looks much more straightforward now.

[kou]

Thanks! I'll merge this.

[conbench-apache-arrow]

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

There were no benchmark performance regressions. 🎉

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