SnapshotManager can't handle rollback calls that would pick the current snapshot as target snapshot.

[szlta]

Apache Iceberg version

0.14.0 (latest release)

Query engine

No response

Please describe the bug 🐞

Example test case for TestSnapshotManager.java:

  @Test
  public void testAttemptToRollbackToCurrentSnapshot() {
    table.newAppend().appendFile(FILE_A).commit();

    long currentSnapshotTimestampPlus100 = table.currentSnapshot().timestampMillis() + 100;
    table
        .manageSnapshots()
        .rollbackToTime(currentSnapshotTimestampPlus100)
        .commit();

    long currentSnapshotId = table.currentSnapshot().snapshotId();
    table
        .manageSnapshots()
        .rollbackTo(currentSnapshotId)
        .commit();
  }

The above case fails with

Cannot commit transaction: last operation has not committed
java.lang.IllegalStateException: Cannot commit transaction: last operation has not committed
	at org.apache.iceberg.relocated.com.google.common.base.Preconditions.checkState(Preconditions.java:502)
	at org.apache.iceberg.BaseTransaction.commitTransaction(BaseTransaction.java:252)
	at org.apache.iceberg.SnapshotManager.commit(SnapshotManager.java:158)
	...

I think in such cases the API should still return without errors, even if the request turned out to be a no-op. This was working well in 0.13.0, but since 0.14.0 it is failing due to the refactor in this commit: https://github.com/apache/iceberg/commit/e41e8de673628baa6d641f5ad6b03680c18cc0fe

The reason for the failure is that SetSnapshotOperation#commit() doesn't commit on taskOps at https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/SetSnapshotOperation.java#L126-L137 leaving https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/BaseTransaction.java#L105 in false state and eventually causing the transaction commit to throw the above exception.

IMHO there isn't a good reason for SetSnapshotOperation#commit() to return without committing in such cases as the commitTransaction() invocation would do a similar logic later anyway. So I think the solution would be to remove this "return if no changes" logic from SetSnapshotOperation. @rdblue - as the original contributor, what's your opinion? cc: @lcspinter @pvary