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

Python: Add support for initial default #7699

Merged
merged 3 commits into from
Jun 2, 2023

Conversation

Fokko
Copy link
Contributor

@Fokko Fokko commented May 25, 2023

This makes it easier to inherit the sequence numbers

This makes it easier to inherit the sequence numbers
assert manifest_list.content is None
assert manifest_list.sequence_number is None
assert manifest_list.min_sequence_number is None
assert manifest_list.content == 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker, but we should have an enum for content and use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will go well for a long time:

python git:(fd-initial-default) ✗ python3
Python 3.11.3 (main, Apr  7 2023, 20:13:31) [Clang 14.0.0 (clang-1400.0.29.202)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from pyiceberg.manifest import ManifestContent
>>> ManifestContent.DATA == 0
True
>>> ManifestContent.DATA == 1
False
>>> 1 == ManifestContent.DATA
False

But you're right and this should be an enum, updated.

Copy link
Contributor

@rdblue rdblue left a comment

Choose a reason for hiding this comment

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

I think there's just one blocker in the resolution behavior with required fields. We should also extend tests to cover those cases.

@Fokko
Copy link
Contributor Author

Fokko commented May 29, 2023

@rdblue Thanks for the review. Went over the comments and added extensive tests for v1 and v2 manifest-file's.

The CI will fail. The caching logic for caching the docker image worked on the PR (#7698) but somehow fails on the master branch. I think the easiest way is to revert the changes for now. I'll open up a new PR with the changes and also refactoring of the caching logic (I don't like it, way to complicated).

@Fokko Fokko requested a review from rdblue May 30, 2023 19:18
@rdblue rdblue merged commit 5f39174 into apache:master Jun 2, 2023
@rdblue
Copy link
Contributor

rdblue commented Jun 2, 2023

Merged. Thanks, @Fokko!

The only thing that caught my eye this time around was adding an enum reader. I was thinking that the enum would work like an alias for the ID or that we would translate in the type after it was read, but it looks like this works just fine.

@Fokko Fokko deleted the fd-initial-default branch June 3, 2023 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants