Support wasb[s] paths in ADLSFileIO #11294

mrcnc · 2024-10-10T04:34:16Z

Resolves #10127 by delegating wasb[s] paths to the ADLSFileIO via ResolvingFileIO.

Additionally, I refactored the parsing logic to use java.net.URI because I believe it's clearer. This also led me to change the invalid URI exception to IllegalArgumentException instead of a ValidationException b/c this seems more appropriate.

RussellSpitzer · 2024-10-10T19:20:49Z

azure/src/main/java/org/apache/iceberg/azure/adlsv2/ADLSLocation.java

 * <p>Locations follow the conventions used by Hadoop's Azure support, i.e.
 *
- * <pre>{@code abfs[s]://[<container>@]<storage account host>/<file path>}</pre>
+ * <pre>{@code abfs[s]://[<container>@]<storageAccount>.dfs.core.windows.net/<path>}</pre>


Is this a change? or is Storage Account Host equivalent to storageAccount.dfs.core.windows.net

I think storageAccount is more commonly used than storageHost. (ref)

I see the ambiguity here so I changed the variable to use storageEndpoint to clarify. Previously we were using the storageAccount variable to store the entire endpoint's hostname and not the storage account's name

azure/src/main/java/org/apache/iceberg/azure/adlsv2/ADLSLocation.java

azure/src/test/java/org/apache/iceberg/azure/adlsv2/ADLSLocationTest.java

RussellSpitzer · 2024-10-10T19:29:27Z

@ashvina Can you please review as well?

azure/src/main/java/org/apache/iceberg/azure/adlsv2/ADLSLocation.java

RussellSpitzer

This looks good to me although I have a few nits, I am not a ADLS expert though so I'm hoping we can get someone with more expertise in that system to give a confirmation that this is correct.

ashvina · 2024-10-10T21:20:21Z

azure/src/main/java/org/apache/iceberg/azure/adlsv2/ADLSLocation.java

 * <p>Locations follow the conventions used by Hadoop's Azure support, i.e.
 *
- * <pre>{@code abfs[s]://[<container>@]<storage account host>/<file path>}</pre>
+ * <pre>{@code abfs[s]://[<container>@]<storageAccount>.dfs.core.windows.net/<path>}</pre>


I think storageAccount is more commonly used than storageHost. (ref)

ashvina · 2024-10-10T21:30:55Z

azure/src/main/java/org/apache/iceberg/azure/adlsv2/ADLSLocation.java

+      // storage account name is the first part of the host
+      int accountSplit = uri.getHost().indexOf('.');
+      String storageAccountName = uri.getHost().substring(0, accountSplit);
+      this.storageAccount = String.format("%s.dfs.core.windows.net", storageAccountName);


For wasb, the original host URL is a blob.core.windows.net endpoint (see the sample above). Could you clarify if the change from blob to dfs is necessary?

I don't believe this is required b/c it appears the blob host would be changed in the ADLS client when setting the endpoint here. I'll make an update for this b/c I think we may also want to change some of the nomenclature as well

RussellSpitzer · 2024-10-15T19:05:50Z

@ashvina could you do another pass?

ashvina

LGTM 👍
I ran basic tests on my storage account (wasb) with the changes in this PR, and they all passed successfully.

RussellSpitzer · 2024-10-16T15:41:10Z

Thanks @mrcnc for the PR and @ashvina for review!

bryanck · 2024-10-16T17:21:00Z

azure/src/main/java/org/apache/iceberg/azure/adlsv2/ADLSLocation.java

-    uriPath = uriPath == null ? "" : uriPath.startsWith("/") ? uriPath.substring(1) : uriPath;
-    this.path = uriPath.split("\\?", -1)[0].split("#", -1)[0];
+    try {
+      URI uri = new URI(location);


Not using java.net.URI before was intentional, in the past we have avoided using java.net.URI for parsing as it can cause some unexpected results. For example, underscores in host names...

jshell> new java.net.URI("abfs://my_endpoint/path").getHost() $1 ==> null

There are some other quirks also IIRC.

(cc @rdblue since you've pointed this out to me)

I think that's fine, but if this is the case we need to add in some explicit tests for this scenario. @mrcnc would you like to add a follow-on pr?

I believe we will not encounter this error b/c underscores aren't valid characters for storage accounts or container names. But maybe we should add more input validation or update the regex before attempting to use use java.net.URI? I was thinking using URI would be more idiomatic but if we have reasons to avoid using it then we can revert or refactor

I wish we had caught this before now, but I think we need to revert the URI additions. There are a number of issues related to the Java implementation which is why we specifically avoid using it.

Could we have some examples of where this doesn't work?

Jordano-Dremio · 2024-10-16T23:49:42Z

Hi @mrcnc, @RussellSpitzer . To confirm this PR solution to 10127: any existing (or new) iceberg tables stored in azure with wasbs + .blob. will be interpreted interchangeably with abfss prefix + .dfs. endpoint, regardless of whether the table's existing metadata displays wasbs + .blob. vs the new abfss + dfs?

That being said, does this mean iceberg api supports tables with possibly mixed schemes? (i.e. some paths in a table's metadata were written with 'wasbs' while others were with 'abfss')?

This reverts commit 11a8a78.

mrcnc · 2024-10-17T21:59:45Z

Hi @mrcnc, @RussellSpitzer . To confirm this PR solution to 10127: any existing (or new) iceberg tables stored in azure with wasbs + .blob. will be interpreted interchangeably with abfss prefix + .dfs. endpoint, regardless of whether the table's existing metadata displays wasbs + .blob. vs the new abfss + dfs?

That being said, does this mean iceberg api supports tables with possibly mixed schemes? (i.e. some paths in a table's metadata were written with 'wasbs' while others were with 'abfss')?

The path for new files in existing tables is determined by the table's base location or LocationProvider so tables that have paths using the wasbs scheme would still use the same scheme for new files. For new tables, the location will default to the default location for the warehouse or namespace but each table can explicitly provide a location when it is created. It seems possible that there could be mixed schemes (for example you changed the implementation of a custom LocationProvider) so I think it's worthwhile to understand if mixed paths are supported but I'm not sure currently. The intent of this PR was to use the ADLSFileIO for wasbs paths by default, instead of delegating these to HadoopFileIO as a fallback

…11344) This reverts commit 11a8a78.

…" (apache#11344) This reverts commit 11a8a78.

github-actions bot added core AZURE labels Oct 10, 2024

mrcnc marked this pull request as draft October 10, 2024 12:47

mrcnc added 2 commits October 10, 2024 07:52

Azure: Accept wasb[s] paths in ADLSLocation

5da8c7f

Core: Resolve wasb[s] scheme to ADLSFileIO via ResolvingFileIO

3095ff6

mrcnc force-pushed the support-wasb-for-adlsfileio branch from 228b731 to 3095ff6 Compare October 10, 2024 12:53

mrcnc marked this pull request as ready for review October 10, 2024 14:25

RussellSpitzer reviewed Oct 10, 2024

View reviewed changes

azure/src/main/java/org/apache/iceberg/azure/adlsv2/ADLSLocation.java Outdated Show resolved Hide resolved

RussellSpitzer reviewed Oct 10, 2024

View reviewed changes

azure/src/test/java/org/apache/iceberg/azure/adlsv2/ADLSLocationTest.java Show resolved Hide resolved

RussellSpitzer reviewed Oct 10, 2024

View reviewed changes

azure/src/main/java/org/apache/iceberg/azure/adlsv2/ADLSLocation.java Outdated Show resolved Hide resolved

RussellSpitzer reviewed Oct 10, 2024

View reviewed changes

azure/src/main/java/org/apache/iceberg/azure/adlsv2/ADLSLocation.java Outdated Show resolved Hide resolved

RussellSpitzer reviewed Oct 10, 2024

View reviewed changes

azure/src/main/java/org/apache/iceberg/azure/adlsv2/ADLSLocation.java Outdated Show resolved Hide resolved

RussellSpitzer approved these changes Oct 10, 2024

View reviewed changes

ashvina reviewed Oct 10, 2024

View reviewed changes

nastra requested a review from bryanck October 11, 2024 10:26

Addressing PR comments

7f4ad69

tzuan16 mentioned this pull request Oct 16, 2024

Block the creation of catalog with WASB[S] locations apache/polaris#376

Closed

11 tasks

ashvina approved these changes Oct 16, 2024

View reviewed changes

RussellSpitzer merged commit 11a8a78 into apache:main Oct 16, 2024

bryanck reviewed Oct 16, 2024

View reviewed changes

RussellSpitzer added a commit that referenced this pull request Oct 17, 2024

Revert "Core, Azure: Support wasb[s] paths in ADLSFileIO (#11294)"

a9266af

This reverts commit 11a8a78.

RussellSpitzer mentioned this pull request Oct 17, 2024

Revert "Support wasb[s] paths in ADLSFileIO" #11344

Merged

mrcnc deleted the support-wasb-for-adlsfileio branch October 17, 2024 21:59

nastra pushed a commit that referenced this pull request Oct 18, 2024

Revert "Core, Azure: Support wasb[s] paths in ADLSFileIO (#11294)" (#…

fd06438

…11344) This reverts commit 11a8a78.

zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024

Core, Azure: Support wasb[s] paths in ADLSFileIO (apache#11294)

3c7f0fd

zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024

Revert "Core, Azure: Support wasb[s] paths in ADLSFileIO (apache#11294)…

b83e337

…" (apache#11344) This reverts commit 11a8a78.

Support wasb[s] paths in ADLSFileIO #11294

Support wasb[s] paths in ADLSFileIO #11294

Uh oh!

Conversation

mrcnc commented Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

RussellSpitzer commented Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RussellSpitzer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer commented Oct 15, 2024

Uh oh!

ashvina left a comment

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer commented Oct 16, 2024

Uh oh!

bryanck Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jordano-Dremio commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrcnc commented Oct 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mrcnc commented Oct 10, 2024 •

edited

Loading

RussellSpitzer commented Oct 10, 2024 •

edited

Loading

bryanck Oct 16, 2024 •

edited

Loading

Jordano-Dremio commented Oct 16, 2024 •

edited

Loading