Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Deprecate ADLFS prefix in favor of ADLS #961

Merged
merged 2 commits into from
Sep 10, 2024
Merged

Deprecate ADLFS prefix in favor of ADLS #961

merged 2 commits into from
Sep 10, 2024

Conversation

ndrluis
Copy link
Collaborator

@ndrluis ndrluis commented Jul 24, 2024

Solves #866

HonahX
HonahX reviewed Jul 26, 2024
View reviewed changes
Copy link
Contributor

@HonahX HonahX left a comment

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

pyiceberg/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"
Copy link
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.

pyiceberg/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)()
Copy link
Contributor

@HonahX HonahX Jul 26, 2024 •
edited
Loading

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

pyiceberg/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(
Copy link
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:
from s3fs import S3FileSystem
from pyiceberg.table import PropertyUtil

@ndrluis ndrluis force-pushed the adlfs-deprecation branch 2 times, most recently from 5f37c08 to 652c453 Compare August 2, 2024 14:22
@ndrluis ndrluis requested a review from HonahX August 2, 2024 14:22
@ndrluis ndrluis added this to the PyIceberg 0.8.0 release milestone Aug 6, 2024
kevinjqliu
kevinjqliu approved these changes Aug 9, 2024
View reviewed changes
Copy link
Contributor

@kevinjqliu kevinjqliu left a comment

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

pyiceberg/io/fsspec.py Outdated Show resolved Hide resolved
@ndrluis ndrluis requested a review from sungwy August 29, 2024 15:28
@kevinjqliu
Copy link
Contributor

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

@ndrluis
Copy link
Collaborator Author

ndrluis commented Sep 6, 2024

@kevinjqliu Done!

kevinjqliu
kevinjqliu reviewed Sep 6, 2024
View reviewed changes
Copy link
Contributor

@kevinjqliu kevinjqliu left a comment

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

@ndrluis ndrluis mentioned this pull request Sep 10, 2024
Open
@sungwy sungwy merged commit b593375 into apache:main Sep 10, 2024
8 checks passed
@sungwy
Copy link
Collaborator

sungwy commented Sep 10, 2024

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinjqliu kevinjqliu kevinjqliu approved these changes

@sungwy sungwy sungwy approved these changes

@HonahX HonahX Awaiting requested review from HonahX

@Fokko Fokko Awaiting 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