HonahX commented on code in PR #748:
URL: https://github.com/apache/iceberg-python/pull/748#discussion_r1621866561
##########
pyiceberg/table/__init__.py:
##########
@@ -1290,6 +1291,19 @@ def snapshot_by_name(self, name: str) ->
Optional[Snapshot]:
return self.snapshot_by_id(ref.snapshot_id)
return None
+ def latest_snapshot_before_timestamp(self, timestamp_ms: int) ->
Optional[Snapshot]:
+ """Get the snapshot right before the given timestamp, or None if there
is no matching snapshot."""
+ result, prev_timestamp = None, 0
+ if self.metadata.current_snapshot_id is not None:
+ for snapshot in self.current_ancestors():
+ if snapshot and prev_timestamp < snapshot.timestamp_ms <
timestamp_ms:
Review Comment:
That's an interesting finding. I am also not sure the exact decision behind
whether inclusive. Although whether to include the exact timestamp does not
matter a lot in this use case, I think we may still want to keep the edge case
behavior the same as Java: that we do not include the given timestamp when
`rollback_to_timestamp`.
Since this will be a public util method, how about adding another parameter
like
- `inclusive`
- `include_given_timstamp`
- ...
that default to `True` and we disable it when using it in
`rollback_to_timestamp` implementation?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
