Core: Fix create v1 table on REST Catalog

apache · May 23, 2024 · 0a60c55 · 0a60c55
1 parent b3c25fb
commit 0a60c55
Show file tree

Hide file tree

Showing 3 changed files with 78 additions and 2 deletions.
diff --git a/core/src/main/java/org/apache/iceberg/TableMetadata.java b/core/src/main/java/org/apache/iceberg/TableMetadata.java
@@ -29,6 +29,7 @@
 import java.util.function.Predicate;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
+import org.apache.iceberg.MetadataUpdate.UpgradeFormatVersion;
 import org.apache.iceberg.exceptions.ValidationException;
 import org.apache.iceberg.relocated.com.google.common.base.MoreObjects;
 import org.apache.iceberg.relocated.com.google.common.base.Objects;
@@ -861,6 +862,19 @@ public static Builder buildFromEmpty() {
     return new Builder();
   }
 
+  public static Builder buildFromMetadataUpdates(List<MetadataUpdate> metadataUpdates) {
+    Builder builder = new Builder();
+    metadataUpdates.forEach(
+        update -> {
+          if (update instanceof UpgradeFormatVersion) {
+            builder.setInitialFormatVersion(((UpgradeFormatVersion) update).formatVersion());
+          } else {
+            update.applyTo(builder);
+          }
+        });
+    return builder;
+  }
+
   public static class Builder {
     private static final int LAST_ADDED = -1;
 

diff --git a/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java b/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java
@@ -374,8 +374,7 @@ private static TableMetadata create(TableOperations ops, UpdateTableRequest requ
     // the only valid requirement is that the table will be created
     request.requirements().forEach(requirement -> requirement.validate(ops.current()));
 
-    TableMetadata.Builder builder = TableMetadata.buildFromEmpty();
-    request.updates().forEach(update -> update.applyTo(builder));
+    TableMetadata.Builder builder = TableMetadata.buildFromMetadataUpdates(request.updates());
 
     // create transactions do not retry. if the table exists, retrying is not a solution
     ops.commit(null, builder.build());

diff --git a/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java b/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java
@@ -20,6 +20,7 @@
 
 import static org.apache.iceberg.types.Types.NestedField.required;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatNoException;
 import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
@@ -2655,6 +2656,68 @@ public void testCleanupCleanableExceptionsReplace() {
         .isInstanceOf(NotFoundException.class);
   }
 
+  @ParameterizedTest
+  @ValueSource(strings = {"1", "2"})
+  public void testCreateTableTransaction(String formatVersion) {
+    RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog));
+    RESTCatalog catalog = catalog(adapter);
+
+    if (requiresNamespaceCreate()) {
+      catalog.createNamespace(NS);
+    }
+
+    // Test create table transaction with no data file
+    Transaction createTableTransaction1 =
+        catalog.newCreateTableTransaction(
+            TableIdentifier.of(NS, "table1"),
+            SCHEMA,
+            PartitionSpec.unpartitioned(),
+            ImmutableMap.of("format-version", formatVersion));
+    assertThatNoException().isThrownBy(() -> createTableTransaction1.commitTransaction());
+
+    // Test create table transaction with data files
+    Transaction createTableTransaction2 =
+        catalog.newCreateTableTransaction(
+            TableIdentifier.of(NS, "table2"),
+            SCHEMA,
+            PartitionSpec.unpartitioned(),
+            ImmutableMap.of("format-version", formatVersion));
+    createTableTransaction2.newAppend().appendFile(FILE_A).commit();
+    assertThatNoException().isThrownBy(() -> createTableTransaction2.commitTransaction());
+  }
+
+  @ParameterizedTest
+  @ValueSource(strings = {"1", "2"})
+  public void testReplaceTableTransaction(String formatVersion) {
+    RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog));
+    RESTCatalog catalog = catalog(adapter);
+
+    if (requiresNamespaceCreate()) {
+      catalog.createNamespace(NS);
+    }
+
+    // Test replace table transaction with no data file
+    Transaction replaceTableTransaction1 =
+        catalog.newReplaceTableTransaction(
+            TableIdentifier.of(NS, "table1"),
+            SCHEMA,
+            PartitionSpec.unpartitioned(),
+            ImmutableMap.of("format-version", formatVersion),
+            true);
+    assertThatNoException().isThrownBy(() -> replaceTableTransaction1.commitTransaction());
+
+    // Test replace table transaction with data files
+    Transaction replaceTableTransaction2 =
+        catalog.newReplaceTableTransaction(
+            TableIdentifier.of(NS, "table2"),
+            SCHEMA,
+            PartitionSpec.unpartitioned(),
+            ImmutableMap.of("format-version", formatVersion),
+            true);
+    replaceTableTransaction2.newAppend().appendFile(FILE_A).commit();
+    assertThatNoException().isThrownBy(() -> replaceTableTransaction2.commitTransaction());
+  }
+
   @Test
   public void testNoCleanupForNonCleanableReplaceTransaction() {
     RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog));