GH-39119: [C++] Refactor the Azure FS tests and filesystem class instantiation by felipecrv · Pull Request #39207 · apache/arrow

felipecrv · 2023-12-13T02:09:27Z

Rationale for this change

This PR contains some unrelated improvements (like better docs) and some nitpicky fixes. The test refactoring makes it easier to see on which environments tests run and make them able to be instantiated with different options in the future once we extend the AzureOptions.

What changes are included in this PR?

Random cleanups
Short namespace aliases
Refactoring of the tests (multiple environments, TYPED_TEST_SUITE, explicit preexisting data setup)
Cleanup of the AzureOptions class

Are these changes tested?

Yes. I created Azure Storage accounts to test with and without Hierarchical Namespace support. I also ran the tests in a shell without my environment variables to ensure the test-skipping behavior is correct.

Are there any user-facing changes?

Changes to AzureOptions, but since AzureFileSystem is not really used yet, these breaking changes won't be a problem.

Closes: [C++][FS][Azure] Make mutations of test data explicit in azurefs_test.cc #39119

github-actions · 2023-12-13T02:09:55Z

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

felipecrv · 2023-12-13T02:11:24Z

@kou @Tom-Newton @av8or1

felipecrv · 2023-12-13T02:16:30Z

cpp/src/arrow/filesystem/azurefs_test.cc

No more awkward status_ being set on ctor now that Make is used to construct the environments.

felipecrv · 2023-12-13T02:18:47Z

cpp/src/arrow/filesystem/azurefs_test.cc

@kou I'm explicitly checking that the Status::IsCancelled() to make sure other failures don't make the tests be skipped quietly.

cpp/src/arrow/filesystem/azurefs_test.cc

kou

+1

I couldn't review all changes carefully because there are too many changes. But I trust you. :-)

cpp/src/arrow/filesystem/azurefs.cc

cpp/src/arrow/filesystem/azurefs_test.cc

cpp/src/arrow/filesystem/azurefs.cc

cpp/src/arrow/filesystem/azurefs.h

pitrou · 2023-12-13T17:40:23Z

cpp/src/arrow/filesystem/azurefs.h

Do we plan to autodetect the backend at some point? We do that in S3 (the DetectS3Backend function).

Now that I have the environments, I think I can remove this option completely. 🤔

Now that I have the environments, I think I can remove this option completely. 🤔

Do you plan to do that here or in a separate PR?

I tried to do on this one, but this is used to configure the URLs in AzureOptions, so I will do it in another PR where I'm adding more options to AzureOptions and documenting it better.

cpp/src/arrow/filesystem/azurefs.h

cpp/src/arrow/filesystem/azurefs_test.cc

pitrou · 2023-12-14T17:05:00Z

The CI failures look unexpected, can you perhaps rebase on main?

…ad of ConcatAbstractPath where applicable

pitrou

+1. Let's see if tests are ok now.

conbench-apache-arrow · 2023-12-15T03:05:21Z

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

There was 1 benchmark result indicating a performance regression:

Commit Run on ursa-i9-9960x at 2023-12-14 19:15:38Z
- csv-read (Python) with compression=uncompressed, dataset=nyctaxi_2010-01, output_format=arrow_table, streaming=file

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.

