-
Notifications
You must be signed in to change notification settings - Fork 343
Metadata Log Entries metadata table #667
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
Changes from 8 commits
9a0423d
ecec57e
9e506c2
9c77d57
b26f08f
58b0609
f7dd165
4655c97
d989802
8613c2e
f45f2ea
2d417da
502e5d8
1cd5f93
3fe675d
6ce55f4
aa068ff
ab74a3e
f274ef9
a338e17
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -226,7 +226,8 @@ def __eq__(self, other: Any) -> bool: | |
class Snapshot(IcebergBaseModel): | ||
snapshot_id: int = Field(alias="snapshot-id") | ||
parent_snapshot_id: Optional[int] = Field(alias="parent-snapshot-id", default=None) | ||
sequence_number: Optional[int] = Field(alias="sequence-number", default=None) | ||
# cannot import `INITIAL_SEQUENCE_NUMBER` due to circular import | ||
sequence_number: Optional[int] = Field(alias="sequence-number", default=0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason the default value for the sequence number has to be changed to 0 as opposed to None? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to the spec, https://iceberg.apache.org/spec/#version-2
Also added this in the PR description There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kevinjqliu Thanks for spotting this! We definitely need to read snapshot.sequence_number as 0 for v1. However, as we have observed in the test outcome, making
I think we may need a new field_serializer in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you! I missed the part about the V1 spec. Following your suggestion, I added a |
||
timestamp_ms: int = Field(alias="timestamp-ms", default_factory=lambda: int(time.time() * 1000)) | ||
manifest_list: Optional[str] = Field( | ||
alias="manifest-list", description="Location of the snapshot's manifest list file", default=None | ||
|
Uh oh!
There was an error while loading. Please reload this page.