GH-38333: [C++][FS][Azure] Implement file writes #38780

Tom-Newton · 2023-11-19T01:43:32Z

Rationale for this change

Writing files is an important part of the filesystem

What changes are included in this PR?

Implements OpenOutputStream and OpenAppendStream for Azure.

Initially I started with the implementation from ARROW-2034: [C++] Filesystem implementation for Azure Blob Storage #12914 but I made quite a few changes:
- Removed the different code path for hierarchical namespace accounts. There should not be any performance advantage to using special APIs only available on hierachical namespace accounts.
- Only implement ObjectAppendStream, not ObjectOutputStream. OpenOutputStream is implemented by truncating the existing file then returning a ObjectAppendStream.
- More precise use of try catch. Every call to Azure is wrapped in a try catch and should return a descriptive error status.
- Avoid unnecessary calls to Azure. For example we now maintain the block list in memory and commit it only once on flush. ARROW-2034: [C++] Filesystem implementation for Azure Blob Storage #12914 committed the block list after each block that was staged and on flush queried Azure to get the list of uncommitted blocks. The new approach is consistent with the Azure fsspec implementation https://github.com/fsspec/adlfs/blob/092685f102c5cd215550d10e8347e5bce0e2b93d/adlfs/spec.py#L2009
- Adjust the block_ids slightly to minimise the risk of them conflicting with blocks written by other blob storage clients.
- Implement metadata writes. Includes adding default metadata to AzureOptions.
Tests are based on the gscfs_test.cc but I added a couple of extra.
Handle the TODO(GH-38333: [C++][FS][Azure] Implement file writes #38780) comments for using the Azure fs to write data in tests

Are these changes tested?

Yes. Everything should be covered by azurite tests

Are there any user-facing changes?

Yes. The Azure filesystem now supports file writes.

Closes: [C++] Implement file writes for Azure filesystem #38333

Tom-Newton · 2023-11-19T01:45:16Z

I think this is ready for review apart from its currently based on top of #38773. Once that PR merges the diff will be more readable.

kou · 2023-11-19T01:55:49Z

OK. I'll merge #38773 right now. Please rebase on main.

kou

(My review isn't completed yet. I'll continue later.)

cpp/src/arrow/filesystem/azurefs.cc

kou · 2023-11-19T06:59:56Z

cpp/src/arrow/filesystem/azurefs.cc

+                               // blocks created by other applications.
+    new_block_id = Azure::Core::Convert::Base64Encode(
+        std::vector<uint8_t>(new_block_id.begin(), new_block_id.end()));
+


Do we need to check whether the new_block_id exists in block_ids_ or not?

If we want to be 100% confident of avoiding clashes then yes but personally I think the current solution is a good compromise.

The risk should be zero when using OpenOutputStream because every block ID will be created by this same scheme, using monotonically increasing integers. The risk when using OpenAppendStream is that previously committed blocks used unusual names that might conflict. For example if some other writer committed one block named 00002-arrow then that would conflict after this writer appends 2 additional blocks, and cause a corrupt blob. I think this is extremely unlikely so personally I think this is a good option. Additionally OpenAppendStream is not implemented at all for S3 and GCS so presumably its not used much.

OK. Could you describe the risk as a comment? If we find a real world problem with the risk, we can revisit the risk.

cpp/src/arrow/filesystem/azurefs.cc

kou · 2023-11-19T07:03:04Z

cpp/src/arrow/filesystem/azurefs.cc

+    if (metadata && metadata->size() != 0) {
+      metadata_ = ArrowMetadataToAzureMetadata(metadata);
+    } else if (options.default_metadata && options.default_metadata->size() != 0) {
+      metadata_ = ArrowMetadataToAzureMetadata(options.default_metadata);
+    }


Do these metadata replace the existing metadata? Should we merge with the existing metadata?

Closing/flushing an append stream will always completely replace the old metadata. This is covered by the AzuriteFileSystemTest, TestWriteMetadata test which I largely copied from gcsfs_test.cc.

I don't feel strongly but I think this is a reasonable choice. I think if it did merge then there would be no way to remove metadata keys through the arrow file system. Also replacing is simpler to implement than merging.

kou

+1

cpp/src/arrow/filesystem/azurefs_test.cc

cpp/src/arrow/filesystem/azurefs.cc

cpp/src/arrow/filesystem/azurefs_test.cc

conbench-apache-arrow · 2023-11-21T07:23:23Z

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

There were no benchmark performance regressions. 🎉

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

### Rationale for this change Writing files is an important part of the filesystem ### What changes are included in this PR? Implements `OpenOutputStream` and `OpenAppendStream` for Azure. - Initially I started with the implementation from apache#12914 but I made quite a few changes: - Removed the different code path for hierarchical namespace accounts. There should not be any performance advantage to using special APIs only available on hierachical namespace accounts. - Only implement `ObjectAppendStream`, not `ObjectOutputStream`. `OpenOutputStream` is implemented by truncating the existing file then returning a `ObjectAppendStream`. - More precise use of `try` `catch`. Every call to Azure is wrapped in a `try` `catch` and should return a descriptive error status. - Avoid unnecessary calls to Azure. For example we now maintain the block list in memory and commit it only once on flush. apache#12914 committed the block list after each block that was staged and on flush queried Azure to get the list of uncommitted blocks. The new approach is consistent with the Azure fsspec implementation https://github.com/fsspec/adlfs/blob/092685f102c5cd215550d10e8347e5bce0e2b93d/adlfs/spec.py#L2009 - Adjust the block_ids slightly to minimise the risk of them conflicting with blocks written by other blob storage clients. - Implement metadata writes. Includes adding default metadata to `AzureOptions`. - Tests are based on the `gscfs_test.cc` but I added a couple of extra. - Handle the TODO(apacheGH-38780) comments for using the Azure fs to write data in tests ### Are these changes tested? Yes. Everything should be covered by azurite tests ### Are there any user-facing changes? Yes. The Azure filesystem now supports file writes. * Closes: apache#38333 Lead-authored-by: Thomas Newton <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

github-actions bot added the Component: C++ label Nov 19, 2023

Tom-Newton changed the title ~~GH-38333: [C++][Azure] Implement file writes~~ GH-38333: [C++][FS][Azure] Implement file writes Nov 19, 2023

github-actions bot added the awaiting review Awaiting review label Nov 19, 2023

Tom-Newton force-pushed the tomnewton/implement_azure_file_writes/GH-38333 branch from 480be61 to ed29906 Compare November 19, 2023 02:02

Tom-Newton marked this pull request as ready for review November 19, 2023 02:24

kou reviewed Nov 19, 2023

View reviewed changes

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

Tom-Newton added 16 commits November 19, 2023 13:22

Paste in object append stream from apache#12914

89899b4

Delete hierarchical namespace code paths

12c1978

Paste in tests from gcsfs_test.cc

bd787cc

Mostly correct tests

cb9879b

Paste in OpenOutputStream and OpenAppendStream from apache#12914

bbaf138

Tests pass

2f27fbb

Better error handling and tidy

bfeb57d

Implement append and output

d4c53b2

Fix rebase

a1a8c15

Add tests to distinguish OpenAppendStream and OpenOutputStream

0b0a2ee

More precise error handling on calls to Azure blob storage

c69f188

Avoid unnecessary extra block list fetching and committing

5a91113

Adjust block_ids and add some comments

1c4300b

Add test for writing blob metadata

a62b95e

Implement metadata writes

28ec1ed

Add simple sanity checks on the location

2800dea

Tom-Newton added 4 commits November 19, 2023 13:22

Tidy

9a98a0f

PR comments 1

d86c374

PR comments2: move DoAppend to private

77f8345

Autoformat

0178660

Tom-Newton force-pushed the tomnewton/implement_azure_file_writes/GH-38333 branch from b04e14c to 0178660 Compare November 19, 2023 13:22

Handle TODO(apacheGH-38780) comments for using fs to write data in tests

7b19feb

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

kou approved these changes Nov 20, 2023

View reviewed changes

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

Tom-Newton added 4 commits November 20, 2023 09:31

PR comments: tests tidy up

74a6d55

Add a comment about the risk of overlapping block ids

9a0d9ca

Lint

0e84a82

Lint

ad0ed8f

kou reviewed Nov 21, 2023

View reviewed changes

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

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

cpp/src/arrow/filesystem/azurefs_test.cc Outdated Show resolved Hide resolved

Simplify

7412b90

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

kou merged commit c1b12ca into apache:main Nov 21, 2023
32 of 34 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-38333: [C++][FS][Azure] Implement file writes #38780

GH-38333: [C++][FS][Azure] Implement file writes #38780

Tom-Newton commented Nov 19, 2023 •

edited

Loading

Tom-Newton commented Nov 19, 2023 •

edited

Loading

kou commented Nov 19, 2023

kou left a comment

kou Nov 19, 2023

Tom-Newton Nov 19, 2023 •

edited

Loading

kou Nov 20, 2023

Tom-Newton Nov 20, 2023

kou Nov 19, 2023

Tom-Newton Nov 19, 2023 •

edited

Loading

kou Nov 20, 2023

kou left a comment

conbench-apache-arrow bot commented Nov 21, 2023

GH-38333: [C++][FS][Azure] Implement file writes #38780

GH-38333: [C++][FS][Azure] Implement file writes #38780

Conversation

Tom-Newton commented Nov 19, 2023 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Tom-Newton commented Nov 19, 2023 • edited Loading

kou commented Nov 19, 2023

kou left a comment

Choose a reason for hiding this comment

kou Nov 19, 2023

Choose a reason for hiding this comment

Tom-Newton Nov 19, 2023 • edited Loading

Choose a reason for hiding this comment

kou Nov 20, 2023

Choose a reason for hiding this comment

Tom-Newton Nov 20, 2023

Choose a reason for hiding this comment

kou Nov 19, 2023

Choose a reason for hiding this comment

Tom-Newton Nov 19, 2023 • edited Loading

Choose a reason for hiding this comment

kou Nov 20, 2023

Choose a reason for hiding this comment

kou left a comment

Choose a reason for hiding this comment

conbench-apache-arrow bot commented Nov 21, 2023

Tom-Newton commented Nov 19, 2023 •

edited

Loading

Tom-Newton commented Nov 19, 2023 •

edited

Loading

Tom-Newton Nov 19, 2023 •

edited

Loading

Tom-Newton Nov 19, 2023 •

edited

Loading