rdblue commented on a change in pull request #3775:
URL: https://github.com/apache/iceberg/pull/3775#discussion_r772632039
##########
File path: core/src/main/java/org/apache/iceberg/util/SnapshotUtil.java
##########
@@ -102,43 +102,37 @@ public static Snapshot oldestAncestor(Table table) {
}
/**
- * Traverses the history of the table's current snapshot and:
- * 1. returns null, if no snapshot exists or target timestamp is more recent
than the current snapshot.
- * 2. else return the first snapshot which satisfies {@literal >=}
targetTimestamp.
- * <p>
- * Given the snapshots (with timestamp): [S1 (10), S2 (11), S3 (12), S4 (14)]
- * <p>
- * firstSnapshotAfterTimestamp(table, x {@literal <=} 10) = S1
- * firstSnapshotAfterTimestamp(table, 11) = S2
- * firstSnapshotAfterTimestamp(table, 13) = S4
- * firstSnapshotAfterTimestamp(table, 14) = S4
- * firstSnapshotAfterTimestamp(table, x {@literal >} 14) = null
- * <p>
- * where x is the target timestamp in milliseconds and Si is the snapshot
+ * Traverses the history of the table's current snapshot and finds the first
snapshot committed after the given time.
*
* @param table a table
- * @param targetTimestampMillis a timestamp in milliseconds
- * @return the first snapshot which satisfies {@literal >=} targetTimestamp,
or null if the current snapshot is
- * more recent than the target timestamp
+ * @param timestampMillis a timestamp in milliseconds
+ * @return the first snapshot after the given timestamp, or null if the
current snapshot is older than the timestamp
+ * @throws IllegalStateException if the first ancestor after the given time
can't be determined
*/
- public static Snapshot firstSnapshotAfterTimestamp(Table table, Long
targetTimestampMillis) {
- Snapshot currentSnapshot = table.currentSnapshot();
- // Return null if no snapshot exists or target timestamp is more recent
than the current snapshot
- if (currentSnapshot == null || currentSnapshot.timestampMillis() <
targetTimestampMillis) {
+ public static Snapshot oldestAncestorAfter(Table table, long
timestampMillis) {
+ if (table.currentSnapshot() == null) {
+ // there are no snapshots or ancestors
return null;
}
- // Return the oldest snapshot which satisfies >= targetTimestamp
Snapshot lastSnapshot = null;
for (Snapshot snapshot : currentAncestors(table)) {
- if (snapshot.timestampMillis() < targetTimestampMillis) {
+ if (snapshot.timestampMillis() < timestampMillis) {
return lastSnapshot;
+ } else if (snapshot.timestampMillis() == timestampMillis) {
+ return snapshot;
Review comment:
This isn't changing the method as much as completely replacing the old
code with a different implementation that I suggested originally. I didn't want
to go through and figure out what had changed and why, I just wanted to make it
work.
The clause here catches the case where the current snapshot is the one to
return because its timestamp matches the requested timestamp. In that case,
there's no need to have an earlier parent so we short-circuit early.
--
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]
