GH-38772: [C++] Implement directory semantics even when the storage account doesn't support HNS #39361

felipecrv · 2023-12-24T01:25:22Z

Rationale for this change

The FileSystem implementation based on Azure Blob Storage should implement directory operations according to filesystem semantics. When Hierarchical Namespace (HNS) is enabled, we can rely on Azure Data Lake Storage Gen 2 APIs implementing the filesystem semantics for us, but when all we have is the Blobs API, we should emulate it.

What changes are included in this PR?

Skip fewer tests
Re-implement GetFileInfo using ListBlobsByHierarchy instead of ListBlobs
Re-implement CreateDir with an upfront HNS support check instead of falling back to Blobs API after an error
Add comprehensive tests to CreateDir
Add HasSubmitBatchBug to check if a test inside any scenario is affected by a certain Azurite issue
Implement DeleteDir to work properly on flat namespace storage accounts (non-HNS accounts)

Are these changes tested?

Yes. By existing and new tests added by this PR itself.

Closes: [C++][FS][Azure] Should GetFileInfo() against a directory always return true without hierarchical namespace support? #38772

…n flat ns accounts

… be duplicates

This will fit better together with another boolean I'm adding to DeleteDirContentsOnContainer.

felipecrv · 2023-12-24T01:28:01Z

@Tom-Newton

Tom-Newton

I have not reviewed everything yet. I have a few comments, but generally I think it looks great so far.

cpp/src/arrow/filesystem/azurefs.cc

Tom-Newton · 2023-12-24T13:54:58Z

cpp/src/arrow/filesystem/azurefs.cc

+          auto dir_marker_blob_path = internal::EnsureTrailingSlash(location.path);
+          return container_client.GetBlobClient(dir_marker_blob_path).AsBlockBlobClient();
+        },
+        [](auto& block_blob_client) { block_blob_client.UploadFrom(nullptr, 0); },


I think it would be a good idea to put some metadata on all directory marker blobs we create. That would allow us to add a simple checks in ObjectInputFile::Init and ObjectAppendStream::Init to ensure they are not being initialised as directories.

Additionally I think this will not be compatible with adlfs if it does not add metadata to directory markers. I think its very likely that people will end up using a combination of this arrow filesystem and adlfs.

Sure. I would like to be as compatible as possible with existing specs. But to clarify, what do you mean when you say adlfs? Don't you mean abfs instead [1]?

[1] https://github.com/apache/hadoop/blob/trunk/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystem.java

I think there is a confusion between abfs (filesystem semantics over the Blobs API) and adlfs (the separate service that Azure created that can also be accessed via the Blobs API with some restrictions).

The fsspec repo seems to be linking both abfs and adlfs to the same thing, but they shouldn't be considered the same thing: https://github.com/fsspec/filesystem_spec/blob/0ffe06cb767456b7c13904b57ec1c3ca60d53eae/docs/source/api.rst?plain=1#L229

Tell me what I'm missing here because my experience with Azure is restricted just to reading the docs (a lot of them) recently.

Sorry, in this case I mean the adlfs pip repo. It's the Azure fsspec implementation.

More specifically I think we should add some metadata which will be compatible such that https://github.com/fsspec/adlfs/blob/32132c4094350fca2680155a5c236f2e9f991ba5/adlfs/spec.py#L855-L870 can correctly detect the directories.

I forgot this, but I will add this change now.

cpp/src/arrow/filesystem/azurefs_test.cc

cpp/src/arrow/filesystem/azurefs.cc

felipecrv · 2024-01-04T20:27:40Z

@Tom-Newton @kou I pushed changes based on your feedback. I would love to merge it before tomorrow's release cut.

kou · 2024-01-04T21:37:45Z

Could you check CI failures?

https://github.com/apache/arrow/actions/runs/7414494286/job/20175680211?pr=39361#step:6:3814

[ RUN      ] TestAzuriteFileSystem.DeleteDirContentsSuccessDirectory
/arrow/cpp/src/arrow/filesystem/test_util.cc:143: Failure
Expected equality of these values:
  info.type()
    Which is: FileType::Directory
  type
    Which is: FileType::NotFound
For path 'hfqilxw9uhk7xqbathiwbudo4ci9dp9w/0iav1y3m4u53d5wvtp8g2j5af1o4s694'

https://github.com/apache/arrow/actions/runs/7414494286/job/20175680211?pr=39361#step:6:3680

[ RUN      ] TestAzuriteFileSystem.DeleteDirSuccessNonexistent
/arrow/cpp/src/arrow/filesystem/azurefs_test.cc:1301: Failure
Failed
'fs()->DeleteDir(directory_path)' failed with IOError: Path does not exist 'go9qhglaypjhxnprrxc79a0e3xtceml1/bwumjdfql0n8hizvv1kd3jn75ucrlw4u'. Detail: [errno 2] No such file or directory

felipecrv · 2024-01-04T22:26:53Z

@kou done. These turned out to be Azurite-only tests that needed updating since the semantics are now different than what was assumed when the tests were written.

kou

+1

conbench-apache-arrow · 2024-01-05T23:34:50Z

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

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

