(iceberg) branch main updated: Core: Don't create empty RemovePartitionSpecs MetadataUpdate (#12465)

[et...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

etudenhoefner pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg.git


The following commit(s) were added to refs/heads/main by this push:
     new 5994b54de4 Core: Don't create empty RemovePartitionSpecs MetadataUpdate (#12465)
5994b54de4 is described below

commit 5994b54de4675e5c40e8f8cd4cf5be48620f09fb
Author: gaborkaszab <ga...@cloudera.com>
AuthorDate: Thu Mar 6 13:27:09 2025 +0100

    Core: Don't create empty RemovePartitionSpecs MetadataUpdate (#12465)
    
    Now if we run an expireSnapshots with the cleanExpiredMetadata() option
    we create a RemovePartitionSpecs update regardless if there are specs to
    be removed or not.
    This change prevents creating such an update if there are no specs to be
    removed.
---
 core/src/main/java/org/apache/iceberg/TableMetadata.java | 12 +++++++-----
 .../java/org/apache/iceberg/TestRemoveSnapshots.java     | 13 ++++++++++---
 .../test/java/org/apache/iceberg/TestTableMetadata.java  | 16 ++++++++++++++++
 3 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/core/src/main/java/org/apache/iceberg/TableMetadata.java b/core/src/main/java/org/apache/iceberg/TableMetadata.java
index c4a328dd79..61a127fed6 100644
--- a/core/src/main/java/org/apache/iceberg/TableMetadata.java
+++ b/core/src/main/java/org/apache/iceberg/TableMetadata.java
@@ -1163,11 +1163,13 @@ public class TableMetadata implements Serializable {
       Preconditions.checkArgument(
           !specIdsToRemove.contains(defaultSpecId), "Cannot remove the default partition spec");
 
-      this.specs =
-          specs.stream()
-              .filter(s -> !specIdsToRemove.contains(s.specId()))
-              .collect(Collectors.toList());
-      changes.add(new MetadataUpdate.RemovePartitionSpecs(specIdsToRemove));
+      if (!specIdsToRemove.isEmpty()) {
+        this.specs =
+            specs.stream()
+                .filter(s -> !specIdsToRemove.contains(s.specId()))
+                .collect(Collectors.toList());
+        changes.add(new MetadataUpdate.RemovePartitionSpecs(specIdsToRemove));
+      }
 
       return this;
     }
diff --git a/core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java b/core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java
index 0a1c9cf215..d60cba9ee2 100644
--- a/core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java
+++ b/core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java
@@ -1740,8 +1740,8 @@ public class TestRemoveSnapshots extends TestBase {
   }
 
   @TestTemplate
-  public void testNoSchemasToRemove() {
-    String tableName = "test_no_schemas_to_remove";
+  public void testNoSchemasOrSpecsToRemove() {
+    String tableName = "test_no_schemas_or_specs_to_remove";
     TestTables.TestTableOperations ops =
         Mockito.spy(new TestTables.TestTableOperations(tableName, tableDir));
     TestTables.TestTable table =
@@ -1762,7 +1762,7 @@ public class TestRemoveSnapshots extends TestBase {
     table.newAppend().appendFile(FILE_B).commit();
 
     Set<String> deletedFiles = Sets.newHashSet();
-    // Expire all snapshots except the current one. No unused schemas to be removed.
+    // Expire all snapshots except the current one. No unused schemas or specs to be removed.
     removeSnapshots(table)
         .expireOlderThan(System.currentTimeMillis())
         .cleanExpiredMetadata(true)
@@ -1771,6 +1771,13 @@ public class TestRemoveSnapshots extends TestBase {
 
     assertThat(deletedFiles).containsExactlyInAnyOrderElementsOf(expectedDeletedFiles);
     assertThat(table.schemas().values()).containsExactly(table.schema());
+    Mockito.verify(ops, Mockito.never())
+        .commit(
+            any(),
+            argThat(
+                meta ->
+                    meta.changes().stream()
+                        .anyMatch(u -> u instanceof MetadataUpdate.RemovePartitionSpecs)));
     Mockito.verify(ops, Mockito.never())
         .commit(
             any(),
diff --git a/core/src/test/java/org/apache/iceberg/TestTableMetadata.java b/core/src/test/java/org/apache/iceberg/TestTableMetadata.java
index 07b4b05916..bff9e2a3a9 100644
--- a/core/src/test/java/org/apache/iceberg/TestTableMetadata.java
+++ b/core/src/test/java/org/apache/iceberg/TestTableMetadata.java
@@ -1859,6 +1859,22 @@ public class TestTableMetadata {
     assertThat(updatedMetadata.previousFiles()).isEmpty();
   }
 
+  @Test
+  public void testMetadataWithRemoveSpecs() {
+    TableMetadata meta =
+        TableMetadata.buildFrom(
+                TableMetadata.newTableMetadata(
+                    TestBase.SCHEMA, PartitionSpec.unpartitioned(), null, ImmutableMap.of()))
+            .removeSpecs(Sets.newHashSet())
+            .build();
+
+    assertThat(meta.changes()).noneMatch(u -> u instanceof MetadataUpdate.RemovePartitionSpecs);
+
+    meta = TableMetadata.buildFrom(meta).removeSpecs(Sets.newHashSet(1, 2)).build();
+
+    assertThat(meta.changes()).anyMatch(u -> u instanceof MetadataUpdate.RemovePartitionSpecs);
+  }
+
   @Test
   public void testMetadataWithRemoveSchemas() {
     TableMetadata meta =