rdblue commented on code in PR #14504:
URL: https://github.com/apache/iceberg/pull/14504#discussion_r2504235503
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -1313,17 +1312,14 @@ public Builder setRef(String name, SnapshotRef ref) {
Snapshot snapshot = snapshotsById.get(snapshotId);
ValidationException.check(
snapshot != null, "Cannot set %s to unknown snapshot: %s", name,
snapshotId);
- if (isAddedSnapshot(snapshotId)) {
- this.lastUpdatedMillis = snapshot.timestampMillis();
- }
if (SnapshotRef.MAIN_BRANCH.equals(name)) {
this.currentSnapshotId = ref.snapshotId();
if (lastUpdatedMillis == null) {
this.lastUpdatedMillis = System.currentTimeMillis();
}
- snapshotLog.add(new SnapshotLogEntry(lastUpdatedMillis,
ref.snapshotId()));
+ snapshotLog.add(new SnapshotLogEntry(snapshot.timestampMillis(),
snapshot.snapshotId()));
Review Comment:
I don't think that this is correct. Whether to use the snapshot's timestamp
in the log entry should still depend on whether the operation is a rollback or
an added snapshot, so this needs to continue to check `isAddedSnapshot`.
I don't think that this PR should change any logic other than the snapshot
used for table metadata.
--
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]
