Gabriel39 commented on code in PR #68349: URL: https://github.com/apache/doris/pull/68349#discussion_r4091557534
########## fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceMetadataOps.java: ########## @@ -0,0 +1,520 @@ +// 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.Column; +import org.apache.doris.catalog.Env; +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.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.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() { + catalog.resetMetaCacheNames(); + } + + @Override + public boolean dropDbImpl(String dbName, boolean ifExists, boolean force) throws DdlException { + ExternalDatabase<?> db = catalog.getDbNullable(dbName); + String remoteDbName = db == null ? dbName : db.getRemoteName(); Review Comment: [P1] Do not fall back to a remote namespace name when local resolution fails Resolving getRemoteName() fixes the normal mapped-name case, but the db == null fallback still allows a DROP to target a different namespace from the Doris name being addressed. For example, with remote Sales mapped to local sales_db, DROP DATABASE IF EXISTS Sales FORCE cannot resolve a local database named Sales, but this fallback proceeds to delete remote Sales, which is exposed as sales_db. getDbNullable() can also return null after a metadata-loading failure; that is not evidence that the input name is a valid remote target. Please stop the mutation when the local database cannot be resolved rather than treating dbName as a remote identifier. Add a regression for the unmapped original name of a mapped namespace, verifying that no dropNamespace request is sent. ########## fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceMetadataOps.java: ########## @@ -0,0 +1,520 @@ +// 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.Column; +import org.apache.doris.catalog.Env; +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.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.databaseExists(dbName)) { Review Comment: [P1] Check existing local database names before creating a namespace This existence check only checks the remote spelling. With lower_case_meta_names=true and an existing remote namespace Sales (exposed as sales), CREATE DATABASE IF NOT EXISTS sales checks for remote sales, finds none, and creates it. Sales and sales then map to the same local name, causing getFilteredDatabaseNames() to report a case-insensitive database-name conflict. An operation that should be a no-op can therefore make catalog metadata lookup fail. Please check whether the requested local name already resolves to a database before creating the remote namespace, preserving IF NOT EXISTS behavior and rejecting local-name conflicts. Cover mixed-case remote namespaces and explicit name mappings in tests. ########## fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceMetadataOps.java: ########## @@ -0,0 +1,520 @@ +// 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.Column; +import org.apache.doris.catalog.Env; +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.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.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() { + catalog.resetMetaCacheNames(); + } + + @Override + public boolean dropDbImpl(String dbName, boolean ifExists, boolean force) throws DdlException { + ExternalDatabase<?> db = catalog.getDbNullable(dbName); + String remoteDbName = db == null ? dbName : db.getRemoteName(); + return execute("Failed to drop Lance database " + dbName, client -> { + 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) { + catalog.unregisterDatabase(dbName); Review Comment: [P2] Retire the database cache using its canonical local name The drop now resolves the remote name, but the post-drop hook still unregisters the caller spelling. With lower_case_database_names=2, a cached database named Sales can be resolved and dropped by DROP DATABASE sales. unregisterDatabase("sales") then misses the cached Sales object: MetaCache.invalidate() removes the exact localName key and the ID derived from that spelling. The old database object and its table-name cache can survive the remote drop and be reused after recreation. The same hook runs during follower replay. Please resolve the cached database through getDbForReplay() and invalidate its canonical local name. Handle an unresolved replay name conservatively, and add coverage for alternate-case DROP followed by recreation and for follower replay. -- 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]
