snazy commented on code in PR #8909:
URL: https://github.com/apache/iceberg/pull/8909#discussion_r1410794852
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -347,4 +327,73 @@ private TableIdentifier identifierWithoutTableReference(
protected Map<String, String> properties() {
return catalogOptions;
}
+
+ @Override
+ protected ViewOperations newViewOps(TableIdentifier identifier) {
+ TableReference tr = parseTableReference(identifier);
+ return new NessieViewOperations(
+ ContentKey.of(
+
org.projectnessie.model.Namespace.of(identifier.namespace().levels()),
tr.getName()),
+ client.withReference(tr.getReference(), tr.getHash()),
+ fileIO);
+ }
+
+ @Override
+ public List<TableIdentifier> listViews(Namespace namespace) {
+ return client.listViews(namespace);
+ }
+
+ @Override
+ public boolean dropView(TableIdentifier identifier) {
+ TableReference tableReference = parseTableReference(identifier);
+ return client
+ .withReference(tableReference.getReference(), tableReference.getHash())
+ .dropView(identifierWithoutTableReference(identifier, tableReference),
false);
+ }
+
+ @Override
+ public void renameView(TableIdentifier from, TableIdentifier to) {
+ renameContent(from, to, Content.Type.ICEBERG_VIEW);
+ }
+
+ private void renameContent(TableIdentifier from, TableIdentifier to,
Content.Type type) {
+ TableReference fromTableReference = parseTableReference(from);
+ TableReference toTableReference = parseTableReference(to);
+ String fromReference =
+ fromTableReference.hasReference()
+ ? fromTableReference.getReference()
+ : client.getRef().getName();
+ String toReference =
+ toTableReference.hasReference()
+ ? toTableReference.getReference()
+ : client.getRef().getName();
+ Preconditions.checkArgument(
+ fromReference.equalsIgnoreCase(toReference),
+ "Cannot rename %s '%s' on reference '%s' to '%s' on reference '%s':"
+ + " source and target references must be the same.",
+ NessieUtil.contentTypeString(type).toLowerCase(),
+ fromTableReference.getName(),
+ fromReference,
+ toTableReference.getName(),
+ toReference);
+
+ TableIdentifier fromIdentifier =
+ NessieUtil.removeCatalogName(
+ identifierWithoutTableReference(from, fromTableReference), name());
+ TableIdentifier toIdentifier =
+ NessieUtil.removeCatalogName(identifierWithoutTableReference(to,
toTableReference), name());
+
+ if (type == Content.Type.ICEBERG_TABLE) {
Review Comment:
Overly complex: This if-else cascade and the method chains are confusing.
Above it "consolidates" table+view renames to `renameContent()` to call
specialized functions on `NessieIcebergClient` which then call a generic
function.
Similar to the `dropTable/View` above.
##########
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:
Still, why is this _always_ run and not only when a conflict happened?
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java:
##########
@@ -136,15 +142,26 @@ private UpdateableReference loadReference(String
requestedRef, String hash) {
}
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);
+ }
+
+ private List<TableIdentifier> listContents(Namespace namespace, Content.Type
type) {
try {
return withReference(api.getEntries()).get().getEntries().stream()
.filter(namespacePredicate(namespace))
- .filter(e -> Content.Type.ICEBERG_TABLE == e.getType())
+ .filter(e -> type == e.getType())
Review Comment:
Should use `.equals` here, Content.Type is _not_ an enum
##########
nessie/src/test/java/org/apache/iceberg/nessie/BaseTestIceberg.java:
##########
@@ -180,6 +188,33 @@ protected Table createTable(TableIdentifier
tableIdentifier, Schema schema) {
return catalog.createTable(tableIdentifier, schema);
}
+ protected View createView(NessieCatalog nessieCatalog, TableIdentifier
tableIdentifier) {
+ createMissingNamespaces(tableIdentifier);
Review Comment:
Why is this done twice?
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieUtil.java:
##########
@@ -165,4 +180,95 @@ public static TableMetadata
updateTableMetadataWithNessieSpecificProperties(
return builder.discardChanges().build();
}
+
+ public static ViewMetadata loadViewMetadata(
Review Comment:
That similarity is IMO not a reason. Also not sure why Trino's related.
##########
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:
I don't understand the reason that two classes cant implement the same
interface???
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieViewOperations.java:
##########
@@ -0,0 +1,149 @@
+/*
+ * 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 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 {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(NessieViewOperations.class);
+
+ private final NessieIcebergClient client;
+ private final ContentKey key;
+ private final FileIO fileIO;
+ private IcebergView icebergView;
+
+ NessieViewOperations(ContentKey key, NessieIcebergClient client, FileIO
fileIO) {
+ this.key = key;
+ this.client = client;
+ this.fileIO = fileIO;
+ }
+
+ @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(
+ "Table with same name already exists: %s in %s",
key, reference);
+ } else {
+ return new AlreadyExistsException(
+ "Cannot refresh Iceberg view: Nessie points to a
non-Iceberg object for path: %s in %s",
+ key, reference);
+ }
+ });
+ metadataLocation = icebergView.getMetadataLocation();
+ }
+ } catch (NessieNotFoundException ex) {
+ if (currentMetadataLocation() != null) {
+ throw new NoSuchViewException("View does not exist: %s in %s", key,
reference);
+ }
+ }
+ refreshFromMetadataLocation(
+ metadataLocation,
+ null,
+ 2,
+ location ->
+ NessieUtil.loadViewMetadata(
+ ViewMetadataParser.read(io().newInputFile(location)),
location, reference));
+ }
+
+ @Override
+ public void doCommit(ViewMetadata base, ViewMetadata metadata) {
+ try {
+ Content content =
+
client.getApi().getContent().key(key).reference(client.getReference()).get().get(key);
Review Comment:
Same concern as for tables: This check is _only_ needed when a conflict
happened - it should _not_ be in the good-case code-path.
--
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