…s instantiation (apache#39207) ### Rationale for this change This PR contains some unrelated improvements (like better docs) and some nitpicky fixes. The test refactoring makes it easier to see on which environments tests run and make them able to be instantiated with different options in the future once we extend the `AzureOptions`. ### What changes are included in this PR? - Random cleanups - Short namespace aliases - Refactoring of the tests (multiple environments, TYPED_TEST_SUITE, explicit preexisting data setup) - Cleanup of the `AzureOptions` class ### Are these changes tested? Yes. I created Azure Storage accounts to test with and without Hierarchical Namespace support. I also ran the tests in a shell without my environment variables to ensure the test-skipping behavior is correct. ### Are there any user-facing changes? Changes to `AzureOptions`, but since `AzureFileSystem` is not really used yet, these breaking changes won't be a problem. * Closes: apache#39119 Authored-by: Felipe Oliveira Carvalho <[email protected]> Signed-off-by: Felipe Oliveira Carvalho <[email protected]>

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

felipecrv force-pushed the azurefs_tests branch from 1d96443 to 27eae54 Compare December 13, 2023 02:14

felipecrv commented Dec 13, 2023

View reviewed changes

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

felipecrv commented Dec 13, 2023

View reviewed changes

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

felipecrv commented Dec 13, 2023

View reviewed changes

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

felipecrv commented Dec 13, 2023

View reviewed changes

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

felipecrv commented Dec 13, 2023

View reviewed changes

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

felipecrv requested a review from kou December 13, 2023 02:24

kou approved these changes Dec 13, 2023

View reviewed changes

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

pitrou self-requested a review December 13, 2023 17:23

pitrou changed the title ~~GH-39119 [C++]: Refactor the Azure FS tests~~ GH-39119: [C++] Refactor the Azure FS tests Dec 13, 2023

pitrou reviewed Dec 13, 2023

View reviewed changes

felipecrv requested a review from pitrou December 14, 2023 00:34

felipecrv added 8 commits December 14, 2023 14:28

Introduce BodyTextView() and IsContainerNotFound()

c8a0d8b

Explain better what ADLS Gen is and how it interacts with Blobs Storage

b0ba6ae

Change AzureOptions towards how it should work

2a13efc

Use namespace aliases to shorten the references to Azure:: types

fe9a923

Refactor the Azure FS tests

aa48525

azurefs_test.cc: Use TYPED_TEST_SUITE to generate tests for all envs

0a8bf1a

Make PreexistingData setup explicit

79145a5

fixup! Refactor the Azure FS tests

551f11c

felipecrv added 18 commits December 14, 2023 14:28

Explain all the factories for the Azure envs

907c2f7

Use instead of size_t

0e827a4

Remove bool from enum class AzureBackend

ffea4e2

Fail if only one of the environment variables is set

502997a

Make PreexistingData::ContainerPath take an argument and use it inste…

e37a7b4

…ad of ConcatAbstractPath where applicable

Simplify condition

0a31158

Use ASSERT_RAISES instead of ASSERT_NOT_OK

dd9868f

Remove redundant std::move

5aab621

Make AzureOptions factories return a Result<>

6b4993d

Rename all the tests to have Test- in the beginning

7fef9f3

Rename hierarchical_namespace_ to hns_detector_

272aa82

Fix lint error

5e47e2b

azurefs.cc: Alias the Azure::Core::HTTP namespace as well

bdf8cb3

Move ExceptionToStatus to azurefs.cc

e45d94d

Prevent half-constructed state and remove AzureFileSystem::Init()

7d2d789

Remove redundant virtual destructors

ccd1f1d

typo

3b3c329

Correct ordering of fields

4f45c67

felipecrv force-pushed the azurefs_tests branch from 4023774 to 4f45c67 Compare December 14, 2023 17:28

pitrou approved these changes Dec 14, 2023

View reviewed changes

felipecrv changed the title ~~GH-39119: [C++] Refactor the Azure FS tests~~ GH-39119: [C++] Refactor the Azure FS tests and filesystem class instantiation Dec 14, 2023

felipecrv merged commit 4e58f7c into apache:main Dec 14, 2023

felipecrv removed the awaiting merge Awaiting merge label Dec 14, 2023

felipecrv deleted the azurefs_tests branch December 14, 2023 18:29

Conversation

felipecrv commented Dec 13, 2023 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Dec 13, 2023

Uh oh!

felipecrv commented Dec 13, 2023

Uh oh!

felipecrv Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

felipecrv Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pitrou Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

felipecrv Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

pitrou Dec 14, 2023

Choose a reason for hiding this comment

Uh oh!

felipecrv Dec 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pitrou commented Dec 14, 2023

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

conbench-apache-arrow bot commented Dec 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

felipecrv commented Dec 13, 2023 •

edited by github-actions bot

Loading