diff --git a/core/src/main/java/org/apache/iceberg/view/BaseMetastoreViewCatalog.java b/core/src/main/java/org/apache/iceberg/view/BaseMetastoreViewCatalog.java index 4d1309a97424..f6d0da45b623 100644 --- a/core/src/main/java/org/apache/iceberg/view/BaseMetastoreViewCatalog.java +++ b/core/src/main/java/org/apache/iceberg/view/BaseMetastoreViewCatalog.java @@ -114,7 +114,25 @@ public ViewBuilder withProperty(String key, String value) { @Override public View create() { + return create(newViewOps(identifier)); + } + + @Override + public View replace() { + return replace(newViewOps(identifier)); + } + + @Override + public View createOrReplace() { ViewOperations ops = newViewOps(identifier); + if (null == ops.current()) { + return create(ops); + } else { + return replace(ops); + } + } + + private View create(ViewOperations ops) { if (null != ops.current()) { throw new AlreadyExistsException("View already exists: %s", identifier); } @@ -142,9 +160,7 @@ public View create() { return new BaseView(ops, ViewUtil.fullViewName(name(), identifier)); } - @Override - public View replace() { - ViewOperations ops = newViewOps(identifier); + private View replace(ViewOperations ops) { if (null == ops.current()) { throw new NoSuchViewException("View does not exist: %s", identifier); } @@ -177,14 +193,5 @@ public View replace() { return new BaseView(ops, ViewUtil.fullViewName(name(), identifier)); } - - @Override - public View createOrReplace() { - if (null == newViewOps(identifier).current()) { - return create(); - } else { - return replace(); - } - } } } diff --git a/core/src/main/java/org/apache/iceberg/view/ViewOperations.java b/core/src/main/java/org/apache/iceberg/view/ViewOperations.java index f9b3a9436f7f..37133161b9cc 100644 --- a/core/src/main/java/org/apache/iceberg/view/ViewOperations.java +++ b/core/src/main/java/org/apache/iceberg/view/ViewOperations.java @@ -44,13 +44,15 @@ public interface ViewOperations { * Once the atomic commit operation succeeds, implementations must not perform any operations that * may fail because failure in this method cannot be distinguished from commit failure. * - *

Implementations must throw a {@link + *

Implementations should throw a {@link * org.apache.iceberg.exceptions.CommitStateUnknownException} in cases where it cannot be * determined if the commit succeeded or failed. For example if a network partition causes the * confirmation of the commit to be lost, the implementation should throw a - * CommitStateUnknownException. This is important because downstream users of this API need to - * know whether they can clean up the commit or not, if the state is unknown then it is not safe - * to remove any files. All other exceptions will be treated as if the commit has failed. + * CommitStateUnknownException. An unknown state indicates to downstream users of this API that it + * is not safe to perform clean up and remove any files. In general, strict metadata cleanup will + * only trigger cleanups when the commit fails with an exception implementing the marker interface + * {@link org.apache.iceberg.exceptions.CleanableFailure}. All other exceptions will be treated as + * if the commit has failed. * * @param base view metadata on which changes were based * @param metadata new view metadata with updates diff --git a/core/src/main/java/org/apache/iceberg/view/ViewVersionReplace.java b/core/src/main/java/org/apache/iceberg/view/ViewVersionReplace.java index d6bd655da1b2..387771f64b2d 100644 --- a/core/src/main/java/org/apache/iceberg/view/ViewVersionReplace.java +++ b/core/src/main/java/org/apache/iceberg/view/ViewVersionReplace.java @@ -38,7 +38,7 @@ class ViewVersionReplace implements ReplaceViewVersion { private final ViewOperations ops; - private final List viewRepresentationsToAdd = Lists.newArrayList(); + private final List representations = Lists.newArrayList(); private ViewMetadata base; private Namespace defaultNamespace; private String defaultCatalog; @@ -51,28 +51,35 @@ class ViewVersionReplace implements ReplaceViewVersion { @Override public ViewVersion apply() { + this.base = ops.refresh(); + + return internalApply(base).currentVersion(); + } + + private ViewMetadata internalApply(ViewMetadata metadata) { Preconditions.checkState( - !viewRepresentationsToAdd.isEmpty(), "Cannot replace view without specifying a query"); + !representations.isEmpty(), "Cannot replace view without specifying a query"); Preconditions.checkState(null != schema, "Cannot replace view without specifying schema"); - this.base = ops.refresh(); - - ViewVersion viewVersion = base.currentVersion(); + ViewVersion viewVersion = metadata.currentVersion(); int maxVersionId = - base.versions().stream() + metadata.versions().stream() .map(ViewVersion::versionId) .max(Integer::compareTo) .orElseGet(viewVersion::versionId); - return ImmutableViewVersion.builder() - .versionId(maxVersionId + 1) - .timestampMillis(System.currentTimeMillis()) - .schemaId(schema.schemaId()) - .defaultNamespace(defaultNamespace) - .defaultCatalog(defaultCatalog) - .putSummary("operation", "replace") - .addAllRepresentations(viewRepresentationsToAdd) - .build(); + ViewVersion newVersion = + ImmutableViewVersion.builder() + .versionId(maxVersionId + 1) + .timestampMillis(System.currentTimeMillis()) + .schemaId(schema.schemaId()) + .defaultNamespace(defaultNamespace) + .defaultCatalog(defaultCatalog) + .putSummary("operation", "replace") + .addAllRepresentations(representations) + .build(); + + return ViewMetadata.buildFrom(metadata).setCurrentVersion(newVersion, schema).build(); } @Override @@ -90,25 +97,12 @@ public void commit() { base.properties(), COMMIT_TOTAL_RETRY_TIME_MS, COMMIT_TOTAL_RETRY_TIME_MS_DEFAULT), 2.0 /* exponential */) .onlyRetryOn(CommitFailedException.class) - .run( - taskOps -> { - ViewVersion newVersion = apply(); - // nothing to do if the version didn't change - if (this.base.currentVersion().equals(newVersion)) { - return; - } - - ViewMetadata updated = - ViewMetadata.buildFrom(this.base).setCurrentVersion(newVersion, schema).build(); - - taskOps.commit(base, updated); - }); + .run(taskOps -> taskOps.commit(base, internalApply(base))); } @Override public ReplaceViewVersion withQuery(String dialect, String sql) { - viewRepresentationsToAdd.add( - ImmutableSQLViewRepresentation.builder().dialect(dialect).sql(sql).build()); + representations.add(ImmutableSQLViewRepresentation.builder().dialect(dialect).sql(sql).build()); return this; } 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 e4629dc020c7..130be1fa8bdb 100644 --- a/core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java +++ b/core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java @@ -22,7 +22,6 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; -import java.util.Arrays; import org.apache.iceberg.Schema; import org.apache.iceberg.catalog.Catalog; import org.apache.iceberg.catalog.Namespace; @@ -33,7 +32,6 @@ import org.apache.iceberg.exceptions.NoSuchNamespaceException; import org.apache.iceberg.exceptions.NoSuchViewException; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; -import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; import org.apache.iceberg.types.Types; import org.assertj.core.api.Assumptions; import org.junit.jupiter.api.Test; @@ -80,8 +78,7 @@ public void basicCreateView() { assertThat(catalog().viewExists(identifier)).as("View should exist").isTrue(); // validate view settings - assertThat(view.name()).isEqualTo(catalog().name() + "." + identifier); - assertThat(view.properties()).isEmpty(); + assertThat(view.name()).isEqualTo(ViewUtil.fullViewName(catalog().name(), identifier)); assertThat(view.history()) .hasSize(1) .first() @@ -135,8 +132,8 @@ public void completeCreateView() { assertThat(catalog().viewExists(identifier)).as("View should exist").isTrue(); // validate view settings - assertThat(view.name()).isEqualTo(catalog().name() + "." + identifier); - assertThat(view.properties()).isEqualTo(ImmutableMap.of("prop1", "val1", "prop2", "val2")); + assertThat(view.name()).isEqualTo(ViewUtil.fullViewName(catalog().name(), identifier)); + assertThat(view.properties()).containsEntry("prop1", "val1").containsEntry("prop2", "val2"); assertThat(view.history()) .hasSize(1) .first() @@ -173,32 +170,32 @@ public void completeCreateView() { @Test public void createViewThatAlreadyExists() { - TableIdentifier viewIdentifier = TableIdentifier.of("ns", "view"); + TableIdentifier identifier = TableIdentifier.of("ns", "view"); if (requiresNamespaceCreate()) { - catalog().createNamespace(viewIdentifier.namespace()); + catalog().createNamespace(identifier.namespace()); } - assertThat(catalog().viewExists(viewIdentifier)).isFalse(); + assertThat(catalog().viewExists(identifier)).as("View should not exist").isFalse(); View view = catalog() - .buildView(viewIdentifier) + .buildView(identifier) .withSchema(SCHEMA) - .withDefaultNamespace(viewIdentifier.namespace()) + .withDefaultNamespace(identifier.namespace()) .withQuery("spark", "select * from ns.tbl") .create(); assertThat(view).isNotNull(); - assertThat(catalog().viewExists(viewIdentifier)).isTrue(); + assertThat(catalog().viewExists(identifier)).as("View should exist").isTrue(); assertThatThrownBy( () -> catalog() - .buildView(viewIdentifier) + .buildView(identifier) .withSchema(OTHER_SCHEMA) .withQuery("spark", "select * from ns.tbl") - .withDefaultNamespace(viewIdentifier.namespace()) + .withDefaultNamespace(identifier.namespace()) .create()) .isInstanceOf(AlreadyExistsException.class) .hasMessageStartingWith("View already exists: ns.view"); @@ -211,40 +208,16 @@ public void createViewThatAlreadyExistsAsTable() { .isNotNull(); TableIdentifier tableIdentifier = TableIdentifier.of("ns", "table"); - TableIdentifier viewIdentifier = TableIdentifier.of("ns", "view"); if (requiresNamespaceCreate()) { - catalog().createNamespace(viewIdentifier.namespace()); + catalog().createNamespace(tableIdentifier.namespace()); } - assertThat(catalog().viewExists(viewIdentifier)).isFalse(); - - View view = - catalog() - .buildView(viewIdentifier) - .withSchema(SCHEMA) - .withDefaultNamespace(viewIdentifier.namespace()) - .withQuery("spark", "select * from ns.tbl") - .create(); - - assertThat(view).isNotNull(); - assertThat(catalog().viewExists(tableIdentifier)).isFalse(); - assertThat(catalog().viewExists(viewIdentifier)).isTrue(); - - assertThatThrownBy( - () -> - catalog() - .buildView(viewIdentifier) - .withSchema(OTHER_SCHEMA) - .withQuery("spark", "select * from ns.tbl") - .withDefaultNamespace(viewIdentifier.namespace()) - .create()) - .isInstanceOf(AlreadyExistsException.class) - .hasMessageStartingWith("View already exists: ns.view"); + assertThat(tableCatalog().tableExists(tableIdentifier)).as("Table should not exist").isFalse(); tableCatalog().buildTable(tableIdentifier, SCHEMA).create(); - assertThat(tableCatalog().tableExists(tableIdentifier)).isTrue(); - assertThat(catalog().viewExists(tableIdentifier)).isFalse(); + + assertThat(tableCatalog().tableExists(tableIdentifier)).as("Table should exist").isTrue(); assertThatThrownBy( () -> @@ -264,33 +237,26 @@ public void createTableThatAlreadyExistsAsView() { .as("Only valid for catalogs that support tables") .isNotNull(); - TableIdentifier viewOne = TableIdentifier.of("ns", "viewOne"); - TableIdentifier viewTwo = TableIdentifier.of("ns", "viewTwo"); + TableIdentifier viewIdentifier = TableIdentifier.of("ns", "view"); if (requiresNamespaceCreate()) { - catalog().createNamespace(viewOne.namespace()); + catalog().createNamespace(viewIdentifier.namespace()); } - assertThat(catalog().viewExists(viewOne)).isFalse(); - assertThat(tableCatalog().tableExists(viewTwo)).isFalse(); - - for (TableIdentifier identifier : Arrays.asList(viewTwo, viewOne)) { - assertThat( - catalog() - .buildView(identifier) - .withSchema(SCHEMA) - .withDefaultNamespace(identifier.namespace()) - .withQuery("spark", "select * from ns.tbl") - .create()) - .isNotNull(); - - assertThat(catalog().viewExists(identifier)).isTrue(); - assertThat(tableCatalog().tableExists(identifier)).isFalse(); - } + assertThat(catalog().viewExists(viewIdentifier)).as("View should not exist").isFalse(); - assertThatThrownBy(() -> tableCatalog().buildTable(viewTwo, SCHEMA).create()) + catalog() + .buildView(viewIdentifier) + .withSchema(SCHEMA) + .withDefaultNamespace(viewIdentifier.namespace()) + .withQuery("spark", "select * from ns.tbl") + .create(); + + assertThat(catalog().viewExists(viewIdentifier)).as("View should exist").isTrue(); + + assertThatThrownBy(() -> tableCatalog().buildTable(viewIdentifier, SCHEMA).create()) .isInstanceOf(AlreadyExistsException.class) - .hasMessageStartingWith("View with same name already exists: ns.viewTwo"); + .hasMessageStartingWith("View with same name already exists: ns.view"); } @Test @@ -309,45 +275,23 @@ public void renameView() { .withQuery("spark", "select * from ns.tbl") .create(); - assertThat(catalog().viewExists(from)).as("View should exist").isTrue(); assertThat(catalog().listViews(from.namespace())).containsExactly(from); + assertThat(catalog().viewExists(from)).as("View should exist").isTrue(); catalog().renameView(from, to); assertThat(catalog().listViews(to.namespace())).containsExactly(to); - assertThat(catalog().viewExists(from)).as("View should not exist").isFalse(); - assertThat(catalog().viewExists(to)).as("View should exist").isTrue(); - - View view = catalog().loadView(to); - assertThat(view).isNotNull(); - - // validate view settings - assertThat(view.name()).isEqualTo(catalog().name() + "." + to); - assertThat(view.properties()).isEmpty(); - assertThat(view.history()) - .hasSize(1) - .first() - .extracting(ViewHistoryEntry::versionId) + assertThat(catalog().viewExists(from)).as("View should not exist with old name").isFalse(); + assertThat(catalog().viewExists(to)).as("View should exist with new name").isTrue(); + + // ensure current view version id didn't change after renaming + assertThat(catalog().loadView(to)) + .isNotNull() + .extracting(View::currentVersion) + .extracting(ViewVersion::versionId) .isEqualTo(1); - assertThat(view.schema().asStruct()).isEqualTo(SCHEMA.asStruct()); - assertThat(view.schemas()).hasSize(1).containsKey(SCHEMA.schemaId()); - assertThat(view.versions()).hasSize(1).containsExactly(view.currentVersion()); - - assertThat(view.currentVersion()) - .isEqualTo( - ImmutableViewVersion.builder() - .timestampMillis(view.currentVersion().timestampMillis()) - .versionId(1) - .schemaId(SCHEMA.schemaId()) - .putSummary("operation", "create") - .defaultNamespace(to.namespace()) - .addRepresentations( - ImmutableSQLViewRepresentation.builder() - .sql("select * from ns.tbl") - .dialect("spark") - .build()) - .build()); + assertThat(catalog().dropView(from)).isFalse(); assertThat(catalog().dropView(to)).isTrue(); assertThat(catalog().viewExists(to)).as("View should not exist").isFalse(); } @@ -375,38 +319,8 @@ public void renameViewUsingDifferentNamespace() { catalog().renameView(from, to); assertThat(catalog().listViews(to.namespace())).containsExactly(to); - assertThat(catalog().viewExists(from)).as("View should not exist").isFalse(); - assertThat(catalog().viewExists(to)).as("View should exist").isTrue(); - - View view = catalog().loadView(to); - assertThat(view).isNotNull(); - - // validate view settings - assertThat(view.name()).isEqualTo(catalog().name() + "." + to); - assertThat(view.properties()).isEmpty(); - assertThat(view.history()) - .hasSize(1) - .first() - .extracting(ViewHistoryEntry::versionId) - .isEqualTo(1); - assertThat(view.schema().asStruct()).isEqualTo(SCHEMA.asStruct()); - assertThat(view.schemas()).hasSize(1).containsKey(SCHEMA.schemaId()); - assertThat(view.versions()).hasSize(1).containsExactly(view.currentVersion()); - - assertThat(view.currentVersion()) - .isEqualTo( - ImmutableViewVersion.builder() - .timestampMillis(view.currentVersion().timestampMillis()) - .versionId(1) - .schemaId(SCHEMA.schemaId()) - .putSummary("operation", "create") - .defaultNamespace(from.namespace()) - .addRepresentations( - ImmutableSQLViewRepresentation.builder() - .sql("select * from ns.tbl") - .dialect("spark") - .build()) - .build()); + assertThat(catalog().viewExists(from)).as("View should not exist with old name").isFalse(); + assertThat(catalog().viewExists(to)).as("View should exist with new name").isTrue(); assertThat(catalog().dropView(from)).isFalse(); assertThat(catalog().dropView(to)).isTrue(); @@ -438,7 +352,7 @@ public void renameViewNamespaceMissing() { @Test public void renameViewSourceMissing() { - TableIdentifier from = TableIdentifier.of("ns", "view"); + TableIdentifier from = TableIdentifier.of("ns", "non_existing"); TableIdentifier to = TableIdentifier.of("ns", "renamedView"); if (requiresNamespaceCreate()) { @@ -450,40 +364,68 @@ public void renameViewSourceMissing() { assertThatThrownBy(() -> catalog().renameView(from, to)) .isInstanceOf(NoSuchViewException.class) .hasMessageContaining("View does not exist"); - - assertThat(catalog().viewExists(from)).as("View should not exist").isFalse(); - assertThat(catalog().viewExists(to)).as("View should not exist").isFalse(); } @Test - public void renameViewTargetAlreadyExists() { - TableIdentifier from = TableIdentifier.of("ns", "view"); - TableIdentifier to = TableIdentifier.of("ns", "renamedView"); + public void renameViewTargetAlreadyExistsAsView() { + TableIdentifier viewOne = TableIdentifier.of("ns", "viewOne"); + TableIdentifier viewTwo = TableIdentifier.of("ns", "viewTwo"); if (requiresNamespaceCreate()) { - catalog().createNamespace(from.namespace()); + catalog().createNamespace(viewOne.namespace()); } - for (TableIdentifier viewIdentifier : ImmutableList.of(from, to)) { + for (TableIdentifier identifier : ImmutableList.of(viewOne, viewTwo)) { + assertThat(catalog().viewExists(identifier)).as("View should not exist").isFalse(); + catalog() - .buildView(viewIdentifier) + .buildView(identifier) .withSchema(SCHEMA) - .withDefaultNamespace(from.namespace()) + .withDefaultNamespace(viewOne.namespace()) .withQuery("spark", "select * from ns.tbl") .create(); + + assertThat(catalog().viewExists(identifier)).as("View should exist").isTrue(); } - assertThatThrownBy(() -> catalog().renameView(from, to)) + assertThatThrownBy(() -> catalog().renameView(viewOne, viewTwo)) .isInstanceOf(AlreadyExistsException.class) - .hasMessageContaining("Cannot rename ns.view to ns.renamedView. View already exists"); + .hasMessageContaining("Cannot rename ns.viewOne to ns.viewTwo. View already exists"); + } + + @Test + public void renameViewTargetAlreadyExistsAsTable() { + Assumptions.assumeThat(tableCatalog()) + .as("Only valid for catalogs that support tables") + .isNotNull(); + + TableIdentifier viewIdentifier = TableIdentifier.of("ns", "view"); + TableIdentifier tableIdentifier = TableIdentifier.of("ns", "table"); + + if (requiresNamespaceCreate()) { + catalog().createNamespace(tableIdentifier.namespace()); + } + + assertThat(tableCatalog().tableExists(tableIdentifier)).as("Table should not exist").isFalse(); + + tableCatalog().buildTable(tableIdentifier, SCHEMA).create(); - // rename view where a table with the same name already exists - TableIdentifier identifier = TableIdentifier.of("ns", "tbl"); - tableCatalog().buildTable(identifier, SCHEMA).create(); + assertThat(tableCatalog().tableExists(tableIdentifier)).as("Table should exist").isTrue(); - assertThatThrownBy(() -> catalog().renameView(from, identifier)) + assertThat(catalog().viewExists(viewIdentifier)).as("View should not exist").isFalse(); + + catalog() + .buildView(viewIdentifier) + .withSchema(SCHEMA) + .withDefaultNamespace(viewIdentifier.namespace()) + .withQuery("spark", "select * from ns.tbl") + .create(); + + assertThat(catalog().viewExists(viewIdentifier)).as("View should exist").isTrue(); + + assertThatThrownBy(() -> catalog().renameView(viewIdentifier, tableIdentifier)) .isInstanceOf(AlreadyExistsException.class) - .hasMessageContaining("Cannot rename ns.view to ns.tbl. Table already exists"); + .hasMessageContaining("Cannot rename ns.view to ns.table. Table already exists"); } @Test @@ -491,7 +433,6 @@ public void listViews() { Namespace ns1 = Namespace.of("ns1"); Namespace ns2 = Namespace.of("ns2"); - TableIdentifier tableIdentifier = TableIdentifier.of(ns1, "table"); TableIdentifier view1 = TableIdentifier.of(ns1, "view1"); TableIdentifier view2 = TableIdentifier.of(ns2, "view2"); TableIdentifier view3 = TableIdentifier.of(ns2, "view3"); @@ -501,12 +442,6 @@ public void listViews() { catalog().createNamespace(ns2); } - if (null != tableCatalog()) { - tableCatalog().buildTable(tableIdentifier, SCHEMA).create(); - assertThat(tableCatalog().listTables(ns1)).containsExactly(tableIdentifier); - assertThat(tableCatalog().listTables(ns2)).isEmpty(); - } - assertThat(catalog().listViews(ns1)).isEmpty(); assertThat(catalog().listViews(ns2)).isEmpty(); @@ -540,11 +475,6 @@ public void listViews() { assertThat(catalog().listViews(ns1)).containsExactly(view1); assertThat(catalog().listViews(ns2)).containsExactlyInAnyOrder(view2, view3); - if (null != tableCatalog()) { - assertThat(tableCatalog().listTables(ns1)).containsExactly(tableIdentifier); - assertThat(tableCatalog().listTables(ns2)).isEmpty(); - } - assertThat(catalog().dropView(view2)).isTrue(); assertThat(catalog().listViews(ns1)).containsExactly(view1); assertThat(catalog().listViews(ns2)).containsExactly(view3); @@ -558,6 +488,47 @@ public void listViews() { assertThat(catalog().listViews(ns2)).isEmpty(); } + @Test + public void listViewsAndTables() { + Assumptions.assumeThat(tableCatalog()) + .as("Only valid for catalogs that support tables") + .isNotNull(); + + Namespace ns = Namespace.of("ns"); + + TableIdentifier tableIdentifier = TableIdentifier.of(ns, "table"); + TableIdentifier viewIdentifier = TableIdentifier.of(ns, "view"); + + if (requiresNamespaceCreate()) { + catalog().createNamespace(ns); + } + + assertThat(catalog().listViews(ns)).isEmpty(); + assertThat(tableCatalog().listTables(ns)).isEmpty(); + + tableCatalog().buildTable(tableIdentifier, SCHEMA).create(); + assertThat(catalog().listViews(ns)).isEmpty(); + assertThat(tableCatalog().listTables(ns)).containsExactly(tableIdentifier); + + catalog() + .buildView(viewIdentifier) + .withSchema(SCHEMA) + .withDefaultNamespace(viewIdentifier.namespace()) + .withQuery("spark", "select * from ns1.tbl") + .create(); + + assertThat(catalog().listViews(ns)).containsExactly(viewIdentifier); + assertThat(tableCatalog().listTables(ns)).containsExactly(tableIdentifier); + + assertThat(tableCatalog().dropTable(tableIdentifier)).isTrue(); + assertThat(catalog().listViews(ns)).containsExactly(viewIdentifier); + assertThat(tableCatalog().listTables(ns)).isEmpty(); + + assertThat(catalog().dropView(viewIdentifier)).isTrue(); + assertThat(catalog().listViews(ns)).isEmpty(); + assertThat(tableCatalog().listTables(ns)).isEmpty(); + } + @ParameterizedTest(name = ".createOrReplace() = {arguments}") @ValueSource(booleans = {false, true}) public void createOrReplaceView(boolean useCreateOrReplace) { @@ -580,8 +551,8 @@ public void createOrReplaceView(boolean useCreateOrReplace) { assertThat(catalog().viewExists(identifier)).as("View should exist").isTrue(); // validate view settings - assertThat(view.name()).isEqualTo(catalog().name() + "." + identifier); - assertThat(view.properties()).isEqualTo(ImmutableMap.of("prop1", "val1", "prop2", "val2")); + assertThat(view.name()).isEqualTo(ViewUtil.fullViewName(catalog().name(), identifier)); + assertThat(view.properties()).containsEntry("prop1", "val1").containsEntry("prop2", "val2"); assertThat(view.history()) .hasSize(1) .first() @@ -618,9 +589,8 @@ public void createOrReplaceView(boolean useCreateOrReplace) { View replacedView = useCreateOrReplace ? viewBuilder.createOrReplace() : viewBuilder.replace(); // validate replaced view settings - assertThat(replacedView.name()).isEqualTo(catalog().name() + "." + identifier); + assertThat(replacedView.name()).isEqualTo(ViewUtil.fullViewName(catalog().name(), identifier)); assertThat(replacedView.properties()) - .hasSize(4) .containsEntry("prop1", "val1") .containsEntry("prop2", "val2") .containsEntry("replacedProp1", "val1") @@ -681,13 +651,41 @@ public void updateViewProperties() { .withQuery("spark", "select * from ns.tbl") .create(); - assertThat(view.properties()).isEmpty(); ViewVersion viewVersion = view.currentVersion(); - assertThat(viewVersion.operation()).isEqualTo("create"); - assertThat(viewVersion.versionId()).isEqualTo(1); - assertThat(view.history()).hasSize(1); - assertThat(view.schemas()).hasSize(1).containsKey(SCHEMA.schemaId()); - assertThat(view.versions()).hasSize(1).containsExactly(viewVersion); + + view.updateProperties().set("key1", "val1").set("key2", "val2").remove("non-existing").commit(); + + View updatedView = catalog().loadView(identifier); + assertThat(updatedView.properties()) + .containsEntry("key1", "val1") + .containsEntry("key2", "val2"); + + // history and view versions should stay the same after updating view properties + assertThat(updatedView.history()).hasSize(1).isEqualTo(view.history()); + assertThat(updatedView.versions()).hasSize(1).containsExactly(viewVersion); + + assertThat(catalog().dropView(identifier)).isTrue(); + assertThat(catalog().viewExists(identifier)).as("View should not exist").isFalse(); + } + + @Test + public void updateViewPropertiesErrorCases() { + TableIdentifier identifier = TableIdentifier.of("ns", "view"); + + if (requiresNamespaceCreate()) { + catalog().createNamespace(identifier.namespace()); + } + + assertThat(catalog().viewExists(identifier)).as("View should not exist").isFalse(); + + catalog() + .buildView(identifier) + .withSchema(SCHEMA) + .withDefaultNamespace(identifier.namespace()) + .withQuery("spark", "select * from ns.tbl") + .create(); + + assertThat(catalog().viewExists(identifier)).as("View should exist").isTrue(); assertThatThrownBy( () -> catalog().loadView(identifier).updateProperties().set(null, "new-val1").commit()) @@ -716,57 +714,6 @@ public void updateViewProperties() { .commit()) .isInstanceOf(IllegalArgumentException.class) .hasMessage("Cannot remove and update the same key: key2"); - - view.updateProperties().set("key1", "val1").set("key2", "val2").remove("non-existing").commit(); - - View updatedView = catalog().loadView(identifier); - assertThat(updatedView.properties()) - .hasSize(2) - .containsEntry("key1", "val1") - .containsEntry("key2", "val2"); - assertThat(updatedView.history()).hasSize(1).isEqualTo(view.history()); - assertThat(updatedView.schemas()).hasSize(1).containsKey(SCHEMA.schemaId()); - assertThat(updatedView.versions()).hasSize(1).containsExactly(viewVersion); - - // updating properties doesn't change the view version - ViewVersion updatedViewVersion = updatedView.currentVersion(); - assertThat(updatedViewVersion).isNotNull(); - assertThat(updatedViewVersion.versionId()).isEqualTo(viewVersion.versionId()); - assertThat(updatedViewVersion.summary()).isEqualTo(viewVersion.summary()); - assertThat(updatedViewVersion.operation()).isEqualTo(viewVersion.operation()); - - assertThatThrownBy( - () -> - catalog() - .loadView(identifier) - .updateProperties() - .set("key1", "new-val1") - .set("key3", "val3") - .remove("key2") - .set("key2", "new-val2") - .commit()) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Cannot remove and update the same key: key2"); - - view.updateProperties().set("key1", "new-val1").set("key3", "val3").remove("key2").commit(); - - View updatedView2 = catalog().loadView(identifier); - assertThat(updatedView2.properties()) - .hasSize(2) - .containsEntry("key1", "new-val1") - .containsEntry("key3", "val3"); - assertThat(updatedView2.history()).hasSize(1).isEqualTo(view.history()); - assertThat(updatedView2.schemas()).hasSize(1).containsKey(SCHEMA.schemaId()); - assertThat(updatedView2.versions()).hasSize(1).containsExactly(viewVersion); - - ViewVersion updatedViewVersion2 = updatedView2.currentVersion(); - assertThat(updatedViewVersion2).isNotNull(); - assertThat(updatedViewVersion2.versionId()).isEqualTo(viewVersion.versionId()); - assertThat(updatedViewVersion2.summary()).isEqualTo(viewVersion.summary()); - assertThat(updatedViewVersion2.operation()).isEqualTo(viewVersion.operation()); - - assertThat(catalog().dropView(identifier)).isTrue(); - assertThat(catalog().viewExists(identifier)).as("View should not exist").isFalse(); } @Test @@ -800,24 +747,12 @@ public void replaceViewVersion() { .withQuery(spark.dialect(), spark.sql()) .create(); + assertThat(catalog().viewExists(identifier)).as("View should exist").isTrue(); + ViewVersion viewVersion = view.currentVersion(); - assertThat(view.properties()).isEmpty(); - assertThat(view.history()) - .hasSize(1) - .first() - .extracting(ViewHistoryEntry::versionId) - .isEqualTo(viewVersion.versionId()); - assertThat(view.history()) - .hasSize(1) - .first() - .extracting(ViewHistoryEntry::versionId) - .isEqualTo(view.currentVersion().versionId()); - assertThat(view.schemas()).hasSize(1).containsKey(SCHEMA.schemaId()); - assertThat(viewVersion.operation()).isEqualTo("create"); - assertThat(viewVersion.versionId()).isEqualTo(1); assertThat(viewVersion.representations()).hasSize(2).containsExactly(trino, spark); - assertThat(view.versions()).hasSize(1).containsExactly(viewVersion); + // uses a different schema and view representation view.replaceVersion() .withSchema(OTHER_SCHEMA) .withQuery(trino.dialect(), trino.sql()) @@ -825,15 +760,14 @@ public void replaceViewVersion() { .withDefaultNamespace(identifier.namespace()) .commit(); + // history and view versions should reflect the changes View updatedView = catalog().loadView(identifier); - assertThat(updatedView.properties()).isEmpty(); assertThat(updatedView.history()) .hasSize(2) .element(0) .extracting(ViewHistoryEntry::versionId) .isEqualTo(viewVersion.versionId()); assertThat(updatedView.history()) - .hasSize(2) .element(1) .extracting(ViewHistoryEntry::versionId) .isEqualTo(updatedView.currentVersion().versionId()); @@ -855,47 +789,71 @@ public void replaceViewVersion() { assertThat(updatedViewVersion.defaultCatalog()).isEqualTo("default"); assertThat(updatedViewVersion.defaultNamespace()).isEqualTo(identifier.namespace()); + assertThat(catalog().dropView(identifier)).isTrue(); + assertThat(catalog().viewExists(identifier)).as("View should not exist").isFalse(); + } + + @Test + public void replaceViewVersionByUpdatingSqlForDialect() { + TableIdentifier identifier = TableIdentifier.of("ns", "view"); + + if (requiresNamespaceCreate()) { + catalog().createNamespace(identifier.namespace()); + } + + assertThat(catalog().viewExists(identifier)).as("View should not exist").isFalse(); + + SQLViewRepresentation spark = + ImmutableSQLViewRepresentation.builder() + .sql("select * from ns.tbl") + .dialect("spark") + .build(); + + View view = + catalog() + .buildView(identifier) + .withSchema(SCHEMA) + .withDefaultNamespace(identifier.namespace()) + .withQuery(spark.dialect(), spark.sql()) + .create(); + + assertThat(catalog().viewExists(identifier)).as("View should exist").isTrue(); + + ViewVersion viewVersion = view.currentVersion(); + assertThat(viewVersion.representations()).hasSize(1).containsExactly(spark); + SQLViewRepresentation updatedSpark = ImmutableSQLViewRepresentation.builder() .sql("select * from ns.updated_tbl") .dialect("spark") .build(); + // only update the SQL for spark view.replaceVersion() - .withQuery(updatedSpark.dialect(), updatedSpark.sql()) + .withSchema(SCHEMA) .withDefaultNamespace(identifier.namespace()) - .withSchema(OTHER_SCHEMA) + .withQuery(updatedSpark.dialect(), updatedSpark.sql()) .commit(); - View updatedView2 = catalog().loadView(identifier); - assertThat(updatedView2.properties()).isEmpty(); - assertThat(updatedView2.history()) - .hasSize(3) + // history and view versions should reflect the changes + View updatedView = catalog().loadView(identifier); + assertThat(updatedView.history()) + .hasSize(2) .element(0) .extracting(ViewHistoryEntry::versionId) .isEqualTo(viewVersion.versionId()); - assertThat(updatedView2.history()) + assertThat(updatedView.history()) .element(1) .extracting(ViewHistoryEntry::versionId) - .isEqualTo(updatedViewVersion.versionId()); - assertThat(updatedView2.history()) - .element(2) - .extracting(ViewHistoryEntry::versionId) - .isEqualTo(updatedView2.currentVersion().versionId()); - assertThat(updatedView.schemas()) + .isEqualTo(updatedView.currentVersion().versionId()); + assertThat(updatedView.versions()) .hasSize(2) - .containsKey(SCHEMA.schemaId()) - .containsKey(OTHER_SCHEMA.schemaId()); - assertThat(updatedView2.versions()) - .hasSize(3) - .containsExactly(viewVersion, updatedViewVersion, updatedView2.currentVersion()); + .containsExactly(viewVersion, updatedView.currentVersion()); - ViewVersion updatedViewVersion2 = updatedView2.currentVersion(); - assertThat(updatedViewVersion2).isNotNull(); - assertThat(updatedViewVersion2.versionId()).isEqualTo(updatedViewVersion.versionId() + 1); - assertThat(updatedViewVersion2.summary()).hasSize(1).containsEntry("operation", "replace"); - assertThat(updatedViewVersion2.operation()).isEqualTo("replace"); - assertThat(updatedViewVersion2.representations()).hasSize(1).containsExactly(updatedSpark); + // updated view should have the new SQL + assertThat(updatedView.currentVersion().representations()) + .hasSize(1) + .containsExactly(updatedSpark); assertThat(catalog().dropView(identifier)).isTrue(); assertThat(catalog().viewExists(identifier)).as("View should not exist").isFalse(); @@ -925,6 +883,8 @@ public void replaceViewVersionErrorCases() { .withQuery(trino.dialect(), trino.sql()) .create(); + assertThat(catalog().viewExists(identifier)).as("View should not exist").isTrue(); + // empty commits are not allowed assertThatThrownBy(() -> view.replaceVersion().commit()) .isInstanceOf(IllegalStateException.class)