This is an automated email from the ASF dual-hosted git repository.
majin1102 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/amoro.git
The following commit(s) were added to refs/heads/master by this push:
new cc0ef839a [Hotfix] improve REST+Lance catalog correctness (#4341)
cc0ef839a is described below
commit cc0ef839a9efc3071c5db5e304efdabc8cd90948
Author: WenLingzhang <[email protected]>
AuthorDate: Tue Sep 8 11:57:28 2026 +0800
[Hotfix] improve REST+Lance catalog correctness (#4341)
* Enforce REST+Lance exclusivity on catalog metadata update
* Fetch all tables and databases across pages via pageToken in
LanceRestCatalog
* keep entered Iceberg storage/auth values when switching to Lance and back
* fixup style
---
.../amoro/server/catalog/CatalogBuilder.java | 19 ++++---
.../server/catalog/DefaultCatalogManager.java | 8 +++
.../catalog/TestDefaultCatalogManagerRecovery.java | 24 +++++++++
.../amoro/formats/lance/AbstractLanceCatalog.java | 18 ++-----
.../formats/lance/LanceDirectoryV1Catalog.java | 25 +++++++++
.../amoro/formats/lance/LanceRestCatalog.java | 60 +++++++++++++++++++---
amoro-web/src/views/catalogs/Detail.vue | 14 -----
7 files changed, 127 insertions(+), 41 deletions(-)
diff --git
a/amoro-ams/src/main/java/org/apache/amoro/server/catalog/CatalogBuilder.java
b/amoro-ams/src/main/java/org/apache/amoro/server/catalog/CatalogBuilder.java
index 5c5594e56..408a8c34a 100644
---
a/amoro-ams/src/main/java/org/apache/amoro/server/catalog/CatalogBuilder.java
+++
b/amoro-ams/src/main/java/org/apache/amoro/server/catalog/CatalogBuilder.java
@@ -86,13 +86,7 @@ public class CatalogBuilder {
"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);
+ checkRestLanceExclusivity(type, tableFormats);
switch (type) {
case CATALOG_TYPE_HADOOP:
@@ -108,4 +102,15 @@ public class CatalogBuilder {
throw new IllegalStateException("unsupported catalog type:" + type);
}
}
+
+ /** Enforced on both catalog creation and metadata updates. */
+ static void checkRestLanceExclusivity(String type, Set<TableFormat>
tableFormats) {
+ 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);
+ }
}
diff --git
a/amoro-ams/src/main/java/org/apache/amoro/server/catalog/DefaultCatalogManager.java
b/amoro-ams/src/main/java/org/apache/amoro/server/catalog/DefaultCatalogManager.java
index 6535283da..2ea31ff2a 100644
---
a/amoro-ams/src/main/java/org/apache/amoro/server/catalog/DefaultCatalogManager.java
+++
b/amoro-ams/src/main/java/org/apache/amoro/server/catalog/DefaultCatalogManager.java
@@ -24,6 +24,7 @@ import static
org.apache.amoro.properties.CatalogMetaProperties.CLIENT_POOL_SIZE
import org.apache.amoro.AmoroTable;
import org.apache.amoro.Constants;
+import org.apache.amoro.TableFormat;
import org.apache.amoro.api.CatalogMeta;
import org.apache.amoro.config.Configurations;
import org.apache.amoro.exception.AlreadyExistsException;
@@ -49,6 +50,7 @@ import java.time.Duration;
import java.util.List;
import java.util.Map;
import java.util.Optional;
+import java.util.Set;
import java.util.concurrent.ExecutionException;
import java.util.stream.Collectors;
@@ -273,6 +275,12 @@ public class DefaultCatalogManager extends PersistentBase
implements CatalogMana
if (!oldType.equals(newType)) {
throw new IllegalMetadataException("Cannot update catalog type");
}
+
+ // A REST catalog serves a single protocol per uri, so Lance cannot be
combined with other
+ // table formats. Reject the update before it is persisted.
+ String type = newMeta.getCatalogType();
+ Set<TableFormat> tableFormats = CatalogUtil.tableFormats(newMeta);
+ CatalogBuilder.checkRestLanceExclusivity(type, tableFormats);
}
private void disposeCatalog(String name) {
diff --git
a/amoro-ams/src/test/java/org/apache/amoro/server/catalog/TestDefaultCatalogManagerRecovery.java
b/amoro-ams/src/test/java/org/apache/amoro/server/catalog/TestDefaultCatalogManagerRecovery.java
index 453430309..b0b79bf43 100644
---
a/amoro-ams/src/test/java/org/apache/amoro/server/catalog/TestDefaultCatalogManagerRecovery.java
+++
b/amoro-ams/src/test/java/org/apache/amoro/server/catalog/TestDefaultCatalogManagerRecovery.java
@@ -124,4 +124,28 @@ public class TestDefaultCatalogManagerRecovery extends
AMSManagerTestBase {
"http://localhost:2/also-unreachable";,
readMeta.getCatalogProperties().get("uri"));
Assert.assertEquals("new-value",
readMeta.getCatalogProperties().get("new-property"));
}
+
+ /**
+ * Verifies that updating a REST catalog to combine Lance with another table
format is rejected
+ * before the update is persisted — the same invariant CatalogBuilder
enforces at creation time.
+ */
+ @Test
+ public void testUpdateCatalogRejectsLanceWithOtherFormats() {
+ // use iceberg with REST catalog
+ DB.insertCatalog(buildUnreachableRestCatalog());
+ Assert.assertTrue(CATALOG_MANAGER.catalogExist(TEST_CATALOG_NAME));
+
+ // add lance format is invalid
+ CatalogMeta updatedMeta =
CATALOG_MANAGER.getCatalogMeta(TEST_CATALOG_NAME);
+
updatedMeta.getCatalogProperties().put(CatalogMetaProperties.TABLE_FORMATS,
"LANCE,ICEBERG");
+
+ Assert.assertThrows(
+ IllegalStateException.class, () ->
CATALOG_MANAGER.updateCatalog(updatedMeta));
+
+ // The invalid update must not be persisted
+ CatalogMeta readMeta = CATALOG_MANAGER.getCatalogMeta(TEST_CATALOG_NAME);
+ Assert.assertEquals(
+ TableFormat.ICEBERG.name(),
+
readMeta.getCatalogProperties().get(CatalogMetaProperties.TABLE_FORMATS));
+ }
}
diff --git
a/amoro-format-lance/src/main/java/org/apache/amoro/formats/lance/AbstractLanceCatalog.java
b/amoro-format-lance/src/main/java/org/apache/amoro/formats/lance/AbstractLanceCatalog.java
index 8ba7e1fc0..420299c3a 100644
---
a/amoro-format-lance/src/main/java/org/apache/amoro/formats/lance/AbstractLanceCatalog.java
+++
b/amoro-format-lance/src/main/java/org/apache/amoro/formats/lance/AbstractLanceCatalog.java
@@ -27,11 +27,8 @@ 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;
@@ -91,17 +88,10 @@ public abstract class AbstractLanceCatalog implements
FormatCatalog {
}
@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());
- }
+ public abstract List<String> listTables(String database);
+
+ @Override
+ public abstract List<String> listDatabases();
protected void validateDatabase(String database) {
if (!databaseExists(database)) {
diff --git
a/amoro-format-lance/src/main/java/org/apache/amoro/formats/lance/LanceDirectoryV1Catalog.java
b/amoro-format-lance/src/main/java/org/apache/amoro/formats/lance/LanceDirectoryV1Catalog.java
index 19f8f2b08..fcabb8d71 100755
---
a/amoro-format-lance/src/main/java/org/apache/amoro/formats/lance/LanceDirectoryV1Catalog.java
+++
b/amoro-format-lance/src/main/java/org/apache/amoro/formats/lance/LanceDirectoryV1Catalog.java
@@ -24,7 +24,10 @@ import org.apache.arrow.util.Preconditions;
import org.apache.iceberg.aliyun.AliyunProperties;
import org.apache.iceberg.aws.s3.S3FileIOProperties;
import org.lance.namespace.LanceNamespace;
+import org.lance.namespace.model.ListTablesRequest;
+import org.lance.namespace.model.ListTablesResponse;
+import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
@@ -110,6 +113,28 @@ public class LanceDirectoryV1Catalog extends
AbstractLanceCatalog {
throw new UnsupportedOperationException("Dropping Lance databases is not
supported.");
}
+ @Override
+ public List<String> listTables(String database) {
+ if (!databaseExists(database)) {
+ return Collections.emptyList();
+ }
+ return listTablesFromNamespace();
+ }
+
+ private List<String> listTablesFromNamespace() {
+ if (namespace == null) {
+ return Collections.emptyList();
+ }
+
+ ListTablesRequest request = new
ListTablesRequest().id(Collections.emptyList());
+ ListTablesResponse response = namespace.listTables(request);
+ if (response == null) {
+ return Collections.emptyList();
+ }
+
+ return new ArrayList<>(response.getTables());
+ }
+
private static String removeFirstNonNull(Map<String, String> properties,
String... keys) {
String value = null;
for (String key : keys) {
diff --git
a/amoro-format-lance/src/main/java/org/apache/amoro/formats/lance/LanceRestCatalog.java
b/amoro-format-lance/src/main/java/org/apache/amoro/formats/lance/LanceRestCatalog.java
index fdf237c12..c665f16f1 100644
---
a/amoro-format-lance/src/main/java/org/apache/amoro/formats/lance/LanceRestCatalog.java
+++
b/amoro-format-lance/src/main/java/org/apache/amoro/formats/lance/LanceRestCatalog.java
@@ -21,6 +21,7 @@ package org.apache.amoro.formats.lance;
import org.apache.amoro.AlreadyExistsException;
import org.apache.arrow.memory.RootAllocator;
import org.apache.arrow.util.Preconditions;
+import org.apache.commons.lang3.StringUtils;
import org.lance.namespace.LanceNamespace;
import org.lance.namespace.errors.NamespaceAlreadyExistsException;
import org.lance.namespace.errors.NamespaceNotFoundException;
@@ -28,13 +29,17 @@ import org.lance.namespace.model.CreateNamespaceRequest;
import org.lance.namespace.model.DropNamespaceRequest;
import org.lance.namespace.model.ListNamespacesRequest;
import org.lance.namespace.model.ListNamespacesResponse;
+import org.lance.namespace.model.ListTablesRequest;
+import org.lance.namespace.model.ListTablesResponse;
import org.lance.namespace.model.NamespaceExistsRequest;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
+import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
+import java.util.Set;
/**
* REST catalog implementation for Lance.
@@ -95,14 +100,57 @@ public class LanceRestCatalog extends AbstractLanceCatalog
{
return id;
}
+ /**
+ * Lists the tables of a database. The REST service pages results via
pageToken, so follow the
+ * token until it is empty to avoid silently missing tables on large
catalogs.
+ */
+ @Override
+ public List<String> listTables(String database) {
+ validateDatabase(database);
+
+ Set<String> tables = new LinkedHashSet<>();
+ String pageToken = null;
+ do {
+ ListTablesRequest request = new
ListTablesRequest().id(tableIdForListTables(database));
+ if (pageToken != null) {
+ request.pageToken(pageToken);
+ }
+ ListTablesResponse response = namespace.listTables(request);
+ if (response == null) {
+ break;
+ }
+ if (response.getTables() != null) {
+ tables.addAll(response.getTables());
+ }
+
+ pageToken = response.getPageToken();
+ } while (StringUtils.isNotEmpty(pageToken));
+
+ return new ArrayList<>(tables);
+ }
+
+ /** Lists the schemas under the catalog, following the REST pageToken until
it is empty. */
@Override
public List<String> listDatabases() {
- ListNamespacesRequest request = new
ListNamespacesRequest().id(catalogPrefix);
- ListNamespacesResponse response = namespace.listNamespaces(request);
- if (response == null) {
- return Collections.emptyList();
- }
- return new ArrayList<>(response.getNamespaces());
+ Set<String> namespaces = new LinkedHashSet<>();
+ String pageToken = null;
+ do {
+ ListNamespacesRequest request = new
ListNamespacesRequest().id(catalogPrefix);
+ if (pageToken != null) {
+ request.pageToken(pageToken);
+ }
+ ListNamespacesResponse response = namespace.listNamespaces(request);
+ if (response == null) {
+ break;
+ }
+ if (response.getNamespaces() != null) {
+ namespaces.addAll(response.getNamespaces());
+ }
+
+ pageToken = response.getPageToken();
+ } while (StringUtils.isNotEmpty(pageToken));
+
+ return new ArrayList<>(namespaces);
}
@Override
diff --git a/amoro-web/src/views/catalogs/Detail.vue
b/amoro-web/src/views/catalogs/Detail.vue
index 6f78999be..ce6bd5573 100644
--- a/amoro-web/src/views/catalogs/Detail.vue
+++ b/amoro-web/src/views/catalogs/Detail.vue
@@ -436,20 +436,6 @@ watch(
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]
- })
- }
- else if (isRest && !formState.storageConfig['storage.type']) {
- formState.storageConfig['storage.type'] = 'S3'
}
},
)
