GH-39119: [C++] Refactor the Azure FS tests and filesystem class instantiation

[felipecrv]

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]

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

[felipecrv]

@kou@Tom-Newton@av8or1

[felipecrv]

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

[felipecrv]

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

[kou]

+1

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

[pitrou]

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

[felipecrv]

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

[pitrou]

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?

[felipecrv]

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.

[pitrou]

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

[pitrou]

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

[conbench-apache-arrow]

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.

[felipecrv]

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]

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

[felipecrv]

@kou@Tom-Newton@av8or1

[felipecrv]

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

[felipecrv]

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

[kou]

+1

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

[pitrou]

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

[felipecrv]

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

[pitrou]

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?

[felipecrv]

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.

[pitrou]

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

[pitrou]

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

[conbench-apache-arrow]

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.

[felipecrv]

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]

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

[felipecrv]

@kou@Tom-Newton@av8or1

[felipecrv]

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

[felipecrv]

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

[kou]

+1

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

[pitrou]

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

[felipecrv]

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

[pitrou]

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?

[felipecrv]

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.

[pitrou]

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

[pitrou]

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

[conbench-apache-arrow]

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.

[felipecrv]

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]

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

[felipecrv]

@kou@Tom-Newton@av8or1

[felipecrv]

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

[felipecrv]

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

[kou]

+1

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

[pitrou]

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

[felipecrv]

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

[pitrou]

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?

[felipecrv]

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.

[pitrou]

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

[pitrou]

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

[conbench-apache-arrow]

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.

[felipecrv]

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]

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

[felipecrv]

@kou@Tom-Newton@av8or1

[felipecrv]

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

[felipecrv]

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

[kou]

+1

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

[pitrou]

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

[felipecrv]

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

[pitrou]

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?

[felipecrv]

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.

[pitrou]

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

[pitrou]

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

[conbench-apache-arrow]

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.

[felipecrv]

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]

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

[felipecrv]

@kou@Tom-Newton@av8or1

[felipecrv]

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

[felipecrv]

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

[kou]

+1

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

[pitrou]

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

[felipecrv]

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

[pitrou]

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?

[felipecrv]

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.

[pitrou]

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

[pitrou]

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

[conbench-apache-arrow]

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.

[felipecrv]

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]

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

[felipecrv]

@kou@Tom-Newton@av8or1

[felipecrv]

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

[felipecrv]

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

[kou]

+1

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

[pitrou]

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

[felipecrv]

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

[pitrou]

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?

[felipecrv]

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.

[pitrou]

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

[pitrou]

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

[conbench-apache-arrow]

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.

[felipecrv]

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]

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

[felipecrv]

@kou@Tom-Newton@av8or1

[felipecrv]

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

[felipecrv]

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

[kou]

+1

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

[pitrou]

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

[felipecrv]

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

[pitrou]

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?

[felipecrv]

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.

[pitrou]

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

[pitrou]

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

[conbench-apache-arrow]

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.