krishnakaanchan-png commented on issue #2698:
URL:
https://github.com/apache/iceberg-python/issues/2698#issuecomment-5482991757
I looked into this and I think the diagnosis in the issue is slightly off,
in a way that matters for where the fix goes.
The issue says PyArrow expects `abfs://<file_system>/<path>`. That is not
the case. PyArrow parses the canonical Azure URI correctly and pulls the
account name out of it:
```python
>>> from pyarrow.fs import FileSystem
>>>
FileSystem.from_uri("abfss://[email protected]/wh/db/t/d.parquet")
(<pyarrow._azurefs.AzureFileSystem object at 0x...>,
'myfs/wh/db/t/d.parquet')
```
So the problem is entirely on our side. It is this line in `parse_location`,
`pyiceberg/io/pyarrow.py:425`:
```python
return uri.scheme, uri.netloc, f"{uri.netloc}{uri.path}"
```
That rule is written for S3, where netloc is the bucket, so `bucket/key` is
exactly what `S3FileSystem` wants. For Azure the netloc is
`[email protected]` and the whole thing gets pasted in
front of the path:
```
abfss://[email protected]/wh/db/t/d.parquet
-> ('abfss', '[email protected]',
'[email protected]/wh/db/t/d.parquet')
```
PyArrow then takes the first path segment as the container. You can see the
difference on the same filesystem object:
```python
fs = AzureFileSystem(account_name="myacct")
fs.get_file_info("myfs/wh/db/t/d.parquet")
# Check for Hierarchical Namespace support on
'https://myacct.blob.core.windows.net/myfs' failed
fs.get_file_info("[email protected]/wh/db/t/d.parquet")
# ListBlobsByHierarchy failed for prefix='wh/db/t/d.parquet'
```
Both fail here only because I had no credentials, but the container we are
asking for is different. In the second case we are asking for a container
literally named `[email protected]`. Per the [container naming
rules](https://learn.microsoft.com/en-us/rest/api/storageservices/naming-and-referencing-containers--blobs--and-metadata#container-names)
a container name has to be a valid DNS name, lowercase letters and numbers and
hyphens only, 3 to 63 characters. `@` and `.` are not allowed. The same doc
says a name that violates the rules fails with 400. So with working credentials
this does not surface as a not found, it surfaces as a Bad Request, which makes
it harder to recognise.
The URI form itself is confirmed in [the ABFS URI
doc](https://learn.microsoft.com/en-us/azure/storage/blobs/data-lake-storage-introduction-abfs-uri),
`abfs[s]://<file_system>@<account_name>.dfs.core.windows.net/<path>/<file_name>`,
where file system is the same thing as a blob container.
There is a second half to this. `_initialize_fs` calls
`self._initialize_azure_fs()` with no arguments, unlike
`_initialize_s3_fs(netloc)` and `_initialize_hdfs_fs(scheme, netloc)` right
next to it. So the account name can only come from `adls.account-name` and the
account in the location is dropped. That is why a location without the property
set fails with `ArrowInvalid: AzureOptions doesn't contain a valid account
name`, and it is also why a single FileIO cannot serve two accounts.
`FsspecFileIO` already solves both of these. `_ADLS_SCHEMES` at
`fsspec.py:334`, `uri.hostname` passed through for those schemes at
`fsspec.py:498`, and the account derived as `hostname.split(".")[0]` at
`fsspec.py:276` when the property is not set. So this can be framed as making
`PyArrowFileIO` consistent with `FsspecFileIO` rather than as new behaviour.
On why CI did not catch it. All the ADLS tests build their location as
`f"{adls_scheme}://warehouse/{filename}"`, container only with no account, and
the account is supplied to Azurite through properties. So we only ever exercise
the one URI shape that happens to work. `test_parse_location` has hdfs and
local cases and no Azure case at all.
What I would like to do:
1. An Azure branch in `parse_location` for `abfs`, `abfss`, `wasb`, `wasbs`,
returning `f"{uri.username}{uri.path}"` when there is a userinfo part and
falling back to current behaviour when there is not. Gating on `uri.username`
and not on `uri.hostname` is deliberate, on the container only form
`uri.hostname` is the container and inferring an account from it would break
the existing tests.
2. Pass netloc into `_initialize_azure_fs` and derive `account_name` from
the hostname when `adls.account-name` is absent, same precedence as fsspec.
3. Azure cases in `test_parse_location`, which need no infra, plus running
the ADLS integration tests over the account qualified form as well.
Three things I would rather have your call on before I write it:
- Precedence when the property and the location disagree on the account.
fsspec lets the property win. I am inclined to keep that for consistency, but
property wins does mean a catalog spanning two accounts silently reads from the
wrong one, so raising is also defensible. Which do you prefer?
- Deriving the account as `hostname.split(".")[0]` holds for standard
endpoints, private endpoints, sovereign clouds and the DNS zone endpoints,
since the account is the first label in all of those. It does not hold if
someone has mapped a [custom
domain](https://learn.microsoft.com/en-us/azure/storage/blobs/storage-custom-domain-name)
on the blob endpoint. fsspec has the same limitation today. Fine to accept
that, or do you want the property to be mandatory in that case?
- I am leaving the endpoint and TLS side out of this. The scheme carries
meaning in Azure, wasb goes to the blob endpoint and abfs to the dfs endpoint,
and the trailing s indicates TLS, but we collapse all four schemes onto one
`AzureFileSystem` and only the `adls.blob-storage-*` and `adls.dfs-storage-*`
properties control any of it. Nothing is broken there today and inferring it
from the scheme would change behaviour for anyone using those properties, so I
think it belongs in a separate issue. Let me know if you disagree.
I will keep this to #2698 only. This does fix the `PyArrowFileIO` part of
#2271, but that one is fsspec plus a server side error from Nessie, so I do not
want to claim it closes that.
Separately, while reading this I think there is an unrelated bug in `_adls`.
It writes the inferred account back into the dict it is given, and the call
site at `fsspec.py:515` passes `self.properties`. So the first ADLS location a
`FsspecFileIO` touches pins `adls.account-name` for every later location. The
`lru_cache` on `(scheme, hostname)` above it correctly builds a fresh
filesystem for a second account, then `_adls` skips the inference because the
key now exists and hands back a filesystem pointing at the first account. Happy
to raise that as its own issue if it is not already known.
Checked on main with pyarrow 25.0.1.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
