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

Snapshot: Make manifest-list required #1385

Merged
merged 3 commits into from
Dec 17, 2024
Merged

Conversation

Fokko
Copy link
Contributor

@Fokko Fokko commented Nov 27, 2024

Since we don't support manifests, we can assume that the manifest-list is there all the time.

image

@kevinjqliu
Copy link
Contributor

we can assume that the manifest-list is there all the time.

what about for V1 tables where the manifest-list field is optional?

@Fokko
Copy link
Contributor Author

Fokko commented Nov 27, 2024

Then we should read the manifests field, but that has never been implemented on PyIceberg. No engine is using that, and I think we should deprecate it. By making the manifest-list optional, we error explicitly when we try to load a snapshot that relies on the manifests field.

@Fokko Fokko requested a review from kevinjqliu December 9, 2024 14:21
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

pyiceberg/table/snapshots.py Outdated Show resolved Hide resolved
@kevinjqliu
Copy link
Contributor

looks like manifest-list is missing in our example

"snapshots": [{"snapshot-id": 1925, "timestamp-ms": 1602638573822}],

@kevinjqliu
Copy link
Contributor

tests/table/test_metadata.py::test_serialize_v1 and tests/table/test_metadata.py::test_v1_write_metadata_for_v2 also fails

@Fokko
Copy link
Contributor Author

Fokko commented Dec 17, 2024

@kevinjqliu Thanks, some bad tests. They didn't have the manifest-list in the snapshot, which is invalid according to the spec.

@kevinjqliu kevinjqliu merged commit e15f355 into apache:main Dec 17, 2024
7 checks passed
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.

2 participants