Core: Call the replaceCurrentSnapshot with snapshotId for rollback operations

[sririshindra]

While testing rollback_to_snaphot feature, we found that this is feature is failing in some cases.

In the setSetCurrentSnapshot method 'null' is being passed for timestamp which eventually causes the failure of the following precondition check. The timeStamp should also be passed along with the Snapshot just like here

This PR fixes this bug in 0.13.x. I tested this with our internal tests and also by running the unit tests in Iceberg. This issue no longer exists in master because of 4089. The current fix is ported from here

Steps to reproduce the rollback_to_snapshot bug in spark.

Each insert operation below is done separately in its own spark app.

sql("INSERT INTO %s VALUES (3, 'zzz', 1)" % table1)    
sql("INSERT INTO %s VALUES (6, 'www', 1)" % table1)
sql("INSERT INTO %s VALUES (9, 'zyz', 1)" % table1)
sql("INSERT INTO %s VALUES (2, 'yy', 2)" % table1)
sql("INSERT INTO %s VALUES (4, 'xx', 2)" % table1)
sql("INSERT INTO %s VALUES (1, 'x', 3)" % table1)`

launch a new spark shell and follow the following steps

results, _ = sql("select id, data, id2 from %s order by id2, id" % table1) # some internal code to get the results.
results, _ = sql("select snapshot_id from %s.%s.snapshots" % (DATABASE, table1))
snapshots = self.get_rows(results) # some internal code to get snapshots in a python list.
sql("CALL spark_catalog.system.rollback_to_snapshot('%s', %s)" % (table1, str(snapshots[2][0])))`

[sririshindra]

@rdblue As you suggested in #4088 I opened this PR on top of 0.13.x branch to address the rollback issue. Please review it at your convenience.

[rdblue]

Can you port the changes from the other fix? That removed methods from TableMetadata rather than adding new ones. I think it would be best to keep 0.13.x and master in sync rather than having slightly different fixes.

Thanks, @sririshindra!

[wypoon]

@rdblue, would the following be acceptable? (from line 285)

            TableMetadata.Builder update = TableMetadata.buildFrom(base);
            if (base.snapshot(newSnapshot.snapshotId()) != null) {
              // this is a rollback or cherrypick operation
              update.setCurrentSnapshot(newSnapshot.snapshotId());
              } else if (stageOnly) {
              update.addSnapshot(newSnapshot);
            } else {
              update.setCurrentSnapshot(newSnapshot);
            }

            TableMetadata updated = update.build();
            if (updated.changes().isEmpty()) {
              // do not commit if the metadata has not changed. for example, this may happen when setting the current
              // snapshot to an ID that is already current. note that this check uses identity.
              return;
            }

I know the point of #4089 is to replace setCurrentSnapshot with setBranchSnapshot, but in 0.13.x, we don't support branching yet, and the commits #4089 builds on are not present. The above mirrors your change to SnapshotProducer in #4089.

[rdblue]

Yes, calling the right setCurrentSnapshot is a good way to backport this. Thanks!

[rdblue]

Looks good! I'll merge when tests are passing.

[sririshindra]

Looks good! I'll merge when tests are passing.

Thanks @rdblue

[wypoon]

I think this was an oversight, right? We don't need this new method; it is not used.
In fact, with the above change to SnapshotProducer, even replaceCurrentSnapshot(Snapshot) is no longer used and can be removed, I think.

[rdblue]

Yes, that's right. I missed this.

[wypoon]

I filed #4155 to remove the unused methods.