Skip to content

Commit

Permalink
Fix test
Browse files Browse the repository at this point in the history
  • Loading branch information
@ajantha-bhat
ajantha-bhat committed Oct 18, 2023
1 parent b2a781b commit f9779c0
Show file tree
Hide file tree
Showing 6 changed files with 274 additions and 45 deletions.
124 changes: 106 additions & 18 deletions core/src/test/java/org/apache/iceberg/view/ViewCatalogTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ protected boolean requiresNamespaceCreate() {
return false;
}

protected boolean overridesRequestedLocation() {
return false;
}

@Test
public void basicCreateView() {
TableIdentifier identifier = TableIdentifier.of("ns", "view");
Expand All @@ -80,6 +84,7 @@ public void basicCreateView() {

assertThat(view).isNotNull();
assertThat(catalog().viewExists(identifier)).as("View should exist").isTrue();
assertThat(((BaseView) view).operations().current().metadataFileLocation()).isNotNull();

// validate view settings
assertThat(view.name()).isEqualTo(ViewUtil.fullViewName(catalog().name(), identifier));
Expand Down Expand Up @@ -132,15 +137,21 @@ public void completeCreateView() {
.withQuery("trino", "select * from ns.tbl using X")
.withProperty("prop1", "val1")
.withProperty("prop2", "val2")
.withLocation("file://tmp/ns/view")
.withLocation("file:///tmp/ns/view")
.create();

assertThat(view).isNotNull();
assertThat(catalog().viewExists(identifier)).as("View should exist").isTrue();
assertThat(((BaseView) view).operations().current().metadataFileLocation()).isNotNull();

if (!overridesRequestedLocation()) {
assertThat(view.location()).isEqualTo("file:///tmp/ns/view");
} else {
assertThat(view.location()).isNotNull();
}

// validate view settings
assertThat(view.name()).isEqualTo(ViewUtil.fullViewName(catalog().name(), identifier));
assertThat(view.location()).isEqualTo("file://tmp/ns/view");
assertThat(view.properties()).containsEntry("prop1", "val1").containsEntry("prop2", "val2");
assertThat(view.history())
.hasSize(1)
Expand Down Expand Up @@ -225,8 +236,9 @@ public void createViewErrorCases() {
.withQuery(trino.dialect(), trino.sql())
.withQuery(trino.dialect(), trino.sql())
.create())
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid view version: Cannot add multiple queries for dialect trino");
.isInstanceOf(Exception.class)
.hasMessageContaining(
"Invalid view version: Cannot add multiple queries for dialect trino");
}

@Test
Expand Down Expand Up @@ -502,6 +514,7 @@ public void renameView() {
assertThat(catalog().viewExists(from)).as("View should exist").isTrue();

ViewMetadata original = ((BaseView) view).operations().current();
assertThat(original.metadataFileLocation()).isNotNull();

catalog().renameView(from, to);

Expand Down Expand Up @@ -664,6 +677,41 @@ public void renameViewTargetAlreadyExistsAsTable() {
.hasMessageContaining("Cannot rename ns.view to ns.table. Table already exists");
}

@Test
public void renameTableTargetAlreadyExistsAsView() {
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();

assertThat(tableCatalog().tableExists(tableIdentifier)).as("Table should exist").isTrue();

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(() -> 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");
Expand Down Expand Up @@ -787,6 +835,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())
Expand All @@ -808,6 +857,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")
Expand Down Expand Up @@ -1234,8 +1284,15 @@ 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");
.satisfiesAnyOf(
x ->
assertThat(x)
.isInstanceOf(NoSuchViewException.class)
.hasMessageContaining("View does not exist: ns.view"),
x ->
assertThat(x)
.isInstanceOf(CommitFailedException.class)
.hasMessageContaining("Cannot commit"));
}

@Test
Expand Down Expand Up @@ -1270,8 +1327,15 @@ public void replaceViewVersionConflict() {
.withSchema(SCHEMA)
.withDefaultNamespace(identifier.namespace())
.commit())
.isInstanceOf(CommitFailedException.class)
.hasMessageContaining("Cannot commit");
.satisfiesAnyOf(
x ->
assertThat(x)
.isInstanceOf(NoSuchViewException.class)
.hasMessageContaining("View does not exist: ns.view"),
x ->
assertThat(x)
.isInstanceOf(CommitFailedException.class)
.hasMessageContaining("Cannot commit"));
}

@Test
Expand Down Expand Up @@ -1357,22 +1421,31 @@ public void createAndReplaceViewWithLocation() {
.withSchema(SCHEMA)
.withDefaultNamespace(identifier.namespace())
.withQuery("trino", "select * from ns.tbl")
.withLocation("file://tmp/ns/view")
.withLocation("file:///tmp/ns/view")
.create();

assertThat(catalog().viewExists(identifier)).as("View should exist").isTrue();
assertThat(view.location()).isEqualTo("file://tmp/ns/view");

if (!overridesRequestedLocation()) {
assertThat(view.location()).isEqualTo("file:///tmp/ns/view");
} else {
assertThat(view.location()).isNotNull();
}

view =
catalog()
.buildView(identifier)
.withSchema(SCHEMA)
.withDefaultNamespace(identifier.namespace())
.withQuery("trino", "select * from ns.tbl")
.withLocation("file://updated_tmp/ns/view")
.withLocation("file:///updated_tmp/ns/view")
.replace();

assertThat(view.location()).isEqualTo("file://updated_tmp/ns/view");
if (!overridesRequestedLocation()) {
assertThat(view.location()).isEqualTo("file:///updated_tmp/ns/view");
} else {
assertThat(view.location()).isNotNull();
}

assertThat(catalog().dropView(identifier)).isTrue();
assertThat(catalog().viewExists(identifier)).as("View should not exist").isFalse();
Expand All @@ -1394,17 +1467,25 @@ public void updateViewLocation() {
.withSchema(SCHEMA)
.withDefaultNamespace(identifier.namespace())
.withQuery("trino", "select * from ns.tbl")
.withLocation("file://tmp/ns/view")
.withLocation("file:///tmp/ns/view")
.create();

assertThat(catalog().viewExists(identifier)).as("View should exist").isTrue();
assertThat(view.location()).isEqualTo("file://tmp/ns/view");
if (!overridesRequestedLocation()) {
assertThat(view.location()).isEqualTo("file:///tmp/ns/view");
} else {
assertThat(view.location()).isNotNull();
}

view.updateLocation().setLocation("file://updated_tmp/ns/view").commit();
view.updateLocation().setLocation("file:///updated_tmp/ns/view").commit();

View updatedView = catalog().loadView(identifier);

assertThat(updatedView.location()).isEqualTo("file://updated_tmp/ns/view");
if (!overridesRequestedLocation()) {
assertThat(updatedView.location()).isEqualTo("file:///updated_tmp/ns/view");
} 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());
Expand Down Expand Up @@ -1446,7 +1527,14 @@ public void updateViewLocationConflict() {

// the view was already dropped concurrently
assertThatThrownBy(() -> updateViewLocation.setLocation("new-location").commit())
.isInstanceOf(CommitFailedException.class)
.hasMessageContaining("Cannot commit");
.satisfiesAnyOf(
x ->
assertThat(x)
.isInstanceOf(NoSuchViewException.class)
.hasMessageContaining("View does not exist: ns.view"),
x ->
assertThat(x)
.isInstanceOf(CommitFailedException.class)
.hasMessageContaining("Cannot commit"));
}
}
29 changes: 23 additions & 6 deletions nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
import org.projectnessie.client.api.OnReferenceBuilder;
import org.projectnessie.client.http.HttpClientException;
import org.projectnessie.error.BaseNessieClientServerException;
import org.projectnessie.error.NessieBadRequestException;
import org.projectnessie.error.NessieConflictException;
import org.projectnessie.error.NessieNamespaceAlreadyExistsException;
import org.projectnessie.error.NessieNamespaceNotEmptyException;
Expand Down Expand Up @@ -351,15 +352,19 @@ private void renameContent(TableIdentifier from, TableIdentifier to, boolean isV
IcebergContent existingFromContent = isView ? view(from) : table(from);
if (existingFromContent == null) {
if (isView) {
throw new NoSuchViewException("View does not exist: %s", from.name());
throw new NoSuchViewException("View does not exist: %s", from);
} else {
throw new NoSuchTableException("Table does not exist: %s", from.name());
throw new NoSuchTableException("Table does not exist: %s", from);
}
}

IcebergContent existingToContent = isView ? view(to) : table(to);
if (existingToContent != null) {
throw new AlreadyExistsException("%s already exists: %s", contentType, to.name());
if (isView) {
throw new AlreadyExistsException("Cannot rename %s to %s. View already exists", from, to);
} else {
throw new AlreadyExistsException("Cannot rename %s to %s. Table already exists", from, to);
}
}

CommitMultipleOperationsBuilder operations =
Expand Down Expand Up @@ -395,21 +400,33 @@ private void renameContent(TableIdentifier from, TableIdentifier to, boolean isV
throw new RuntimeException(
String.format(
"Cannot rename %s '%s' to '%s': " + "ref '%s' no longer exists.",
contentType, from.name(), to.name(), getRef().getName()),
contentType, from, to, getRef().getName()),
e);
} catch (BaseNessieClientServerException e) {
throw new CommitFailedException(
e,
"Cannot rename %s '%s' to '%s': " + "the current reference is not up to date.",
contentType,
from.name(),
to.name());
from,
to);
} catch (HttpClientException ex) {
// Intentionally catch all nessie-client-exceptions here and not just the "timeout" variant
// to catch all kinds of network errors (e.g. connection reset). Network code implementation
// details and all kinds of network devices can induce unexpected behavior. So better be
// safe than sorry.
throw new CommitStateUnknownException(ex);
} catch (NessieBadRequestException ex) {
if (ex.getMessage().contains("already exists with content ID")) {
// View might have created with the same name concurrently.
if (isView) {
throw new AlreadyExistsException(
ex, "Cannot rename %s to %s. Table already exists", from, to);
} else {
throw new AlreadyExistsException(
ex, "Cannot rename %s to %s. View already exists", from, to);
}
}
throw ex;
}
// Intentionally just "throw through" Nessie's HttpClientException here and do not "special
// case"
Expand Down
22 changes: 19 additions & 3 deletions nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,17 +22,20 @@
import org.apache.iceberg.BaseMetastoreTableOperations;
import org.apache.iceberg.TableMetadata;
import org.apache.iceberg.TableMetadataParser;
import org.apache.iceberg.exceptions.AlreadyExistsException;
import org.apache.iceberg.exceptions.CommitFailedException;
import org.apache.iceberg.exceptions.CommitStateUnknownException;
import org.apache.iceberg.exceptions.NoSuchTableException;
import org.apache.iceberg.io.FileIO;
import org.projectnessie.client.http.HttpClientException;
import org.projectnessie.error.NessieBadRequestException;
import org.projectnessie.error.NessieConflictException;
import org.projectnessie.error.NessieNotFoundException;
import org.projectnessie.error.NessieReferenceConflictException;
import org.projectnessie.model.Content;
import org.projectnessie.model.ContentKey;
import org.projectnessie.model.IcebergTable;
import org.projectnessie.model.IcebergView;
import org.projectnessie.model.Reference;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -97,12 +100,18 @@ protected void doRefresh() {
content
.unwrap(IcebergTable.class)
.orElseThrow(
() ->
new IllegalStateException(
() -> {
if (content instanceof IcebergView) {
return new AlreadyExistsException(
"View with same name already exists: %s", key);
} else {
return new IllegalStateException(
String.format(
"Cannot refresh iceberg table: "
+ "Nessie points to a non-Iceberg object for path: %s.",
key)));
key));
}
});
metadataLocation = table.getMetadataLocation();
}
} catch (NessieNotFoundException ex) {
Expand Down Expand Up @@ -154,6 +163,13 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
failure = true;
throw new RuntimeException(
String.format("Cannot commit: Reference '%s' no longer exists", refName), ex);
} catch (NessieBadRequestException ex) {
if (ex.getMessage().contains("New value to update existing key")) {
failure = true;
// View might have created with the same name concurrently.
throw new AlreadyExistsException(ex, "View with same name already exists: %s", key);
}
throw ex;
} finally {
if (failure) {
io().deleteFile(newMetadataLocation);
Expand Down
8 changes: 7 additions & 1 deletion nessie/src/main/java/org/apache/iceberg/nessie/NessieUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.apache.iceberg.exceptions.NamespaceNotEmptyException;
import org.apache.iceberg.exceptions.NoSuchNamespaceException;
import org.apache.iceberg.exceptions.NoSuchTableException;
import org.apache.iceberg.exceptions.NoSuchViewException;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.relocated.com.google.common.collect.Maps;
Expand Down Expand Up @@ -197,7 +198,12 @@ static void maybeThrowSpecializedException(NessieReferenceConflictException ex,
case NAMESPACE_NOT_EMPTY:
throw new NamespaceNotEmptyException(ex, "Namespace not empty: %s", conflict.key());
case KEY_DOES_NOT_EXIST:
throw new NoSuchTableException(ex, "%s does not exist: %s", contentType, conflict.key());
if (isView) {
throw new NoSuchViewException(ex, "%s does not exist: %s", contentType, conflict.key());
} else {
throw new NoSuchTableException(
ex, "%s does not exist: %s", contentType, conflict.key());
}
case KEY_EXISTS:
throw new AlreadyExistsException(
ex, "%s already exists: %s", contentType, conflict.key());
Expand Down
Loading

0 comments on commit f9779c0

Please sign in to comment.