zhengzengprc commented on a change in pull request #3039:
URL: https://github.com/apache/iceberg/pull/3039#discussion_r758900265
##########
File path: core/src/main/java/org/apache/iceberg/util/SnapshotUtil.java
##########
@@ -101,6 +101,41 @@ public static Snapshot oldestAncestor(Table table) {
return ancestorsOf(start, lookup);
}
+ /**
+ * Traverses the history of the table's current snapshot
+ * and finds the oldest ancestor snapshot after or equal to the timestamp in
milliseconds.
+ * @return null if there is no current snapshot in the table,
+ * else the oldest ancestor snapshot after or equal to the timestamp in
milliseconds.
+ */
+ public static Snapshot oldestAncestorAfter(Table table, Long
timestampMillis) {
+ Snapshot current = table.currentSnapshot();
+ long timestamp = timestampMillis == null ? -1L : timestampMillis;
+ if (current == null || current.timestampMillis() < timestamp) {
+ return null;
+ }
+
+ for (Snapshot snapshot : currentAncestors(table)) {
+ if (snapshot.timestampMillis() < timestamp) {
+ break;
+ }
+
+ current = snapshot;
+ }
+
+ return current;
Review comment:
I check the logic from `snapshotAfter`, and compare the logic with the
current function `oldestAncestorAfter`, I think the current implementation is
correct. In current implementation, current will point the snapshot which has
the timestamp >= targetTimestamp, the loop will break when current snapshot
points to the first available one that is greater than the targetTimestamp. It
is not like the `snapshotAfter` implementation, it is potential that there is
no `current.parentId() != snapshotId`, we throw IllegalStateException.
The implementation of `oldestAncestorAfter` will try its best effort to
find the 1st snapshot greater than targetTimestamp. If it can't find, the
result will be the oldestAncestor while the loop function exits. I don't see
any difference as call the `oldestAncestor` if it fails to find one(result
should be the same with current implementation). Please let me know if I miss
something here.
But for safety purpose, I will add `oldestAncestor` calls last when it
fails to find one, thanks for the suggestion.
--
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]
