Skip to content

Conversation

@OliLay
Copy link
Contributor

@OliLay OliLay commented Jul 1, 2024

Rationale for this change

See #43097.

What changes are included in this PR?

Implements AzureFS::PathFromUri using existing URI parsing and path extraction inside the AzureOptions.

Are these changes tested?

Yes, added a unit test.

Are there any user-facing changes?

No, but calling PathFromUri will now work instead of throwing due to no implementation provided.

@github-actions
Copy link

github-actions bot commented Jul 1, 2024

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

@OliLay OliLay changed the title GH-43097: [C+] Implement PathFromUri support for Azure file system GH-43097: [C++] Implement PathFromUri support for Azure file system Jul 3, 2024
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that other filesystem implementations use internal::PathFromUriHelper(). Can we use it in Azure filesystem too?

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Jul 19, 2024
@OliLay
Copy link
Contributor Author

OliLay commented Jul 19, 2024

It seems that other filesystem implementations use internal::PathFromUriHelper(). Can we use it in Azure filesystem too?

I tried that at first, but the issue is that for Azure we have to support different URI schemes where the authority is handled differently.
An example:
abfss://storageacc.blob.core.windows.net/container/some/path vs. abfss://acc:pw@container/some/path where both are accepted URIs by arrow and should yield the same path container/some/path.

So just using internal::PathFromUriHelper() (either with prepending the authority or with not prepending it) would break parsing one of these URIs. So we'll have to check which kind of URI we have and decide based on that - I saw this is already implemented in AzureOptions::FromUri (and this also returns the path), so I just used this.

@OliLay OliLay requested a review from kou July 19, 2024 06:36
Copy link
Contributor

@Tom-Newton Tom-Newton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy 👍

RETURN_NOT_OK(uri.Parse(uri_string));
RETURN_NOT_OK(AzureOptions::FromUri(uri, &path));

std::vector<std::string> supported_schemes = {"abfs", "abfss"};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: It would be nice to have a comment explaining why internal::PathFromUriHelper() can't be used in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I added a comment. 👍

@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Aug 5, 2024
@OliLay
Copy link
Contributor Author

OliLay commented Aug 13, 2024

@kou Can you give it another look? I think it should be good to go.

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Sorry for my late review...

@kou kou merged commit 88e8140 into apache:main Aug 14, 2024
@kou kou removed the awaiting change review Awaiting change review label Aug 14, 2024
@github-actions github-actions bot added the awaiting merge Awaiting merge label Aug 14, 2024
@conbench-apache-arrow
Copy link

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants