Skip to content

Commit

Permalink
Address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@ajantha-bhat
ajantha-bhat committed Dec 8, 2023
1 parent 34d55cd commit c727200
Show file tree
Hide file tree
Showing 7 changed files with 59 additions and 43 deletions.
27 changes: 17 additions & 10 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 @@ -147,14 +147,16 @@ private UpdateableReference loadReference(String requestedRef, String hash) {
}
}

/** @deprecated will be removed after 1.5.0; use listContents() instead */
@Deprecated
public List<TableIdentifier> listTables(Namespace namespace) {
return listContents(namespace, Content.Type.ICEBERG_TABLE);
}

public List<TableIdentifier> listViews(Namespace namespace) {
return listContents(namespace, Content.Type.ICEBERG_VIEW);
}

/** Lists Iceberg table or view from the given namespace */
public List<TableIdentifier> listContents(Namespace namespace, Content.Type type) {
protected List<TableIdentifier> listContents(Namespace namespace, Content.Type type) {
try {
return withReference(api.getEntries()).get().getEntries().stream()
.filter(namespacePredicate(namespace))
Expand Down Expand Up @@ -414,13 +416,15 @@ namespace, getRef().getName()),
}
}

/** @deprecated will be removed after 1.5.0; use renameContent() instead */
@Deprecated
public void renameTable(TableIdentifier from, TableIdentifier to) {
renameContent(from, to, Content.Type.ICEBERG_TABLE);
}

public void renameContent(TableIdentifier from, TableIdentifier to, Content.Type type) {
public void renameView(TableIdentifier from, TableIdentifier to) {
renameContent(from, to, Content.Type.ICEBERG_VIEW);
}

protected void renameContent(TableIdentifier from, TableIdentifier to, Content.Type type) {
getRef().checkMutable();

IcebergContent existingFromContent = fetchContent(from);
Expand Down Expand Up @@ -482,7 +486,8 @@ private static void validateToContentForRename(
throw new AlreadyExistsException("Cannot rename %s to %s. Table already exists", from, to);
} else {
throw new AlreadyExistsException(
"Cannot rename %s to %s. Another content with same name already exists", from, to);
"Cannot rename %s to %s. Another content of type %s with same name already exists",
from, to, existingToContent.getType());
}
}
}
Expand All @@ -503,13 +508,15 @@ private static void validateFromContentForRename(
}
}

/** @deprecated will be removed after 1.5.0; use dropContent() instead */
@Deprecated
public boolean dropTable(TableIdentifier identifier, boolean purge) {
return dropContent(identifier, purge, Content.Type.ICEBERG_TABLE);
}

