GH-38335: [C++] Implement `GetFileInfo` for a single file in Azure filesystem #38505

Tom-Newton · 2023-10-29T16:15:56Z

Rationale for this change

GetFileInfo is an important part of an Arrow filesystem implementation.

What changes are included in this PR?

Start azurefs_internal similar to GCS and S3 filesystems.
Implement HierarchicalNamespaceDetector.
- This does not use the obvious and simple implementation. It uses a more complicated option inspired by hadoop-azure that avoids requiring the significantly elevated permissions needed for blob_service_client->GetAccountInfo().
- This can't be detected an initialisation time of the filesystem because it requires a container_name. Its packed into its only class so that the result can be cached.
Implement GetFileInfo for single paths.
- Supports hierarchical or flat namespace accounts and takes advantage of hierarchical namespace where possible to avoid unnecessary extra calls to blob storage. The performance difference is actually noticeable just from running the GetFileInfoObjectWithNestedStructure test against real flat and hierarchical accounts. Its about 3 seconds with hierarchical namespace or 5 seconds with a flat namespace.
Update tests with TODO([C++] Implement GetFileInfo for a single file in Azure filesystem #38335) to now use this implementation of GetFileInfo to replace the temporary direct Azure SDK usage.
Rename the main test fixture and introduce new ones for connecting to real blob storage. If details of real blob storage is not provided then the real blob storage tests will be skipped.

Are these changes tested?

Yes. There are new Azurite based tests for everything that can be tested with Azurite.

There are also some tests that are designed to test against a real blob storage account. This is because Azurite cannot emulate a hierarchical namespace account. Additionally some of the behaviour used to detect a hierarchical namespace account is different on Azurite compared to a real flat namespace account. These tests will be automatically skipped unless environment variables are provided with details for connecting to the relevant real storage accounts.

Initially I based the tests on the GCS filesystem but I added a few extras where I thought it was appropriate.

Are there any user-facing changes?

Yes. GetFileInfo is now supported on the Azure filesystem.

Closes: [C++] Implement GetFileInfo for a single file in Azure filesystem #38335

github-actions · 2023-10-29T16:16:27Z

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

Tom-Newton · 2023-11-05T17:40:08Z

cpp/src/arrow/filesystem/azurefs.cc

@@ -78,18 +81,17 @@ struct AzurePath {
          "Expected an Azure object path of the form 'container/path...', got a URI: '",
          s, "'");
    }
-    const auto src = internal::RemoveTrailingSlash(s);


This was preventing GetFileInfo working on directories. The other filesystems did not have this.

Tom-Newton · 2023-11-05T18:08:42Z

cpp/src/arrow/filesystem/azurefs_test.cc

+                 FileType::NotFound);
+
+  AssertFileInfo(fs_.get(), PreexistingContainerPath() + "test-empty-object-dir",
+                 FileType::Directory);


Ideally I would have liked to add an assertion here which confirms that with the hierarchical namespace there are no calls to ListBlobs. That would require patching an Azure container client, which I didn't know how to do. If anyone was any suggestions that would be appreciated.

We can do it by adding internal ListBlobs call counter and exporting it only for testing.
Or we may be able to provide AzureFileSystem::GetStatistics() and the return value provides statistics including the number of ListBlobs calles.

(I think that we don't need test it. If we want to test it, we can open a new issue for it and defer it as a separated task to merge this as soon as possible.)

I think I'm happy to leave out such an assertion at least initially. If it was python I would have done it seems like mocking in C++ would be more complicated even if I did understand the language 😅

cpp/src/arrow/filesystem/azurefs.cc

cpp/src/arrow/filesystem/azurefs_internal.cc

cpp/src/arrow/filesystem/azurefs_internal.h

cpp/src/arrow/filesystem/azurefs_test.cc

Tom-Newton · 2023-11-06T10:27:25Z

Thanks for reviewing kou. I have addressed most of the comments and I should be able to address the remaining ones this evening.

kou

+1

cpp/src/arrow/filesystem/azurefs.cc

kou · 2023-11-08T01:51:12Z

cpp/src/arrow/filesystem/azurefs.cc

  AzureOptions options_;
+  internal::HierarchicalNamespaceDetector hierarchical_namespace_;


It seems that HierarchicalNamespaceDetector is enough simple to move to Impl. (HierarchicalNamespaceDetector::Enabled() is the only important method in the class.)

How about moving HierarchicalNamespaceDetector::Enabled() to Impl::IsHierarchicalNamespaceEnabled() and removing HierarchicalNamespaceDetector (or something)?
If we do it, we can make datalake_service_client_ std::unique_ptr.

I made it separate because I wanted to keep the cached value enabled_ private from the rest of Impl. I was a bit concerned that people might try to directly access the cached state without realising that everything should use the Enabled() function. Additionally making it a separate class made it easier to test.

I think one possibility is to use a non-smart pointer in HierarchicalNamespaceDetector because HierarchicalNamespaceDetector will always be destructed at the same time as Impl. https://stackoverflow.com/questions/7657718/when-to-use-shared-ptr-and-when-to-use-raw-pointers. I think that should allow us to use a unique_ptr for datalake_service_client_. I think this would be my preferred solution. What do you think?

I decided to just make my preferred change. If you think its a bad idea I'm happy to change it again to something else.

OK. Let's use the approach.

kou · 2023-11-08T02:50:28Z

cpp/src/arrow/filesystem/azurefs_test.cc

+                 FileType::NotFound);
+
+  AssertFileInfo(fs_.get(), PreexistingContainerPath() + "test-empty-object-dir",
+                 FileType::Directory);


