-
Notifications
You must be signed in to change notification settings - Fork 150
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
Add spec_id
back to data file
#63
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great start, but I think we need to inherit the spec from the manifest.
I think we can generalize the _inherit_sequence_number
by renaming it to something like _inherit_from_manifest
, and add a line to copy the spec-id
from the manifest:
entry.data_file.spec_id = manifest.partition_spec_id
pyiceberg/manifest.py
Outdated
@@ -182,6 +182,7 @@ def __repr__(self) -> str: | |||
doc="Splittable offsets", | |||
), | |||
NestedField(field_id=140, name="sort_order_id", field_type=IntegerType(), required=False, doc="Sort order ID"), | |||
NestedField(field_id=141, name="spec_id", field_type=IntegerType(), required=False, doc="Partition spec ID"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to add the fields here since the spec_id
isn't read/written.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @puchengy for fixing this 🙌
@puchengy can you fix the CI? We need to make this part of 0.5.1 since the |
@Fokko Will do that. |
|
@Fokko thanks I will address that today or tomorrow. If this becomes a blocker of the release and we need get it done quick, feel free to take it over. |
@puchengy let me give it a try |
@puchengy I think this fixes it: puchengy#1 |
@Fokko Thanks, adopted your suggestion and tried a local integration test and it passed. |
Thanks! It looks like some changes on the request side broke the CI: #69 |
62edada
to
d23e107
Compare
spec_id
back to data file
This is great, thanks @puchengy for fixing this 👍 |
Spec id was removed in https://github.com/apache/iceberg-python/pull/40/files to wait for the decision of apache/iceberg#8730. This PR adds it back and is needed to unblock issue #46