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.

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

Centralized table properties management #388

Merged
merged 3 commits into from
Feb 8, 2024

Conversation

HonahX
Copy link
Contributor

@HonahX HonahX commented Feb 7, 2024

Fixes #365

My initial plan was to add the names and defaults of all table properties. However, upon further consideration, I think it may be better to add properties as needed.

@Fokko Fokko added this to the PyIceberg 0.7.0 release milestone Feb 7, 2024
pyiceberg/table/__init__.py Outdated Show resolved Hide resolved
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.

this is awesome!
Slight nit, it is better code organization-wise to move the properties to something like table/properties.py? just so that properties are easier to find. table/__init__.py is already 2.5k lines long

@HonahX
Copy link
Contributor Author

HonahX commented Feb 8, 2024

this is awesome! Slight nit, it is better code organization-wise to move the properties to something like table/properties.py? just so that properties are easier to find. table/__init__.py is already 2.5k lines long

@kevinjqliu Thanks for the review! According to #365 (comment), the purpose of putting properties in table/__init__.py is to avoid reduced import speed caused by the new file. I totally agree that we should make these properties easy to find. So I put the class at the top of the file. Does this look good to you?

@Fokko Fokko merged commit ba2fe43 into apache:main Feb 8, 2024
6 checks passed
@Fokko
Copy link
Contributor

Fokko commented Feb 8, 2024

This is much better, thanks @HonahX for working on this. Let's get this in, so we don't break the references later on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Centralized Management of Table Properties
4 participants