public boolean dropContent(TableIdentifier identifier, boolean purge, Content.Type type) {
public boolean dropView(TableIdentifier identifier, boolean purge) {
return dropContent(identifier, purge, Content.Type.ICEBERG_VIEW);
}

protected boolean dropContent(TableIdentifier identifier, boolean purge, Content.Type type) {
getRef().checkMutable();

IcebergContent existingContent = fetchContent(identifier);
Expand Down
4 changes: 2 additions & 2 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 @@ -137,8 +137,8 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
NessieUtil.handleExceptionsForCommits(ex, client.refName(), Content.Type.ICEBERG_TABLE);
} catch (NessieBadRequestException ex) {
failure = true;
NessieUtil.handleBadRequestForCommit(client, key, Content.Type.ICEBERG_TABLE);
throw ex;
throw NessieUtil.handleBadRequestForCommit(client, key, Content.Type.ICEBERG_TABLE)
.orElse(ex);
} finally {
if (failure) {
io().deleteFile(newMetadataLocation);
Expand Down
19 changes: 11 additions & 8 deletions nessie/src/main/java/org/apache/iceberg/nessie/NessieUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ static void handleExceptionsForCommits(Exception exception, String refName, Cont
}
}

static void handleBadRequestForCommit(
static Optional<RuntimeException> handleBadRequestForCommit(
NessieIcebergClient client, ContentKey key, Content.Type type) {
Content.Type anotherType =
type == Content.Type.ICEBERG_TABLE ? Content.Type.ICEBERG_VIEW : Content.Type.ICEBERG_TABLE;
Expand All @@ -252,18 +252,21 @@ static void handleBadRequestForCommit(
client.getApi().getContent().key(key).reference(client.getReference()).get().get(key);
if (content != null) {
if (content.getType().equals(anotherType)) {
throw new AlreadyExistsException(
"%s with same name already exists: %s in %s",
NessieUtil.contentTypeString(anotherType), key, client.getReference());
return Optional.of(
new AlreadyExistsException(
"%s with same name already exists: %s in %s",
NessieUtil.contentTypeString(anotherType), key, client.getReference()));
} else if (!content.getType().equals(type)) {
throw new AlreadyExistsException(
"Another content with same name already exists: %s in %s",
key, client.getReference());
return Optional.of(
new AlreadyExistsException(
"Another content with same name already exists: %s in %s",
key, client.getReference()));
}
}
} catch (NessieNotFoundException e) {
throw new RuntimeException(e);
return Optional.of(new RuntimeException(e));
}
return Optional.empty();
}

private static void maybeThrowSpecializedException(
Expand Down
3 changes: 1 addition & 2 deletions nessie/src/main/java/org/apache/iceberg/nessie/NessieViewOperations.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,7 @@ public void doCommit(ViewMetadata base, ViewMetadata metadata) {
NessieUtil.handleExceptionsForCommits(ex, client.refName(), Content.Type.ICEBERG_VIEW);
} catch (NessieBadRequestException ex) {
failure = true;
NessieUtil.handleBadRequestForCommit(client, key, Content.Type.ICEBERG_VIEW);
throw ex;
throw NessieUtil.handleBadRequestForCommit(client, key, Content.Type.ICEBERG_VIEW).orElse(ex);
} finally {
if (failure) {
io().deleteFile(newMetadataLocation);
Expand Down
7 changes: 3 additions & 4 deletions nessie/src/test/java/org/apache/iceberg/nessie/BaseTestIceberg.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.types.Types;
import org.apache.iceberg.types.Types.LongType;
import org.apache.iceberg.types.Types.StructType;
import org.apache.iceberg.view.BaseView;
import org.apache.iceberg.view.View;
import org.junit.jupiter.api.AfterEach;
Expand Down Expand Up @@ -179,7 +178,7 @@ protected Table createTable(TableIdentifier tableIdentifier, int count) {

protected void createTable(TableIdentifier tableIdentifier) {
createMissingNamespaces(tableIdentifier);
Schema schema = new Schema(StructType.of(required(1, "id", LongType.get())).fields());
Schema schema = new Schema(required(1, "id", LongType.get()));
catalog.createTable(tableIdentifier, schema).location();
}

Expand All @@ -189,7 +188,7 @@ protected Table createTable(TableIdentifier tableIdentifier, Schema schema) {
}

protected View createView(NessieCatalog nessieCatalog, TableIdentifier tableIdentifier) {
Schema schema = new Schema(StructType.of(required(1, "id", LongType.get())).fields());
Schema schema = new Schema(required(1, "id", LongType.get()));
return createView(nessieCatalog, tableIdentifier, schema);
}

Expand All @@ -205,7 +204,7 @@ protected View createView(
}

protected View replaceView(NessieCatalog nessieCatalog, TableIdentifier identifier) {
Schema schema = new Schema(StructType.of(required(2, "age", Types.IntegerType.get())).fields());
Schema schema = new Schema(required(2, "age", Types.IntegerType.get()));
return nessieCatalog
.buildView(identifier)
.withSchema(schema)
Expand Down
37 changes: 21 additions & 16 deletions nessie/src/test/java/org/apache/iceberg/nessie/TestNessieView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
import java.nio.file.Paths;
import java.util.Comparator;
import java.util.List;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import org.apache.iceberg.Schema;
import org.apache.iceberg.catalog.TableIdentifier;
Expand All @@ -49,6 +48,7 @@
import org.projectnessie.model.IcebergView;
import org.projectnessie.model.ImmutableTableReference;
import org.projectnessie.model.LogResponse.LogEntry;
import org.projectnessie.model.TableReference;

public class TestNessieView extends BaseTestIceberg {

Expand Down Expand Up @@ -130,10 +130,9 @@ public void verifyStateMovesForDML() throws Exception {
Assertions.assertThat(contentInitialMain)
.as("global-contents + snapshot-id equal on both branches in Nessie")
.isEqualTo(contentInitialBranch);
Assertions.assertThat(viewInitialMain.currentVersion()).isNotNull();
Assertions.assertThat(viewInitialMain.currentVersion().versionId()).isEqualTo(2);

// 3. modify view in "main" branch

icebergView
.replaceVersion()
.withQuery("trino", "some other query")
Expand All @@ -158,6 +157,7 @@ public void verifyStateMovesForDML() throws Exception {
.describedAs("schema ID must be same across branches")
.isEqualTo(contentsAfter1Branch.getSchemaId());
// verify updates
Assertions.assertThat(viewAfter1Main.currentVersion().versionId()).isEqualTo(3);
Assertions.assertThat(
((SQLViewRepresentation) viewAfter1Main.currentVersion().representations().get(0))
.dialect())
Expand All @@ -178,12 +178,15 @@ public void verifyStateMovesForDML() throws Exception {

// --> assert getValue() against both branches returns the updated metadata-location
// verify view-metadata-location
Assertions.assertThat(contentsAfter2Main.getVersionId()).isEqualTo(4);
Assertions.assertThat(contentsAfter2Main.getMetadataLocation())
.describedAs("metadata-location must change on %s", BRANCH)
.isNotEqualTo(contentsAfter1Main.getMetadataLocation());
Assertions.assertThat(contentsAfter1Main.getVersionId()).isEqualTo(3);
Assertions.assertThat(contentsAfter2Branch.getMetadataLocation())
.describedAs("on-reference-state must not change on %s", testCaseBranch)
.isEqualTo(contentsAfter1Branch.getMetadataLocation());
Assertions.assertThat(viewAfter2Main.currentVersion().versionId()).isEqualTo(4);
Assertions.assertThat(
((SQLViewRepresentation) viewAfter2Main.currentVersion().representations().get(0))
.dialect())
Expand Down Expand Up @@ -218,12 +221,12 @@ public void testRenameWithTableReference() throws NessieNotFoundException {
TableIdentifier renameViewIdentifier =
TableIdentifier.of(VIEW_IDENTIFIER.namespace(), renamedViewName);

ImmutableTableReference fromTableReference =
TableReference fromTableReference =
ImmutableTableReference.builder()
.reference(catalog.currentRefName())
.name(VIEW_IDENTIFIER.name())
.build();
ImmutableTableReference toTableReference =
TableReference toTableReference =
ImmutableTableReference.builder()
.reference(catalog.currentRefName())
.name(renameViewIdentifier.name())
Expand All @@ -233,9 +236,13 @@ public void testRenameWithTableReference() throws NessieNotFoundException {
TableIdentifier toIdentifier =
TableIdentifier.of(VIEW_IDENTIFIER.namespace(), toTableReference.toString());

View viewBeforeRename = catalog.loadView(fromIdentifier);
catalog.renameView(fromIdentifier, toIdentifier);
Assertions.assertThat(catalog.viewExists(fromIdentifier)).isFalse();
Assertions.assertThat(catalog.viewExists(toIdentifier)).isTrue();
View viewAfterRename = catalog.loadView(toIdentifier);
Assertions.assertThat(viewBeforeRename.currentVersion().versionId())
.isEqualTo(viewAfterRename.currentVersion().versionId());

Assertions.assertThat(catalog.dropView(toIdentifier)).isTrue();

Expand Down Expand Up @@ -299,10 +306,12 @@ private void verifyCommitMetadata() throws NessieNotFoundException {
.allSatisfy(
logEntry -> {
CommitMeta commit = logEntry.getCommitMeta();
Assertions.assertThat(commit.getAuthor()).isNotNull().isNotEmpty();
Assertions.assertThat(commit.getAuthor()).isEqualTo(System.getProperty("user.name"));
Assertions.assertThat(commit.getProperties().get(NessieUtil.APPLICATION_TYPE))
.isEqualTo("iceberg");
Assertions.assertThat(commit.getAuthor())
.isNotNull()
.isNotEmpty()
.isEqualTo(System.getProperty("user.name"));
Assertions.assertThat(commit.getProperties())
.containsEntry(NessieUtil.APPLICATION_TYPE, "iceberg");
Assertions.assertThat(commit.getMessage()).startsWith("Iceberg");
});
}
Expand All @@ -317,16 +326,12 @@ public void testDrop() throws NessieNotFoundException {
}

@Test
public void testListviews() {
public void testListViews() {
TableIdentifier newIdentifier = TableIdentifier.of(DB_NAME, "newView");
createView(catalog, newIdentifier, SCHEMA);

List<TableIdentifier> tableIdents = catalog.listViews(VIEW_IDENTIFIER.namespace());
List<TableIdentifier> expectedIdents =
tableIdents.stream()
.filter(t -> t.equals(newIdentifier) || t.equals(VIEW_IDENTIFIER))
.collect(Collectors.toList());
Assertions.assertThat(expectedIdents).hasSize(2);
List<TableIdentifier> viewIdents = catalog.listViews(VIEW_IDENTIFIER.namespace());
Assertions.assertThat(viewIdents).contains(VIEW_IDENTIFIER, newIdentifier);
Assertions.assertThat(catalog.viewExists(VIEW_IDENTIFIER)).isTrue();
Assertions.assertThat(catalog.viewExists(newIdentifier)).isTrue();
}
Expand Down
5 changes: 4 additions & 1 deletion nessie/src/test/java/org/apache/iceberg/nessie/TestNessieViewCatalog.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import org.apache.iceberg.view.ViewMetadata;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.api.extension.RegisterExtension;
import org.junit.jupiter.api.io.TempDir;
Expand Down Expand Up @@ -64,7 +65,7 @@ public class TestNessieViewCatalog extends ViewCatalogTests<NessieCatalog> {
@RegisterExtension
static NessieJaxRsExtension server = NessieJaxRsExtension.jaxRsExtension(() -> persist);

@TempDir public Path temp;
@TempDir private Path temp;

private NessieCatalog catalog;
private NessieApiV1 api;
Expand Down Expand Up @@ -151,6 +152,7 @@ protected boolean requiresNamespaceCreate() {
// Nessie adds extra properties (like commit id) on every operation. Hence, view metadata will not
// be same after rename.
@Override
@Test
public void renameView() {
TableIdentifier from = TableIdentifier.of("ns", "view");
TableIdentifier to = TableIdentifier.of("ns", "renamedView");
Expand Down Expand Up @@ -185,6 +187,7 @@ public void renameView() {
}

@Override
@Test
public void renameViewUsingDifferentNamespace() {
TableIdentifier from = TableIdentifier.of("ns", "view");
TableIdentifier to = TableIdentifier.of("other_ns", "renamedView");
Expand Down

0 comments on commit c727200

Please sign in to comment.