Skip to content

Deprecate ADLFS prefix in favor of ADLS - #961

Merged
sungwy merged 2 commits into
apache:mainfrom
ndrluis:adlfs-deprecation
Sep 10, 2024
Merged

Deprecate ADLFS prefix in favor of ADLS#961
sungwy merged 2 commits into
apache:mainfrom
ndrluis:adlfs-deprecation

Conversation

@ndrluis

@ndrluisndrluis commented Jul 24, 2024

Copy link
Copy Markdown
Collaborator

Solves #866

HonahX
HonahX reviewed Jul 26, 2024
View reviewed changes

@HonahXHonahX left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for working on this! Just some minor comments

Comment threadpyiceberg/io/__init__.py Outdated
Comment on lines +73 to +79
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"

@HonahXHonahXJul 26, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How about

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.

Comment threadpyiceberg/io/fsspec.py Outdated
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)()

@HonahXHonahXJul 26, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just put a note here: a more elegant way to send deprecation message will be available in @ndrluis 's another PR: #962

Comment threadpyiceberg/io/fsspec.py Outdated
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(

@HonahXHonahXJul 26, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[nit] For TableProperties and PropertyUtil, we usually do inline import:

iceberg-python/pyiceberg/io/fsspec.py

Lines 128 to 132 in 4fc7589

def_s3(properties: Properties) ->AbstractFileSystem:
froms3fsimportS3FileSystem
frompyiceberg.tableimportPropertyUtil

@ndrluis
ndrluisforce-pushed the adlfs-deprecation branch 2 times, most recently from 5f37c08 to 652c453CompareAugust 2, 2024 14:22
@ndrluis
ndrluis requested a review from HonahXAugust 2, 2024 14:22
@ndrluisndrluis added this to the PyIceberg 0.8.0 release milestone Aug 6, 2024
@ndrluis
ndrluis requested review from Fokko and kevinjqliuAugust 9, 2024 12:41
kevinjqliu
kevinjqliu approved these changes Aug 9, 2024
View reviewed changes

@kevinjqliukevinjqliu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, looks like you cover all the s/adlfs/adls
minor comment on the deprecation message

Comment threadpyiceberg/io/fsspec.py Outdated
@ndrluis
ndrluis requested a review from sungwyAugust 29, 2024 15:28
@kevinjqliu

kevinjqliu commented Sep 5, 2024

Copy link
Copy Markdown
Contributor

@ndrluis do you mind rebasing this PR? Looks like its almost good to go

@ndrluis

ndrluis commented Sep 6, 2024

Copy link
Copy Markdown
CollaboratorAuthor

@kevinjqliu Done!

kevinjqliu
kevinjqliu reviewed Sep 6, 2024
View reviewed changes

@kevinjqliukevinjqliu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

https://github.com/search?q=repo%3Aapache%2Ficeberg-python%20adlfs&type=code

there are a couple more instances of adlfs in the repo

@ndrluisndrluis mentioned this pull request Sep 10, 2024
Closed
@sungwy
sungwy merged commit b593375 into apache:mainSep 10, 2024
@sungwy

sungwy commented Sep 10, 2024

Copy link
Copy Markdown
Collaborator

Thank you for working on this fix @ndrluis and thank you @kevinjqliu and @HonahX for the reviews!

@ndrluis
ndrluis deleted the adlfs-deprecation branch September 10, 2024 15:37
@kevinjqliukevinjqliu mentioned this pull request Nov 15, 2024
Closed
sungwy pushed a commit to sungwy/iceberg-python that referenced this pull request Dec 7, 2024
@ndrluis@sungwy
Deprecate ADLFS prefix in favor of ADLS (apache#961)
39f2989
* Deprecate ADLFS prefix in favor of ADLS
* Add missing renaming
sungwy pushed a commit to sungwy/iceberg-python that referenced this pull request Dec 7, 2024
@ndrluis@sungwy
Deprecate ADLFS prefix in favor of ADLS (apache#961)
817e0d7
* Deprecate ADLFS prefix in favor of ADLS
* Add missing renaming
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kevinjqliukevinjqliukevinjqliu approved these changes
@sungwysungwysungwy approved these changes
@HonahXHonahXAwaiting requested review from HonahX
@FokkoFokkoAwaiting requested review from Fokko

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

PyIceberg 0.8.0 release

Development

Successfully merging this pull request may close these issues.

4 participants

@ndrluis@kevinjqliu@sungwy@HonahX