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
this. 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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
