[GitHub] [iceberg] chenjunjiedada commented on a change in pull request #1384: Remove a wrong check for commit operation

[GitBox <gi...@apache.org>]

chenjunjiedada commented on a change in pull request #1384:
URL: https://github.com/apache/iceberg/pull/1384#discussion_r480016637



##########
File path: core/src/main/java/org/apache/iceberg/BaseTransaction.java
##########
@@ -451,7 +452,6 @@ public void commit(TableMetadata underlyingBase, TableMetadata metadata) {
       }
 
       BaseTransaction.this.current = metadata;
-

Review comment:
       nit: useless change.

##########
File path: core/src/test/java/org/apache/iceberg/TestCreateTransaction.java
##########
@@ -267,10 +267,6 @@ public void testCreateDetectsUncommittedChangeOnCommit() throws IOException {
         TestTables.metadataVersion("uncommitted_change"));
 
     txn.updateProperties().set("test-property", "test-value"); // not committed
-
-    AssertHelpers.assertThrows("Should reject commit when last operation has not committed",

Review comment:
       You may want to remove the entire unit test since this assertion is mainly logic for this unit test.
   
   This introduces a behaviour change which could ignore the operations that haven't been commit yet. Not sure whether the downstream client will be impacted by this or not.

##########
File path: core/src/main/java/org/apache/iceberg/RemoveSnapshots.java
##########
@@ -138,10 +138,23 @@ public ExpireSnapshots executeDeleteWith(ExecutorService executorService) {
   private TableMetadata internalApply() {
     this.base = ops.refresh();
 
-    return base.removeSnapshotsIf(snapshot ->
+    TableMetadata updated = base.removeSnapshotsIf(snapshot ->
         idsToRemove.contains(snapshot.snapshotId()) ||
         (expireOlderThan != null && snapshot.timestampMillis() < expireOlderThan &&
             !idsToRetain.contains(snapshot.snapshotId())));
+
+    List<Snapshot> updateSnapshots = updated.snapshots();
+    List<Snapshot> baseSnapshots = base.snapshots();
+    if (updateSnapshots.size() != baseSnapshots.size()) {
+      return updated;
+    } else {
+      for (int i = 0; i < baseSnapshots.size(); i += 1) {
+        if (!updateSnapshots.get(i).equals(baseSnapshots.get(i))) {

Review comment:
       Can we compare the snapshot ID？




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org