review feedback

apache · Nov 2, 2023 · 626dedc · 626dedc
1 parent bdbed42
commit 626dedc
Show file tree

Hide file tree

Showing 3 changed files with 94 additions and 6 deletions.
diff --git a/core/src/main/java/org/apache/iceberg/UpdateRequirement.java b/core/src/main/java/org/apache/iceberg/UpdateRequirement.java
@@ -19,6 +19,7 @@
 package org.apache.iceberg;
 
 import org.apache.iceberg.exceptions.CommitFailedException;
+import org.apache.iceberg.exceptions.ValidationException;
 import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
 import org.apache.iceberg.view.ViewMetadata;
 
@@ -27,8 +28,8 @@ public interface UpdateRequirement {
   void validate(TableMetadata base);
 
   default void validate(ViewMetadata base) {
-    throw new UnsupportedOperationException(
-        String.format("Cannot validate %s against a view", this.getClass().getSimpleName()));
+    throw new ValidationException(
+        "Cannot validate %s against a view", this.getClass().getSimpleName());
   }
 
   class AssertTableDoesNotExist implements UpdateRequirement {

diff --git a/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java b/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java
@@ -387,8 +387,9 @@ static TableMetadata commit(TableOperations ops, UpdateTableRequest request) {
     return ops.current();
   }
 
-  private static BaseView baseView(View view) {
-    Preconditions.checkArgument(view instanceof BaseView, "View must be a BaseView");
+  private static BaseView asBaseView(View view) {
+    Preconditions.checkState(
+        view instanceof BaseView, "Cannot wrap catalog that does not produce BaseView");
     return (BaseView) view;
   }
 
@@ -431,7 +432,7 @@ public static LoadViewResponse createView(
   }
 
   private static LoadViewResponse viewResponse(View view) {
-    ViewMetadata metadata = baseView(view).operations().current();
+    ViewMetadata metadata = asBaseView(view).operations().current();
     return ImmutableLoadViewResponse.builder()
         .metadata(metadata)
         .metadataLocation(metadata.metadataFileLocation())
@@ -446,7 +447,7 @@ public static LoadViewResponse loadView(ViewCatalog catalog, TableIdentifier vie
   public static LoadViewResponse updateView(
       ViewCatalog catalog, TableIdentifier ident, UpdateTableRequest request) {
     View view = catalog.loadView(ident);
-    ViewMetadata metadata = commit(baseView(view).operations(), request);
+    ViewMetadata metadata = commit(asBaseView(view).operations(), request);
 
     return ImmutableLoadViewResponse.builder()
         .metadata(metadata)

diff --git a/core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java b/core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java
@@ -1535,4 +1535,90 @@ public void updateViewLocationConflict() {
         .isInstanceOf(NoSuchViewException.class)
         .hasMessageContaining("View does not exist: ns.view");
   }
+
+  @Test
+  public void concurrentReplaceViewVersion() {
+    TableIdentifier identifier = TableIdentifier.of("ns", "view");
+
+    if (requiresNamespaceCreate()) {
+      catalog().createNamespace(identifier.namespace());
+    }
+
+    assertThat(catalog().viewExists(identifier)).as("View should not exist").isFalse();
+
+    View view =
+        catalog()
+            .buildView(identifier)
+            .withSchema(SCHEMA)
+            .withDefaultNamespace(identifier.namespace())
+            .withQuery("trino", "select * from ns.tbl")
+            .create();
+
+    assertThat(catalog().viewExists(identifier)).as("View should exist").isTrue();
+
+    ReplaceViewVersion replaceViewVersion =
+        view.replaceVersion()
+            .withQuery("trino", "select count(*) from ns.tbl")
+            .withSchema(OTHER_SCHEMA)
+            .withDefaultNamespace(identifier.namespace());
+
+    ReplaceViewVersion replaceViewVersionConcurrent =
+        view.replaceVersion()
+            .withQuery("spark", "select count(*) from ns.tbl")
+            .withSchema(OTHER_SCHEMA)
+            .withDefaultNamespace(identifier.namespace());
+
+    // concurrently replace the view version, the last replace wins
+    replaceViewVersionConcurrent.commit();
+    replaceViewVersion.commit();
+
+    View updatedView = catalog().loadView(identifier);
+    ViewVersion viewVersion = updatedView.currentVersion();
+    assertThat(viewVersion.versionId()).isEqualTo(3);
+    assertThat(updatedView.versions()).hasSize(3);
+    assertThat(updatedView.version(1))
+        .isEqualTo(
+            ImmutableViewVersion.builder()
+                .timestampMillis(updatedView.version(1).timestampMillis())
+                .versionId(1)
+                .schemaId(0)
+                .summary(updatedView.version(1).summary())
+                .defaultNamespace(identifier.namespace())
+                .addRepresentations(
+                    ImmutableSQLViewRepresentation.builder()
+                        .sql("select * from ns.tbl")
+                        .dialect("trino")
+                        .build())
+                .build());
+
+    assertThat(updatedView.version(2))
+        .isEqualTo(
+            ImmutableViewVersion.builder()
+                .timestampMillis(updatedView.version(2).timestampMillis())
+                .versionId(2)
+                .schemaId(1)
+                .summary(updatedView.version(2).summary())
+                .defaultNamespace(identifier.namespace())
+                .addRepresentations(
+                    ImmutableSQLViewRepresentation.builder()
+                        .sql("select count(*) from ns.tbl")
+                        .dialect("spark")
+                        .build())
+                .build());
+
+    assertThat(updatedView.version(3))
+        .isEqualTo(
+            ImmutableViewVersion.builder()
+                .timestampMillis(updatedView.version(3).timestampMillis())
+                .versionId(3)
+                .schemaId(1)
+                .summary(updatedView.version(3).summary())
+                .defaultNamespace(identifier.namespace())
+                .addRepresentations(
+                    ImmutableSQLViewRepresentation.builder()
+                        .sql("select count(*) from ns.tbl")
+                        .dialect("trino")
+                        .build())
+                .build());
+  }
 }