majin1102 commented on code in PR #4334:
URL: https://github.com/apache/amoro/pull/4334#discussion_r3859387120
##########
amoro-ams/src/main/java/org/apache/amoro/server/catalog/CatalogBuilder.java:
##########
@@ -86,6 +86,13 @@ public static ServerCatalog buildServerCatalog(
"Table format %s is not supported for metastore type: %s",
tableFormats,
type);
+ Preconditions.checkState(
+ !(CATALOG_TYPE_REST.equals(type)
+ && tableFormats.contains(TableFormat.LANCE)
+ && tableFormats.size() > 1),
+ "REST catalog serves a single protocol per uri,"
+ + " Lance cannot be combined with other table formats: %s",
+ tableFormats);
Review Comment:
This check only runs when building a catalog. Updating an already loaded
REST catalog bypasses `CatalogBuilder`, so an API request can persist
`LANCE,ICEBERG` and leave the catalog broken after refresh or restart. Please
enforce this invariant before updates are persisted as well.
##########
amoro-format-lance/src/main/java/org/apache/amoro/formats/lance/AbstractLanceCatalog.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * 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.amoro.formats.lance;
+
+import org.apache.amoro.AmoroTable;
+import org.apache.amoro.FormatCatalog;
+import org.apache.amoro.NoSuchDatabaseException;
+import org.apache.amoro.NoSuchTableException;
+import org.apache.amoro.table.TableIdentifier;
+import org.lance.Dataset;
+import org.lance.namespace.LanceNamespace;
+import org.lance.namespace.errors.TableNotFoundException;
+import org.lance.namespace.model.DropTableRequest;
+import org.lance.namespace.model.ListTablesRequest;
+import org.lance.namespace.model.ListTablesResponse;
+import org.lance.namespace.model.TableExistsRequest;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+/** Base implementation of {@link FormatCatalog} for Lance. */
+public abstract class AbstractLanceCatalog implements FormatCatalog {
+
+ protected final String catalogName;
+ protected final LanceNamespace namespace;
+
+ protected AbstractLanceCatalog(String catalogName, LanceNamespace namespace)
{
+ this.catalogName = catalogName;
+ this.namespace = namespace;
+ }
+
+ /** Composes the namespace table id used to reference a single table. */
+ protected abstract List<String> tableId(String database, String table);
+
+ /** Composes the namespace id used to list tables of a database. */
+ protected abstract List<String> tableIdForListTables(String database);
+
+ @Override
+ public boolean tableExists(String database, String table) {
+ if (!databaseExists(database)) {
+ return false;
+ }
+
+ try {
+ namespace.tableExists(new TableExistsRequest().id(tableId(database,
table)));
+ return true;
+ } catch (TableNotFoundException e) {
+ return false;
+ }
+ }
+
+ @Override
+ public AmoroTable<?> loadTable(String database, String tableName) {
+ if (!tableExists(database, tableName)) {
+ throw new NoSuchTableException("Table: " + database + "." + tableName +
" does not exist");
+ }
+
+ TableIdentifier identifier = TableIdentifier.of(catalogName, database,
tableName);
+ Dataset dataset =
+ Dataset.open().namespaceClient(namespace).tableId(tableId(database,
tableName)).build();
+ return new LanceTable(identifier, dataset, Collections.emptyMap());
+ }
+
+ @Override
+ public boolean dropTable(String database, String table, boolean purge) {
+ validateDatabase(database);
+
+ try {
+ namespace.dropTable(new DropTableRequest().id(tableId(database, table)));
+ return true;
+ } catch (TableNotFoundException e) {
+ return false;
+ }
+ }
+
+ @Override
+ public List<String> listTables(String database) {
+ if (!databaseExists(database)) {
+ return Collections.emptyList();
+ }
+ ListTablesRequest request = new
ListTablesRequest().id(tableIdForListTables(database));
+ ListTablesResponse response = namespace.listTables(request);
+ if (response == null) {
+ return Collections.emptyList();
+ }
+ return new ArrayList<>(response.getTables());
Review Comment:
The REST response is paginated via `pageToken`, but this returns only the
first page, so larger catalogs silently miss tables. Please follow the token
until it is empty; `listDatabases()` needs the same handling.
##########
amoro-web/src/views/catalogs/Detail.vue:
##########
@@ -409,7 +421,37 @@ watch(
formState.authConfig[simpleUsernameKey] = 'local'
}
}
- }
+ },
+)
+
+watch(
+ () => formState.tableFormatList,
+ (formats, oldFormats) => {
+ const isRest = formState.catalog.type === 'rest'
+ const hasLance = formats.includes(tableFormatMap.LANCE)
+
+ if (isRest && hasLance && formats.length > 1) {
+ // If Lance was just added, keep only Lance; otherwise the user switched
away from Lance.
+ const hadLance = oldFormats?.includes(tableFormatMap.LANCE)
+ formState.tableFormatList = hadLance
+ ? formats.filter(format => format !== tableFormatMap.LANCE)
+ : [tableFormatMap.LANCE]
+ return
+ }
+
+ // REST + Lance needs no client-side storage/auth; drop them on selection .
+ if (isRest && hasLance) {
+ Object.keys(formState.storageConfig).forEach((key) => {
+ delete formState.storageConfig[key]
+ })
+ Object.keys(formState.authConfig).forEach((key) => {
+ delete formState.authConfig[key]
+ })
Review Comment:
Selecting Lance deletes the existing storage/auth values. If the user
switches back to Iceberg and saves, those credentials are lost. Since
`handleSave()` already replaces the Lance payload, please preserve the form
state here.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
