HonahX commented on code in PR #961:
URL: https://github.com/apache/iceberg-python/pull/961#discussion_r1693562585
##########
pyiceberg/io/__init__.py:
##########
@@ -62,13 +70,13 @@
HDFS_PORT = "hdfs.port"
HDFS_USER = "hdfs.user"
HDFS_KERB_TICKET = "hdfs.kerberos_ticket"
-ADLFS_CONNECTION_STRING = "adlfs.connection-string"
-ADLFS_ACCOUNT_NAME = "adlfs.account-name"
-ADLFS_ACCOUNT_KEY = "adlfs.account-key"
-ADLFS_SAS_TOKEN = "adlfs.sas-token"
-ADLFS_TENANT_ID = "adlfs.tenant-id"
-ADLFS_CLIENT_ID = "adlfs.client-id"
-ADLFS_ClIENT_SECRET = "adlfs.client-secret"
+ADLFS_CONNECTION_STRING = "adls.connection-string"
+ADLFS_ACCOUNT_NAME = "adls.account-name"
+ADLFS_ACCOUNT_KEY = "adls.account-key"
+ADLFS_SAS_TOKEN = "adls.sas-token"
+ADLFS_TENANT_ID = "adls.tenant-id"
+ADLFS_CLIENT_ID = "adls.client-id"
+ADLFS_ClIENT_SECRET = "adls.client-secret"
Review Comment:
How about
```python
ADLS_CONNECTION_STRING = "adls.connection-string"
...
```
for new `adls.*` properties and leave `ADLFS_*` properties unchanged to
remain backward compatible? This could make the constants naming more
consistent.
##########
pyiceberg/io/fsspec.py:
##########
@@ -176,14 +186,50 @@ def _gs(properties: Properties) -> AbstractFileSystem:
def _adlfs(properties: Properties) -> AbstractFileSystem:
from adlfs import AzureBlobFileSystem
+ for property_name in properties:
+ if property_name.startswith(DEPRECATED_ADLFS_PREFIX):
+ deprecated(
+ deprecated_in="0.7.0",
+ removed_in="0.8.0",
+ help_message=f"The property {property_name} is deprecated.
Please use properties that start with adls.",
+ )(lambda: None)()
Review Comment:
Just put a note here: a more elegant way to send deprecation message will be
available in @ndrluis 's another PR:
https://github.com/apache/iceberg-python/pull/963
##########
pyiceberg/io/fsspec.py:
##########
@@ -176,14 +186,50 @@ def _gs(properties: Properties) -> AbstractFileSystem:
def _adlfs(properties: Properties) -> AbstractFileSystem:
from adlfs import AzureBlobFileSystem
+ for property_name in properties:
+ if property_name.startswith(DEPRECATED_ADLFS_PREFIX):
+ deprecated(
+ deprecated_in="0.7.0",
+ removed_in="0.8.0",
+ help_message=f"The property {property_name} is deprecated.
Please use properties that start with adls.",
+ )(lambda: None)()
+
return AzureBlobFileSystem(
- connection_string=properties.get(ADLFS_CONNECTION_STRING),
- account_name=properties.get(ADLFS_ACCOUNT_NAME),
- account_key=properties.get(ADLFS_ACCOUNT_KEY),
- sas_token=properties.get(ADLFS_SAS_TOKEN),
- tenant_id=properties.get(ADLFS_TENANT_ID),
- client_id=properties.get(ADLFS_CLIENT_ID),
- client_secret=properties.get(ADLFS_ClIENT_SECRET),
+ connection_string=PropertyUtil.get_first_property_value(
Review Comment:
[nit] For `TableProperties` and `PropertyUtil`, we usually do inline import:
https://github.com/apache/iceberg-python/blob/4fc75891b84b8511a67df47894b2137feb2c4733/pyiceberg/io/fsspec.py#L128-L132
--
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]