We can do it by adding internal ListBlobs call counter and exporting it only for testing.
Or we may be able to provide AzureFileSystem::GetStatistics() and the return value provides statistics including the number of ListBlobs calles.

(I think that we don't need test it. If we want to test it, we can open a new issue for it and defer it as a separated task to merge this as soon as possible.)

kou · 2023-11-09T02:11:21Z

cpp/src/arrow/filesystem/azurefs.cc

  AzureOptions options_;
+  internal::HierarchicalNamespaceDetector hierarchical_namespace_;


OK. Let's use the approach.

cpp/src/arrow/filesystem/azurefs.cc

kou · 2023-11-09T02:15:36Z

The lint failure was fixed by #38639.
I'll rebase on main before we merge this.

…space check in the case that the result is cached.

…vided

kou · 2023-11-09T03:56:02Z

I'll merge this.

conbench-apache-arrow · 2023-11-09T07:34:38Z

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 75a0403.

There were no benchmark performance regressions. 🎉

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

…ure filesystem (apache#38505) ### Rationale for this change `GetFileInfo` is an important part of an Arrow filesystem implementation. ### What changes are included in this PR? - Start `azurefs_internal` similar to GCS and S3 filesystems. - Implement `HierarchicalNamespaceDetector`. - This does not use the obvious and simple implementation. It uses a more complicated option inspired by `hadoop-azure` that avoids requiring the significantly elevated permissions needed for `blob_service_client->GetAccountInfo()`. - This can't be detected an initialisation time of the filesystem because it requires a `container_name`. Its packed into its only class so that the result can be cached. - Implement `GetFileInfo` for single paths. - Supports hierarchical or flat namespace accounts and takes advantage of hierarchical namespace where possible to avoid unnecessary extra calls to blob storage. The performance difference is actually noticeable just from running the `GetFileInfoObjectWithNestedStructure` test against real flat and hierarchical accounts. Its about 3 seconds with hierarchical namespace or 5 seconds with a flat namespace. - Update tests with TODO(apacheGH-38335) to now use this implementation of `GetFileInfo` to replace the temporary direct Azure SDK usage. - Rename the main test fixture and introduce new ones for connecting to real blob storage. If details of real blob storage is not provided then the real blob storage tests will be skipped. ### Are these changes tested? Yes. There are new Azurite based tests for everything that can be tested with Azurite. There are also some tests that are designed to test against a real blob storage account. This is because [Azurite cannot emulate a hierarchical namespace account](Azure/Azurite#553). Additionally some of the behaviour used to detect a hierarchical namespace account is different on Azurite compared to a real flat namespace account. These tests will be automatically skipped unless environment variables are provided with details for connecting to the relevant real storage accounts. Initially I based the tests on the GCS filesystem but I added a few extras where I thought it was appropriate. ### Are there any user-facing changes? Yes. `GetFileInfo` is now supported on the Azure filesystem. * Closes: apache#38335 Lead-authored-by: Thomas Newton <thomas.w.newton@gmail.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

github-actions bot added Component: C++ awaiting review Awaiting review labels Oct 29, 2023

Tom-Newton force-pushed the tomnewton/azure_getfileinfo/GH-38335 branch from 544299f to 4142733 Compare November 1, 2023 22:21

Tom-Newton commented Nov 5, 2023

View reviewed changes

Tom-Newton marked this pull request as ready for review November 5, 2023 18:38

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Nov 5, 2023

Tom-Newton mentioned this pull request Nov 5, 2023

[C++] Implement GetFileInfo for a single file in Azure filesystem #38335

Closed

Tom-Newton force-pushed the tomnewton/azure_getfileinfo/GH-38335 branch from 1ab87dc to 28357b0 Compare November 5, 2023 20:33

Tom-Newton mentioned this pull request Nov 6, 2023

[C++] Implement GetFileInfo selector for Azure filesystem #38597

Closed

kou reviewed Nov 6, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting committer review Awaiting committer review awaiting changes Awaiting changes labels Nov 6, 2023

Tom-Newton requested a review from kou November 6, 2023 22:41

Tom-Newton force-pushed the tomnewton/azure_getfileinfo/GH-38335 branch from 42e3d31 to 7fe94f1 Compare November 7, 2023 09:04

kou approved these changes Nov 8, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Nov 8, 2023

kou reviewed Nov 9, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Nov 9, 2023

Tom-Newton added 4 commits November 9, 2023 11:16

Paste in s3 GetFileInfo tests

abf6412

Tests build correctly

80b12ee

Add test for root of storage account

16b6b9a

Partially complete GetFileInfo

fa869ea

Tom-Newton and others added 21 commits November 9, 2023 11:16

Avoid unnecessary creation of filesystem clients for hierachical name…

4c89729

…space check in the case that the result is cached.

Rename test fixtures

218312b

Skip tests against real Azure blob storage if credentials are not pro…

22d64d3

…vided

Spelling

80d0a55

Tidy

d0f3311

Update error messages

8912dbf

Add missing include

3b920f1

Lint

6ab232a

Adjust some more error messges

89320c0

Reduce duplication in tests

e868125

Lint

2cd51c1

Fix clang linker error

6ece0c9

PR comments 1

c562c7e

PR comments 2

2486c86

PR comments: shared_ptr -> unique_ptr

fb03dd8

Adjust handling of missing credentials

c8370b3

Fix namespacing issue after rebase

25a0c76

Auto-formatting

5540c70

PR comments

3fd35c6

Use unique_ptr for datalake_service_client

44f6aa5

Use auto

0659a39

kou force-pushed the tomnewton/azure_getfileinfo/GH-38335 branch from 737d926 to 0659a39 Compare November 9, 2023 02:16

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Nov 9, 2023

kou merged commit 75a0403 into apache:main Nov 9, 2023
32 of 33 checks passed

kou removed the awaiting change review Awaiting change review label Nov 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-38335: [C++] Implement `GetFileInfo` for a single file in Azure filesystem #38505

GH-38335: [C++] Implement `GetFileInfo` for a single file in Azure filesystem #38505

Tom-Newton commented Oct 29, 2023 •

edited

Loading

github-actions bot commented Oct 29, 2023

Tom-Newton Nov 5, 2023

Tom-Newton Nov 5, 2023

kou Nov 8, 2023

Tom-Newton Nov 8, 2023

Tom-Newton commented Nov 6, 2023

kou left a comment

kou Nov 8, 2023

Tom-Newton Nov 8, 2023 •

edited

Loading

Tom-Newton Nov 8, 2023 •

edited

Loading

kou Nov 9, 2023

kou Nov 8, 2023

kou Nov 9, 2023

kou commented Nov 9, 2023

kou commented Nov 9, 2023

conbench-apache-arrow bot commented Nov 9, 2023

		AzureOptions options_;
		internal::HierarchicalNamespaceDetector hierarchical_namespace_;

GH-38335: [C++] Implement GetFileInfo for a single file in Azure filesystem #38505

GH-38335: [C++] Implement GetFileInfo for a single file in Azure filesystem #38505

Conversation

Tom-Newton commented Oct 29, 2023 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Oct 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tom-Newton commented Nov 6, 2023

kou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tom-Newton Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Tom-Newton Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kou commented Nov 9, 2023

kou commented Nov 9, 2023

conbench-apache-arrow bot commented Nov 9, 2023

GH-38335: [C++] Implement `GetFileInfo` for a single file in Azure filesystem #38505

GH-38335: [C++] Implement `GetFileInfo` for a single file in Azure filesystem #38505

Tom-Newton commented Oct 29, 2023 •

edited

Loading

Tom-Newton Nov 8, 2023 •

edited

Loading

Tom-Newton Nov 8, 2023 •

edited

Loading