github-actions[bot] commented on code in PR #69384:
URL: https://github.com/apache/doris/pull/69384#discussion_r4359334975
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/AlterTableCommand.java:
##########
@@ -270,6 +270,21 @@ static void checkColumnOperationsSupported(TableIf table,
List<AlterTableOp> alt
}
return;
}
+ if (table instanceof LanceExternalTable) {
Review Comment:
[P1] Route Lance tables through the external ALTER dispatcher
`ALTER TABLE lance.analytics.events ADD COLUMN score INT NULL` passes these
new checks, but `AlterTableCommand.run()` then calls `Env.alterTable()` ->
`Alter.processAlterTable()`. That switch has no `LANCE_EXTERNAL_TABLE` case and
throws `Do not support alter LANCE_EXTERNAL_TABLE table[events]` instead of
invoking `processAlterTableForExternalTable`. Consequently every new
ADD/MODIFY/RENAME/DROP COLUMN operation, and REST RENAME TABLE, fails before
reaching `LanceMetadataOps`; the new regression stops at its first ADD. Add
Lance to the external-table dispatch and cover the full command execution path.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceManifestPaths.java:
##########
@@ -108,15 +111,72 @@ static String objectStorePath(String location) {
throw new IllegalArgumentException("Lance namespace returned no
table location");
}
if (!URL_SCHEME.matcher(location).find()) {
- return StringUtils.strip(location, "/");
+ return normalizeObjectPath(StringUtils.strip(location, "/"));
}
String path =
StringUtils.substringBefore(StringUtils.substringBefore(location, "?"), "#");
path = path.substring(path.indexOf(':') + 1);
if (path.startsWith("//")) {
int slash = path.indexOf('/', 2);
path = slash < 0 ? "" : path.substring(slash);
}
- return StringUtils.strip(percentDecode(path), "/");
+ return normalizeObjectPath(StringUtils.strip(percentDecode(path),
"/"));
+ }
+
+ private static String normalizeObjectPath(String path) {
+ if (path.isEmpty()) {
+ return "";
+ }
+ ArrayDeque<String> segments = new ArrayDeque<>();
+ for (String segment : path.split("/+")) {
Review Comment:
[P2] Preserve URI segments until dot-segment resolution
Collapsing `/+` before resolving `..` differs from the pinned SDK's [URL
parser](https://github.com/servo/rust-url/blob/v2.5.8/url/src/parser.rs). For a
managed response with `location=s3://b/items.lance` and
`table_uri=s3://b/warehouse//../items.lance`, Doris normalizes both to
`s3://b/items.lance`, whereas Lance resolves the latter to
`s3://b/warehouse/items.lance`: `..` consumes the empty segment. The shared
`objectStorePath()` also accepts a namespace version-3 manifest at
`items.lance/_versions/3.manifest`. Since `openDataset()` passes the original
URI to Lance, a dataset at `warehouse/items.lance` with version 3 is read
instead of the namespace's dataset. Match the SDK's segment/decoding order, or
reject locators whose equivalence cannot be established, rather than weakening
both identity checks.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceMetadataOps.java:
##########
@@ -0,0 +1,668 @@
+// 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.doris.datasource.lance;
+
+import org.apache.doris.analysis.ColumnPosition;
+import org.apache.doris.catalog.ArrayType;
+import org.apache.doris.catalog.Column;
+import org.apache.doris.catalog.Env;
+import org.apache.doris.catalog.MapType;
+import org.apache.doris.catalog.PrimitiveType;
+import org.apache.doris.catalog.ScalarType;
+import org.apache.doris.catalog.StructField;
+import org.apache.doris.catalog.StructType;
+import org.apache.doris.catalog.Type;
+import org.apache.doris.common.DdlException;
+import org.apache.doris.common.ErrorCode;
+import org.apache.doris.common.ErrorReport;
+import org.apache.doris.common.UserException;
+import org.apache.doris.datasource.ExternalDatabase;
+import org.apache.doris.datasource.ExternalObjectLog;
+import org.apache.doris.datasource.ExternalTable;
+import org.apache.doris.datasource.lance.metadata.LanceTypeConverter;
+import org.apache.doris.datasource.operations.ExternalMetadataOps;
+import
org.apache.doris.nereids.trees.plans.commands.info.CreateOrReplaceBranchInfo;
+import
org.apache.doris.nereids.trees.plans.commands.info.CreateOrReplaceTagInfo;
+import org.apache.doris.nereids.trees.plans.commands.info.CreateTableInfo;
+import org.apache.doris.nereids.trees.plans.commands.info.DropBranchInfo;
+import org.apache.doris.nereids.trees.plans.commands.info.DropTagInfo;
+
+import org.apache.arrow.vector.types.pojo.Schema;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.lance.namespace.errors.NamespaceAlreadyExistsException;
+import org.lance.namespace.errors.NamespaceNotFoundException;
+import org.lance.namespace.errors.TableAlreadyExistsException;
+import org.lance.namespace.errors.TableNotFoundException;
+import org.lance.namespace.model.AddColumnsEntry;
+import org.lance.namespace.model.AlterColumnsEntry;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.TreeSet;
+
+/** Doris external metadata operations backed by the Lance Namespace API. */
+public class LanceMetadataOps implements ExternalMetadataOps {
+ private static final Logger LOG =
LogManager.getLogger(LanceMetadataOps.class);
+ private static final String TABLE_COMMENT_PROPERTY = "comment";
+
+ private final LanceExternalCatalog catalog;
+
+ public LanceMetadataOps(LanceExternalCatalog catalog) {
+ this.catalog = catalog;
+ }
+
+ @Override
+ public boolean createDbImpl(String dbName, boolean ifNotExists,
Map<String, String> properties)
+ throws DdlException {
+ return execute("Failed to create Lance database " + dbName, client -> {
+ if (client.isRootDatabase(dbName)) {
+ throw new DdlException("Cannot create the configured Lance
root database: " + dbName);
+ }
+ ExternalDatabase<?> cachedDb = catalog.getDbNullable(dbName);
+ if (cachedDb != null) {
+ if (!cachedDb.getRemoteName().equals(dbName)) {
+ if (ifNotExists) {
+ return true;
+ }
+
ErrorReport.reportDdlException(ErrorCode.ERR_DB_CREATE_EXISTS, dbName);
+ }
+ if (client.databaseExists(cachedDb.getRemoteName())) {
+ if (ifNotExists) {
+ return true;
+ }
+
ErrorReport.reportDdlException(ErrorCode.ERR_DB_CREATE_EXISTS, dbName);
+ }
+ catalog.unregisterDatabase(cachedDb.getFullName());
+ catalog.resetMetaCacheNames();
+ }
+ if (client.databaseExists(dbName)) {
+ if (ifNotExists) {
+ catalog.resetMetaCacheNames();
+ return true;
+ }
+ ErrorReport.reportDdlException(ErrorCode.ERR_DB_CREATE_EXISTS,
dbName);
+ }
+ try {
+ client.createDatabase(dbName, new HashMap<>(
+
Optional.ofNullable(properties).orElse(Collections.emptyMap())));
+ return false;
+ } catch (NamespaceAlreadyExistsException e) {
+ if (ifNotExists) {
+ catalog.resetMetaCacheNames();
+ return true;
+ }
+ ErrorReport.reportDdlException(ErrorCode.ERR_DB_CREATE_EXISTS,
dbName);
+ throw new IllegalStateException("unreachable");
+ }
+ });
+ }
+
+ @Override
+ public void afterCreateDb(String dbName) {
+ catalog.retireCachedDatabaseForNoOp(dbName);
+ catalog.resetMetaCacheNames();
+ }
+
+ @Override
+ public boolean dropDbImpl(String dbName, boolean ifExists, boolean force)
throws DdlException {
+ if (force &&
LanceExternalCatalog.LANCE_FILESYSTEM.equals(catalog.getLanceCatalogType())) {
+ throw new DdlException(
+ "DROP DATABASE FORCE is not supported for Lance filesystem
catalogs");
+ }
+ ExternalDatabase<?> db = catalog.getDbNullable(dbName);
+ return execute("Failed to drop Lance database " + dbName, client -> {
+ if (client.isRootDatabase(dbName)) {
+ throw new DdlException("Cannot drop the configured Lance root
database: " + dbName);
+ }
+ if (db == null) {
+ if (ifExists) {
+ return false;
+ }
+ ErrorReport.reportDdlException(ErrorCode.ERR_DB_DROP_EXISTS,
dbName);
+ return false;
+ }
+ String remoteDbName = db.getRemoteName();
+ if (client.isRootDatabase(remoteDbName)) {
+ throw new DdlException("Cannot drop the configured Lance root
database: " + dbName);
+ }
+ if (!client.databaseExists(remoteDbName)) {
+ if (ifExists) {
+ return false;
+ }
+ ErrorReport.reportDdlException(ErrorCode.ERR_DB_DROP_EXISTS,
dbName);
+ }
+ try {
+ client.dropDatabase(remoteDbName, ifExists, force);
+ } catch (NamespaceNotFoundException e) {
+ if (ifExists) {
+ return false;
+ }
+ ErrorReport.reportDdlException(ErrorCode.ERR_DB_DROP_EXISTS,
dbName);
+ }
+ return true;
+ });
+ }
+
+ @Override
+ public void afterDropDb(String dbName) {
+ Optional<ExternalDatabase<? extends ExternalTable>> db =
catalog.getDbForReplay(dbName);
+ if (db.isPresent()) {
+ catalog.unregisterDatabase(db.get().getFullName());
Review Comment:
[P2] Retire descendant databases after a cascading namespace drop
On a REST catalog, warm the `parent` and `parent.child` database objects and
the `SHOW DATABASES` names snapshot, then execute `DROP DATABASE catalog.parent
FORCE`. The request uses `Cascade`, which [also deletes child
namespaces](https://github.com/lance-format/lance-namespace/blob/v0.11.1/java/lance-namespace-apache-client/src/main/java/org/lance/namespace/model/DropNamespaceRequest.java),
but this success hook unregisters only `parent` and returns.
`listDatabaseNames()` exposes descendants as separate Doris databases, and
`MetaCache.invalidate()` removes only the supplied name, so deleted
`parent.child` remains listed and cached; replay on warm followers has the same
gap. Retire the affected descendant identities and reset names after a
successful cascade. This differs from the already-fixed no-op DROP names
reconciliation.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceMetadataOps.java:
##########
@@ -0,0 +1,668 @@
+// 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.doris.datasource.lance;
+
+import org.apache.doris.analysis.ColumnPosition;
+import org.apache.doris.catalog.ArrayType;
+import org.apache.doris.catalog.Column;
+import org.apache.doris.catalog.Env;
+import org.apache.doris.catalog.MapType;
+import org.apache.doris.catalog.PrimitiveType;
+import org.apache.doris.catalog.ScalarType;
+import org.apache.doris.catalog.StructField;
+import org.apache.doris.catalog.StructType;
+import org.apache.doris.catalog.Type;
+import org.apache.doris.common.DdlException;
+import org.apache.doris.common.ErrorCode;
+import org.apache.doris.common.ErrorReport;
+import org.apache.doris.common.UserException;
+import org.apache.doris.datasource.ExternalDatabase;
+import org.apache.doris.datasource.ExternalObjectLog;
+import org.apache.doris.datasource.ExternalTable;
+import org.apache.doris.datasource.lance.metadata.LanceTypeConverter;
+import org.apache.doris.datasource.operations.ExternalMetadataOps;
+import
org.apache.doris.nereids.trees.plans.commands.info.CreateOrReplaceBranchInfo;
+import
org.apache.doris.nereids.trees.plans.commands.info.CreateOrReplaceTagInfo;
+import org.apache.doris.nereids.trees.plans.commands.info.CreateTableInfo;
+import org.apache.doris.nereids.trees.plans.commands.info.DropBranchInfo;
+import org.apache.doris.nereids.trees.plans.commands.info.DropTagInfo;
+
+import org.apache.arrow.vector.types.pojo.Schema;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.lance.namespace.errors.NamespaceAlreadyExistsException;
+import org.lance.namespace.errors.NamespaceNotFoundException;
+import org.lance.namespace.errors.TableAlreadyExistsException;
+import org.lance.namespace.errors.TableNotFoundException;
+import org.lance.namespace.model.AddColumnsEntry;
+import org.lance.namespace.model.AlterColumnsEntry;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.TreeSet;
+
+/** Doris external metadata operations backed by the Lance Namespace API. */
+public class LanceMetadataOps implements ExternalMetadataOps {
+ private static final Logger LOG =
LogManager.getLogger(LanceMetadataOps.class);
+ private static final String TABLE_COMMENT_PROPERTY = "comment";
+
+ private final LanceExternalCatalog catalog;
+
+ public LanceMetadataOps(LanceExternalCatalog catalog) {
+ this.catalog = catalog;
+ }
+
+ @Override
+ public boolean createDbImpl(String dbName, boolean ifNotExists,
Map<String, String> properties)
+ throws DdlException {
+ return execute("Failed to create Lance database " + dbName, client -> {
+ if (client.isRootDatabase(dbName)) {
+ throw new DdlException("Cannot create the configured Lance
root database: " + dbName);
+ }
+ ExternalDatabase<?> cachedDb = catalog.getDbNullable(dbName);
+ if (cachedDb != null) {
+ if (!cachedDb.getRemoteName().equals(dbName)) {
+ if (ifNotExists) {
+ return true;
+ }
+
ErrorReport.reportDdlException(ErrorCode.ERR_DB_CREATE_EXISTS, dbName);
+ }
+ if (client.databaseExists(cachedDb.getRemoteName())) {
+ if (ifNotExists) {
+ return true;
+ }
+
ErrorReport.reportDdlException(ErrorCode.ERR_DB_CREATE_EXISTS, dbName);
+ }
+ catalog.unregisterDatabase(cachedDb.getFullName());
+ catalog.resetMetaCacheNames();
+ }
+ if (client.databaseExists(dbName)) {
+ if (ifNotExists) {
+ catalog.resetMetaCacheNames();
+ return true;
+ }
+ ErrorReport.reportDdlException(ErrorCode.ERR_DB_CREATE_EXISTS,
dbName);
+ }
+ try {
+ client.createDatabase(dbName, new HashMap<>(
+
Optional.ofNullable(properties).orElse(Collections.emptyMap())));
+ return false;
+ } catch (NamespaceAlreadyExistsException e) {
+ if (ifNotExists) {
+ catalog.resetMetaCacheNames();
+ return true;
+ }
+ ErrorReport.reportDdlException(ErrorCode.ERR_DB_CREATE_EXISTS,
dbName);
+ throw new IllegalStateException("unreachable");
+ }
+ });
+ }
+
+ @Override
+ public void afterCreateDb(String dbName) {
+ catalog.retireCachedDatabaseForNoOp(dbName);
+ catalog.resetMetaCacheNames();
+ }
+
+ @Override
+ public boolean dropDbImpl(String dbName, boolean ifExists, boolean force)
throws DdlException {
+ if (force &&
LanceExternalCatalog.LANCE_FILESYSTEM.equals(catalog.getLanceCatalogType())) {
+ throw new DdlException(
+ "DROP DATABASE FORCE is not supported for Lance filesystem
catalogs");
+ }
+ ExternalDatabase<?> db = catalog.getDbNullable(dbName);
+ return execute("Failed to drop Lance database " + dbName, client -> {
+ if (client.isRootDatabase(dbName)) {
+ throw new DdlException("Cannot drop the configured Lance root
database: " + dbName);
+ }
+ if (db == null) {
+ if (ifExists) {
+ return false;
+ }
+ ErrorReport.reportDdlException(ErrorCode.ERR_DB_DROP_EXISTS,
dbName);
+ return false;
+ }
+ String remoteDbName = db.getRemoteName();
+ if (client.isRootDatabase(remoteDbName)) {
+ throw new DdlException("Cannot drop the configured Lance root
database: " + dbName);
+ }
+ if (!client.databaseExists(remoteDbName)) {
+ if (ifExists) {
+ return false;
+ }
+ ErrorReport.reportDdlException(ErrorCode.ERR_DB_DROP_EXISTS,
dbName);
+ }
+ try {
+ client.dropDatabase(remoteDbName, ifExists, force);
+ } catch (NamespaceNotFoundException e) {
+ if (ifExists) {
+ return false;
+ }
+ ErrorReport.reportDdlException(ErrorCode.ERR_DB_DROP_EXISTS,
dbName);
+ }
+ return true;
+ });
+ }
+
+ @Override
+ public void afterDropDb(String dbName) {
+ Optional<ExternalDatabase<? extends ExternalTable>> db =
catalog.getDbForReplay(dbName);
+ if (db.isPresent()) {
+ catalog.unregisterDatabase(db.get().getFullName());
+ return;
+ }
+ catalog.unregisterDatabase(dbName);
+ catalog.retireAllDatabaseObjectsWithoutEngineInvalidation();
+ }
+
+ @Override
+ public void afterDropDbNoOp(String dbName) {
+ catalog.invalidateTableAccessCache();
+ catalog.retireAllDatabaseObjectsWithoutEngineInvalidation();
Review Comment:
[P2] Invalidate schema entries during no-op database reconciliation
After caching the schema of `analytics.events`, delete `analytics`
externally and run `DROP DATABASE IF EXISTS lance.analytics`. This hook retires
database objects with engine invalidation explicitly disabled and resets
names/access, leaving the independent default schema cache intact. If
`analytics.events` is recreated externally under the same names with a
different schema, replacement objects use the same name-based `SchemaCacheKey`,
so the leader's next `DESC` still returns the old columns until expiry or
refresh. A follower replaying `afterDropDb()` performs engine invalidation
instead. Invalidate the affected schema entries as part of no-op cleanup; the
names-only fix requested in existing thread 4344273843 does not clear this
separate cache.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
