diff --git a/core/src/main/java/org/apache/iceberg/inmemory/InMemoryCatalog.java b/core/src/main/java/org/apache/iceberg/inmemory/InMemoryCatalog.java index 566e56eec9b5..51d242f93419 100644 --- a/core/src/main/java/org/apache/iceberg/inmemory/InMemoryCatalog.java +++ b/core/src/main/java/org/apache/iceberg/inmemory/InMemoryCatalog.java @@ -165,6 +165,10 @@ public void renameTable(TableIdentifier from, TableIdentifier to) { throw new AlreadyExistsException("Cannot rename %s to %s. Table already exists", from, to); } + if (views.containsKey(to)) { + throw new AlreadyExistsException("Cannot rename %s to %s. View already exists", from, to); + } + tables.put(to, fromLocation); tables.remove(from); } @@ -404,6 +408,10 @@ public void doCommit(TableMetadata base, TableMetadata metadata) { throw new AlreadyExistsException("Table already exists: %s", tableName()); } + if (null == existingLocation) { + throw new NoSuchTableException("Table does not exist: %s", tableName()); + } + throw new CommitFailedException( "Cannot commit to table %s metadata location from %s to %s " + "because it has been concurrently modified to %s", @@ -470,6 +478,10 @@ public void doCommit(ViewMetadata base, ViewMetadata metadata) { throw new AlreadyExistsException("View already exists: %s", identifier); } + if (null == existingLocation) { + throw new NoSuchViewException("View does not exist: %s", identifier); + } + throw new CommitFailedException( "Cannot commit to view %s metadata location from %s to %s " + "because it has been concurrently modified to %s", diff --git a/core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java b/core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java index 66de4d92ca4f..f35511de8ed0 100644 --- a/core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java +++ b/core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java @@ -22,6 +22,8 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import java.nio.file.Path; +import java.nio.file.Paths; import org.apache.iceberg.Schema; import org.apache.iceberg.Transaction; import org.apache.iceberg.UpdateLocation; @@ -31,7 +33,6 @@ import org.apache.iceberg.catalog.TableIdentifier; import org.apache.iceberg.catalog.ViewCatalog; import org.apache.iceberg.exceptions.AlreadyExistsException; -import org.apache.iceberg.exceptions.CommitFailedException; import org.apache.iceberg.exceptions.NoSuchNamespaceException; import org.apache.iceberg.exceptions.NoSuchTableException; import org.apache.iceberg.exceptions.NoSuchViewException; @@ -39,6 +40,7 @@ import org.apache.iceberg.types.Types; import org.assertj.core.api.Assumptions; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; @@ -56,10 +58,16 @@ public abstract class ViewCatalogTests tableCatalog().renameTable(tableIdentifier, viewIdentifier)) + .isInstanceOf(AlreadyExistsException.class) + .hasMessageContaining("Cannot rename ns.table to ns.view. View already exists"); + } + @Test public void listViews() { Namespace ns1 = Namespace.of("ns1"); @@ -789,6 +842,7 @@ public void createOrReplaceView(boolean useCreateOrReplace) { View view = useCreateOrReplace ? viewBuilder.createOrReplace() : viewBuilder.create(); assertThat(catalog().viewExists(identifier)).as("View should exist").isTrue(); + assertThat(((BaseView) view).operations().current().metadataFileLocation()).isNotNull(); ViewVersion viewVersion = view.currentVersion(); assertThat(viewVersion.representations()) @@ -810,6 +864,7 @@ public void createOrReplaceView(boolean useCreateOrReplace) { // validate replaced view settings assertThat(replacedView.name()).isEqualTo(ViewUtil.fullViewName(catalog().name(), identifier)); + assertThat(((BaseView) replacedView).operations().current().metadataFileLocation()).isNotNull(); assertThat(replacedView.properties()) .containsEntry("prop1", "val1") .containsEntry("prop2", "val2") @@ -1234,8 +1289,8 @@ public void updateViewPropertiesConflict() { assertThat(catalog().viewExists(identifier)).as("View should not exist").isFalse(); assertThatThrownBy(() -> updateViewProperties.set("key1", "val1").commit()) - .isInstanceOf(CommitFailedException.class) - .hasMessageContaining("Cannot commit"); + .isInstanceOf(NoSuchViewException.class) + .hasMessageContaining("View does not exist: ns.view"); } @Test @@ -1270,8 +1325,8 @@ public void replaceViewVersionConflict() { .withSchema(SCHEMA) .withDefaultNamespace(identifier.namespace()) .commit()) - .isInstanceOf(CommitFailedException.class) - .hasMessageContaining("Cannot commit"); + .isInstanceOf(NoSuchViewException.class) + .hasMessageContaining("View does not exist: ns.view"); } @Test @@ -1351,28 +1406,42 @@ public void createAndReplaceViewWithLocation() { assertThat(catalog().viewExists(identifier)).as("View should not exist").isFalse(); + String location = + Paths.get(tempDir.toUri().toString(), Paths.get("ns", "view").toString()).toString(); View view = catalog() .buildView(identifier) .withSchema(SCHEMA) .withDefaultNamespace(identifier.namespace()) .withQuery("trino", "select * from ns.tbl") - .withLocation("file://tmp/ns/view") + .withLocation(location) .create(); assertThat(catalog().viewExists(identifier)).as("View should exist").isTrue(); - assertThat(view.location()).isEqualTo("file://tmp/ns/view"); + if (!overridesRequestedLocation()) { + assertThat(view.location()).isEqualTo(location); + } else { + assertThat(view.location()).isNotNull(); + } + + String updatedLocation = + Paths.get(tempDir.toUri().toString(), Paths.get("updated", "ns", "view").toString()) + .toString(); view = catalog() .buildView(identifier) .withSchema(SCHEMA) .withDefaultNamespace(identifier.namespace()) .withQuery("trino", "select * from ns.tbl") - .withLocation("file://updated_tmp/ns/view") + .withLocation(updatedLocation) .replace(); - assertThat(view.location()).isEqualTo("file://updated_tmp/ns/view"); + if (!overridesRequestedLocation()) { + assertThat(view.location()).isEqualTo(updatedLocation); + } else { + assertThat(view.location()).isNotNull(); + } assertThat(catalog().dropView(identifier)).isTrue(); assertThat(catalog().viewExists(identifier)).as("View should not exist").isFalse(); @@ -1388,23 +1457,36 @@ public void updateViewLocation() { assertThat(catalog().viewExists(identifier)).as("View should not exist").isFalse(); + String location = + Paths.get(tempDir.toUri().toString(), Paths.get("ns", "view").toString()).toString(); View view = catalog() .buildView(identifier) .withSchema(SCHEMA) .withDefaultNamespace(identifier.namespace()) .withQuery("trino", "select * from ns.tbl") - .withLocation("file://tmp/ns/view") + .withLocation(location) .create(); assertThat(catalog().viewExists(identifier)).as("View should exist").isTrue(); - assertThat(view.location()).isEqualTo("file://tmp/ns/view"); + if (!overridesRequestedLocation()) { + assertThat(view.location()).isEqualTo(location); + } else { + assertThat(view.location()).isNotNull(); + } - view.updateLocation().setLocation("file://updated_tmp/ns/view").commit(); + String updatedLocation = + Paths.get(tempDir.toUri().toString(), Paths.get("updated", "ns", "view").toString()) + .toString(); + view.updateLocation().setLocation(updatedLocation).commit(); View updatedView = catalog().loadView(identifier); - assertThat(updatedView.location()).isEqualTo("file://updated_tmp/ns/view"); + if (!overridesRequestedLocation()) { + assertThat(updatedView.location()).isEqualTo(updatedLocation); + } else { + assertThat(view.location()).isNotNull(); + } // history and view versions should stay the same after updating view properties assertThat(updatedView.history()).hasSize(1).isEqualTo(view.history()); @@ -1446,7 +1528,7 @@ public void updateViewLocationConflict() { // the view was already dropped concurrently assertThatThrownBy(() -> updateViewLocation.setLocation("new-location").commit()) - .isInstanceOf(CommitFailedException.class) - .hasMessageContaining("Cannot commit"); + .isInstanceOf(NoSuchViewException.class) + .hasMessageContaining("View does not exist: ns.view"); } }