…rage account doesn't support HNS (apache#39361) ### Rationale for this change The `FileSystem` implementation based on Azure Blob Storage should implement directory operations according to filesystem semantics. When Hierarchical Namespace (HNS) is enabled, we can rely on Azure Data Lake Storage Gen 2 APIs implementing the filesystem semantics for us, but when all we have is the Blobs API, we should emulate it. ### What changes are included in this PR? - Skip fewer tests - Re-implement `GetFileInfo` using `ListBlobsByHierarchy` instead of `ListBlobs` - Re-implement `CreateDir` with an upfront HNS support check instead of falling back to Blobs API after an error - Add comprehensive tests to `CreateDir` - Add `HasSubmitBatchBug` to check if a test inside any scenario is affected by a certain Azurite issue - Implement `DeleteDir` to work properly on flat namespace storage accounts (non-HNS accounts) - ### Are these changes tested? Yes. By existing and new tests added by this PR itself. * Closes: apache#38772 Authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>

felipecrv added 14 commits December 23, 2023 22:01

FIX: Remove GTEST_SKIP() that would skip the second call with (false)

682c885

Implement GetFileInfo with upfront HNS checking

899a724

Implement CreateDir with upfront HNS checks and directory semantics o…

edc8035

…n flat ns accounts

Add comprehensive tests to CreateDir and delete tiny tests that would…

e7a36e1

… be duplicates

Add HasSubmitBatchBug() to TestAzureFileSystem

045eb4d

Move all the HNS-only tests to all scenarios

6e03984

Fix DeleteDir bugs on Flat namespace storage accounts

715781a

Split HNS and non-HNS code for the implementation of DeleteDir

e9ea375

Extract DeleteDirContentsOnFileSystem from DeleteDirContents

2748798

Flip name and meaning of boolean passed to DeleteDirContentsOnContainer

e9a1705

This will fit better together with another boolean I'm adding to DeleteDirContentsOnContainer.

Add a local type alias for convenience

ca28c0f

Simplify the use of DeleteDirContentsOnContainer by adding a bool param

ee12b7c

Document a pre-condition of DeleteDirContentsOnContainer

59e113d

Declare all client parameters as const &

7b663b9

github-actions bot added Component: C++ awaiting review Awaiting review labels Dec 24, 2023

felipecrv requested review from kou and pitrou December 24, 2023 01:27

Tom-Newton reviewed Dec 24, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Dec 24, 2023

felipecrv added 3 commits December 26, 2023 12:59

Fix typo

8518476

Only call GetAzureEnv if env is used

2a64996

Document the interpretation of the ListBlobByHierarchy response

de2a768

kou reviewed Jan 1, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jan 1, 2024

Tom-Newton reviewed Jan 2, 2024

View reviewed changes

cpp/src/arrow/filesystem/azurefs.cc Show resolved Hide resolved

Add more information to some error messages

f091ad5

felipecrv added 4 commits January 4, 2024 16:58

Remove GetDirectoryClient tparam from CreateDirTemplate

2e4d4e0

Extract EnsureEmptyDirExistsImplThatThrows and use it in two places

5203071

Fix typo

8e2e9ff

Nest second kContainerNotFound check

9957577

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jan 4, 2024

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jan 4, 2024

Attach metadata to directory marker blobs

96bdba2

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jan 4, 2024

Fix test-code that doesn't match the expectations

432dd61

felipecrv requested a review from kou January 4, 2024 22:27

felipecrv added this to the 15.0.0 milestone Jan 4, 2024

felipecrv requested a review from Tom-Newton January 5, 2024 01:36

kou approved these changes Jan 5, 2024

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jan 5, 2024

felipecrv merged commit aae6fa4 into apache:main Jan 5, 2024
33 checks passed

felipecrv removed the awaiting merge Awaiting merge label Jan 5, 2024

felipecrv deleted the azure_dir_semantics branch January 5, 2024 15:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-38772: [C++] Implement directory semantics even when the storage account doesn't support HNS #39361

GH-38772: [C++] Implement directory semantics even when the storage account doesn't support HNS #39361

felipecrv commented Dec 24, 2023 •

edited by github-actions bot

felipecrv commented Dec 24, 2023

Tom-Newton left a comment •

edited

Tom-Newton Dec 24, 2023

felipecrv Dec 26, 2023

Tom-Newton Dec 26, 2023 •

edited

Tom-Newton Jan 2, 2024 •

edited

felipecrv Jan 4, 2024

felipecrv commented Jan 4, 2024

kou commented Jan 4, 2024

felipecrv commented Jan 4, 2024

kou left a comment

conbench-apache-arrow bot commented Jan 5, 2024

GH-38772: [C++] Implement directory semantics even when the storage account doesn't support HNS #39361

GH-38772: [C++] Implement directory semantics even when the storage account doesn't support HNS #39361

Conversation

felipecrv commented Dec 24, 2023 • edited by github-actions bot

Rationale for this change

What changes are included in this PR?

Are these changes tested?

felipecrv commented Dec 24, 2023

Tom-Newton left a comment • edited

Choose a reason for hiding this comment

Tom-Newton Dec 24, 2023

Choose a reason for hiding this comment

felipecrv Dec 26, 2023

Choose a reason for hiding this comment

Tom-Newton Dec 26, 2023 • edited

Choose a reason for hiding this comment

Tom-Newton Jan 2, 2024 • edited

Choose a reason for hiding this comment

felipecrv Jan 4, 2024

Choose a reason for hiding this comment

felipecrv commented Jan 4, 2024

kou commented Jan 4, 2024

felipecrv commented Jan 4, 2024

kou left a comment

Choose a reason for hiding this comment

conbench-apache-arrow bot commented Jan 5, 2024

felipecrv commented Dec 24, 2023 •

edited by github-actions bot

Tom-Newton left a comment •

edited

Tom-Newton Dec 26, 2023 •

edited

Tom-Newton Jan 2, 2024 •

edited