snazy commented on code in PR #8909:
URL: https://github.com/apache/iceberg/pull/8909#discussion_r1397151718
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java:
##########
@@ -312,26 +327,32 @@ namespace, getRef().getName()),
}
public void renameTable(TableIdentifier from, TableIdentifier to) {
+ renameContent(from, to, Content.Type.ICEBERG_TABLE);
+ }
+
+ public void renameView(TableIdentifier from, TableIdentifier to) {
+ renameContent(from, to, Content.Type.ICEBERG_VIEW);
+ }
+
+ private void renameContent(TableIdentifier from, TableIdentifier to,
Content.Type type) {
getRef().checkMutable();
- IcebergTable existingFromTable = table(from);
- if (existingFromTable == null) {
- throw new NoSuchTableException("Table does not exist: %s", from.name());
- }
- IcebergTable existingToTable = table(to);
- if (existingToTable != null) {
- throw new AlreadyExistsException("Table already exists: %s", to.name());
- }
+ IcebergContent existingFromContent = asContent(from, IcebergContent.class);
+ validateFromContentForRename(from, type, existingFromContent);
+
+ IcebergContent existingToContent = asContent(to, IcebergContent.class);
+ validateToContentForRename(from, to, existingToContent);
+ String contentType = type == Content.Type.ICEBERG_VIEW ? "view" : "table";
Review Comment:
This should be a utility function reused across the code base.
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java:
##########
@@ -170,11 +184,12 @@ private TableIdentifier
toIdentifier(EntriesResponse.Entry entry) {
return TableIdentifier.of(elements.toArray(new String[elements.size()]));
}
- public IcebergTable table(TableIdentifier tableIdentifier) {
+ public <C extends IcebergContent> C asContent(
Review Comment:
`as` implies a conversion, but this function _fetches_ content.
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieViewOperations.java:
##########
@@ -0,0 +1,157 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.nessie;
+
+import java.util.Map;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.NoSuchViewException;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.view.BaseViewOperations;
+import org.apache.iceberg.view.ViewMetadata;
+import org.apache.iceberg.view.ViewMetadataParser;
+import org.projectnessie.client.http.HttpClientException;
+import org.projectnessie.error.NessieConflictException;
+import org.projectnessie.error.NessieNotFoundException;
+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;
+
+public class NessieViewOperations extends BaseViewOperations {
Review Comment:
Generally, `NessieViewOperations` and `NessieTableOperations` share a lot of
similar code - critical code duplicated.
I'd prefer to move the common parts to a package-private interface with
default implementations.
##########
nessie/src/test/java/org/apache/iceberg/nessie/BaseTestIceberg.java:
##########
@@ -267,4 +306,23 @@ static DataFile makeDataFile(Table icebergTable, String
fileLocation) {
.withFileSizeInBytes(Files.localInput(fileLocation).getLength())
.build();
}
+
+ protected static List<String> metadataVersionFiles(String tablePath) {
Review Comment:
This function (and the ones below) looks unused?
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java:
##########
@@ -540,4 +630,72 @@ public void close() {
api.close();
}
}
+
+ public void commitView(
+ ViewMetadata base,
+ ViewMetadata metadata,
+ String newMetadataLocation,
+ String contentId,
+ ContentKey key)
+ throws NessieConflictException, NessieNotFoundException {
+ UpdateableReference updateableReference = getRef();
+
+ updateableReference.checkMutable();
+
+ Branch current = (Branch) updateableReference.getReference();
+ Branch expectedHead = current;
+ if (base != null) {
+ String metadataCommitId =
+ base.properties()
+ .getOrDefault(
+ NessieTableOperations.NESSIE_COMMIT_ID_PROPERTY,
expectedHead.getHash());
+ if (metadataCommitId != null) {
+ expectedHead = Branch.of(expectedHead.getName(), metadataCommitId);
+ }
+ }
+
+ long versionId = metadata.currentVersion().versionId();
+
+ ImmutableIcebergView.Builder newViewBuilder =
ImmutableIcebergView.builder();
+ // Directly casting to `SQLViewRepresentation` as only SQL type exist in
+ // `ViewRepresentation.Type`.
+ // Assuming only one engine's dialect will be used, Nessie IcebergView
currently holds one
+ // representation.
+ // View loaded from catalog will have all the representation as it parses
the view metadata
+ // file.
+ SQLViewRepresentation sqlViewRepresentation =
+ (SQLViewRepresentation)
metadata.currentVersion().representations().get(0);
Review Comment:
All metadata should really be taken directly from the view metadata - Nessie
should really _not_ store the dialect nor the SQL representation. I don't
understand how this related to "global state" - those things are not related to
each other at all.
Also the unconditional cast is error prone IMO - similar to assuming that
there is only one representation - either of different kinds or multiple SQL
representation.
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieViewOperations.java:
##########
@@ -0,0 +1,159 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.nessie;
+
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicBoolean;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.NoSuchViewException;
+import org.apache.iceberg.io.FileIO;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.view.BaseViewOperations;
+import org.apache.iceberg.view.ViewMetadata;
+import org.apache.iceberg.view.ViewMetadataParser;
+import org.projectnessie.client.http.HttpClientException;
+import org.projectnessie.error.NessieConflictException;
+import org.projectnessie.error.NessieNotFoundException;
+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;
+
+public class NessieViewOperations extends BaseViewOperations {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(NessieViewOperations.class);
+
+ private final NessieIcebergClient client;
+ private final ContentKey key;
+ private final FileIO fileIO;
+ private final Map<String, String> catalogOptions;
+ private IcebergView icebergView;
+
+ NessieViewOperations(
+ ContentKey key,
+ NessieIcebergClient client,
+ FileIO fileIO,
+ Map<String, String> catalogOptions) {
+ this.key = key;
+ this.client = client;
+ this.fileIO = fileIO;
+ this.catalogOptions = catalogOptions;
+ }
+
+ @Override
+ public void doRefresh() {
+ try {
+ client.refresh();
+ } catch (NessieNotFoundException e) {
+ throw new RuntimeException(
+ String.format(
+ "Failed to refresh as ref '%s' is no longer valid.",
client.getRef().getName()),
+ e);
+ }
+ String metadataLocation = null;
+ Reference reference = client.getRef().getReference();
+ try {
+ Content content =
client.getApi().getContent().key(key).reference(reference).get().get(key);
+ LOG.debug("Content '{}' at '{}': {}", key, reference, content);
+ if (content == null) {
+ if (currentMetadataLocation() != null) {
+ throw new NoSuchViewException("View does not exist: %s in %s", key,
reference);
+ }
+ } else {
+ this.icebergView =
+ content
+ .unwrap(IcebergView.class)
+ .orElseThrow(
+ () -> {
+ if (content instanceof IcebergTable) {
+ return new AlreadyExistsException(
Review Comment:
The special case for `IcebergTable` here and for tables doesn't give much
benefit. Also that this case throws an `AlreadyExistsException` (valid for
commits only) and the `else` below throws and ISE is inconsistent IMO.
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java:
##########
@@ -540,4 +617,72 @@ public void close() {
api.close();
}
}
+
+ public void commitView(
Review Comment:
This function looks mostly identical to `commitTable()`, aka duplicated code.
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java:
##########
@@ -104,12 +96,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);
Review Comment:
View??
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java:
##########
@@ -132,74 +130,42 @@ protected void doRefresh() {
@Override
protected void doCommit(TableMetadata base, TableMetadata metadata) {
+ try {
+ Content content =
+
client.getApi().getContent().key(key).reference(client.getReference()).get().get(key);
+ if (content != null) {
Review Comment:
I don't understand the purpose of this check. The commit functionality in
Nessie checks for content type mismatches.
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java:
##########
@@ -312,26 +327,32 @@ namespace, getRef().getName()),
}
public void renameTable(TableIdentifier from, TableIdentifier to) {
+ renameContent(from, to, Content.Type.ICEBERG_TABLE);
+ }
+
+ public void renameView(TableIdentifier from, TableIdentifier to) {
+ renameContent(from, to, Content.Type.ICEBERG_VIEW);
+ }
+
+ private void renameContent(TableIdentifier from, TableIdentifier to,
Content.Type type) {
getRef().checkMutable();
- IcebergTable existingFromTable = table(from);
- if (existingFromTable == null) {
- throw new NoSuchTableException("Table does not exist: %s", from.name());
- }
- IcebergTable existingToTable = table(to);
- if (existingToTable != null) {
- throw new AlreadyExistsException("Table already exists: %s", to.name());
- }
+ IcebergContent existingFromContent = asContent(from, IcebergContent.class);
Review Comment:
These checks, including the new methods, look overly complicated now - and
do not work correctly for other content types.
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestBranchVisibilityForView.java:
##########
@@ -0,0 +1,291 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.nessie;
+
+import static org.apache.iceberg.types.Types.NestedField.required;
+
+import java.util.Arrays;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.types.Types;
+import org.apache.iceberg.view.BaseView;
+import org.apache.iceberg.view.View;
+import org.assertj.core.api.AbstractStringAssert;
+import org.assertj.core.api.Assertions;
+import org.assertj.core.api.ThrowableAssert.ThrowingCallable;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.projectnessie.error.NessieConflictException;
+import org.projectnessie.error.NessieNotFoundException;
+import org.projectnessie.model.Branch;
+import org.projectnessie.model.Reference;
+
+public class TestBranchVisibilityForView extends BaseTestIceberg {
Review Comment:
This test class duplicates the test cases - most are not specific to views.
It seems the view-specific stuff can be merged into `TestBranchVisibility`.
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java:
##########
@@ -378,27 +400,72 @@ public void renameTable(TableIdentifier from,
TableIdentifier to) {
// behavior. So better be safe than sorry.
}
+ private static void validateToContentForRename(
+ TableIdentifier from, TableIdentifier to, IcebergContent
existingToContent) {
+ if (existingToContent != null) {
+ if (existingToContent.getType() == Content.Type.ICEBERG_VIEW) {
+ throw new AlreadyExistsException("Cannot rename %s to %s. View already
exists", from, to);
+ } else if (existingToContent.getType() == Content.Type.ICEBERG_TABLE) {
+ 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);
+ }
+ }
+ }
+
+ private static void validateFromContentForRename(
+ TableIdentifier from, Content.Type type, IcebergContent
existingFromContent) {
+ if (existingFromContent == null) {
+ if (type == Content.Type.ICEBERG_VIEW) {
+ throw new NoSuchViewException("View does not exist: %s", from);
+ } else if (type == Content.Type.ICEBERG_TABLE) {
+ throw new NoSuchTableException("Table does not exist: %s", from);
+ } else {
+ throw new UnsupportedOperationException("Cannot perform rename for
content type: " + type);
+ }
+ } else if (existingFromContent.getType() != type) {
+ throw new UnsupportedOperationException(
+ String.format("content type of from identifier %s should be of %s",
from, type));
+ }
+ }
+
public boolean dropTable(TableIdentifier identifier, boolean purge) {
+ return dropContent(identifier, purge, Content.Type.ICEBERG_TABLE);
+ }
+
+ public boolean dropView(TableIdentifier identifier, boolean purge) {
+ return dropContent(identifier, purge, Content.Type.ICEBERG_VIEW);
+ }
+
+ private boolean dropContent(TableIdentifier identifier, boolean purge,
Content.Type type) {
getRef().checkMutable();
- IcebergTable existingTable = table(identifier);
- if (existingTable == null) {
+ IcebergContent existingContent =
+ asContent(
+ identifier, type == Content.Type.ICEBERG_VIEW ? IcebergView.class
: IcebergTable.class);
Review Comment:
Why do you derive the type class manually here??
##########
nessie/src/main/java/org/apache/iceberg/nessie/UpdateableReference.java:
##########
@@ -62,7 +62,7 @@ public Reference getReference() {
public void checkMutable() {
Preconditions.checkArgument(
- mutable, "You can only mutate tables when using a branch without a
hash or timestamp.");
+ mutable, "You can only mutate contents when using a branch without a
hash or timestamp.");
Review Comment:
"contents" is not a well-known term in the database world. I'd prefer
"tables/views" here - not sure if this is used for namespaces as well.
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieViewCatalog.java:
##########
@@ -0,0 +1,222 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.nessie;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.IOException;
+import java.net.URI;
+import java.nio.file.Path;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.iceberg.CatalogProperties;
+import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.view.BaseView;
+import org.apache.iceberg.view.View;
+import org.apache.iceberg.view.ViewCatalogTests;
+import org.apache.iceberg.view.ViewMetadata;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.extension.RegisterExtension;
+import org.junit.jupiter.api.io.TempDir;
+import org.projectnessie.client.api.NessieApiV1;
+import org.projectnessie.client.ext.NessieApiVersion;
+import org.projectnessie.client.ext.NessieApiVersions;
+import org.projectnessie.client.ext.NessieClientFactory;
+import org.projectnessie.client.ext.NessieClientUri;
+import org.projectnessie.error.NessieConflictException;
+import org.projectnessie.error.NessieNotFoundException;
+import org.projectnessie.jaxrs.ext.NessieJaxRsExtension;
+import org.projectnessie.model.Branch;
+import org.projectnessie.model.Reference;
+import org.projectnessie.model.Tag;
+import org.projectnessie.versioned.storage.common.persist.Persist;
+import org.projectnessie.versioned.storage.inmemory.InmemoryBackendTestFactory;
+import org.projectnessie.versioned.storage.testextension.NessieBackend;
+import org.projectnessie.versioned.storage.testextension.NessiePersist;
+import org.projectnessie.versioned.storage.testextension.PersistExtension;
+
+@ExtendWith(PersistExtension.class)
+@NessieBackend(InmemoryBackendTestFactory.class)
+@NessieApiVersions // test all versions
+public class TestNessieViewCatalog extends ViewCatalogTests<NessieCatalog> {
+
+ @NessiePersist static Persist persist;
+
+ @RegisterExtension
+ static NessieJaxRsExtension server = NessieJaxRsExtension.jaxRsExtension(()
-> persist);
+
+ @TempDir public Path temp;
+
+ private NessieCatalog catalog;
+ private NessieApiV1 api;
+ private NessieApiVersion apiVersion;
+ private Configuration hadoopConfig;
+ private String initialHashOfDefaultBranch;
+ private String uri;
+
+ @BeforeEach
+ public void setUp(NessieClientFactory clientFactory, @NessieClientUri URI
nessieUri)
+ throws NessieNotFoundException {
+ api = clientFactory.make();
+ apiVersion = clientFactory.apiVersion();
+ initialHashOfDefaultBranch = api.getDefaultBranch().getHash();
+ uri = nessieUri.toASCIIString();
+ hadoopConfig = new Configuration();
+ catalog = initNessieCatalog("main");
+ }
+
+ @AfterEach
+ public void afterEach() throws IOException {
+ resetData();
+ try {
+ if (catalog != null) {
+ catalog.close();
+ }
+ api.close();
+ } finally {
+ catalog = null;
+ api = null;
+ hadoopConfig = null;
+ }
+ }
+
+ private void resetData() throws NessieConflictException,
NessieNotFoundException {
+ Branch defaultBranch = api.getDefaultBranch();
+ for (Reference r : api.getAllReferences().get().getReferences()) {
+ if (r instanceof Branch && !r.getName().equals(defaultBranch.getName()))
{
+ api.deleteBranch().branch((Branch) r).delete();
+ }
+ if (r instanceof Tag) {
+ api.deleteTag().tag((Tag) r).delete();
+ }
+ }
+ api.assignBranch()
+ .assignTo(Branch.of(defaultBranch.getName(),
initialHashOfDefaultBranch))
+ .branch(defaultBranch)
+ .assign();
+ }
+
+ private NessieCatalog initNessieCatalog(String ref) {
+ NessieCatalog newCatalog = new NessieCatalog();
+ newCatalog.setConf(hadoopConfig);
+ ImmutableMap<String, String> options =
+ ImmutableMap.of(
+ "ref",
+ ref,
+ CatalogProperties.URI,
+ uri,
+ CatalogProperties.WAREHOUSE_LOCATION,
+ temp.toUri().toString(),
+ "client-api-version",
+ apiVersion == NessieApiVersion.V2 ? "2" : "1");
+ newCatalog.initialize("nessie", options);
+ return newCatalog;
+ }
+
+ @Override
+ protected NessieCatalog catalog() {
+ return catalog;
+ }
+
+ @Override
+ protected Catalog tableCatalog() {
+ return catalog;
+ }
+
+ @Override
+ protected boolean requiresNamespaceCreate() {
+ return true;
+ }
+
+ @Override
+ public void renameViewNamespaceMissing() {
+ // Need a fix from Nessie
(https://github.com/projectnessie/nessie/issues/7645)
Review Comment:
This issue is closed - why is this test still disabled?
Same for `TestNessieTable`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
