Gabriel39 commented on code in PR #68349: URL: https://github.com/apache/doris/pull/68349#discussion_r4091052635
########## fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceMetadataOps.java: ########## @@ -0,0 +1,532 @@ +// 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 void dropDbImpl(String dbName, boolean ifExists, boolean force) throws DdlException { Review Comment: [P1] Match the current dropDbImpl return contract ExternalMetadataOps.dropDbImpl(String, boolean, boolean) returns boolean in this same revision, but this implementation returns void. This prevents LanceMetadataOps from compiling. Please return true when the remote namespace was actually dropped and false for an IF EXISTS no-op, as required by ExternalMetadataOps.dropDb() and ExternalCatalog.dropDb() to decide whether to journal the operation. The reported compilation result needs to be revalidated against this head. ########## fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceMetadataOps.java: ########## @@ -0,0 +1,532 @@ +// 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 void dropDbImpl(String dbName, boolean ifExists, boolean force) throws DdlException { + execute("Failed to drop Lance database " + dbName, client -> { + if (client.isRootDatabase(dbName)) { + throw new DdlException("Cannot drop the configured Lance root database: " + dbName); + } + if (!client.databaseExists(dbName)) { + if (ifExists) { + return null; + } + ErrorReport.reportDdlException(ErrorCode.ERR_DB_DROP_EXISTS, dbName); + } + try { + client.dropDatabase(dbName, ifExists, force); Review Comment: [P1] Resolve the remote namespace before dropping a mapped database The dbName passed to this method is the Doris database name, but databaseExists(), isRootDatabase(), and dropDatabase() interpret it as the remote namespace name. With meta_names_mapping or lower_case_meta_names, these can differ. For example, if remote Sales is exposed as sales_db, DROP DATABASE sales_db targets remote sales_db instead of Sales. If that remote name is absent, the command fails or silently skips under IF EXISTS; if it exists independently, FORCE can delete the wrong namespace and its contents. Please resolve the ExternalDatabase and use its getRemoteName() for the remote existence check, root-namespace guard, and drop request. Use the canonical local name for cache retirement. Add coverage for mapped names and mixed-case remote namespaces. ########## fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceNamespaceClient.java: ########## @@ -198,6 +276,86 @@ boolean tableExists(String dbName, String tblName) { } } + void createTable(String dbName, String tableName, Map<String, String> properties, + byte[] arrowStream) { + try { + CreateTableRequest request = new CreateTableRequest() + .id(buildTableId(dbName, tableName)) + .mode("Create") + .properties(properties == null ? Collections.emptyMap() : properties) + .storageOptions(namespaceStorageOptions); + synchronized (namespaceLock) { + namespace.createTable(request, arrowStream); + } + } catch (DdlException e) { + throw new RuntimeException(e); + } + } + + void dropTable(String dbName, String tableName) { + try { + DropTableRequest request = new DropTableRequest().id(buildTableId(dbName, tableName)); + synchronized (namespaceLock) { + namespace.dropTable(request); + } + } catch (DdlException e) { + throw new RuntimeException(e); + } + } + + void renameTable(String dbName, String oldTableName, String newTableName) { + try { + RenameTableRequest request = new RenameTableRequest() + .id(buildTableId(dbName, oldTableName)) + .newNamespaceId(buildNamespaceId(dbName)) + .newTableName(newTableName); + synchronized (namespaceLock) { + namespace.renameTable(request); Review Comment: [P1] Do not expose filesystem rename through an unimplemented SDK operation With the pinned Lance 11.0.0 dependency, DirectoryNamespace.renameTable() forwards through JNI to inner.rename_table(), but the Rust DirectoryNamespace implementation does not override that method. It therefore reaches the trait default, which returns "rename_table not implemented": https://github.com/lance-format/lance/blob/v11.0.0/rust/lance-namespace/src/namespace.rs#L288 Consequently the filesystem ALTER TABLE ... RENAME in the new regression suite cannot succeed, even though the mocked request test passes. Please either provide and pin a supported SDK implementation, or explicitly reject filesystem rename and adjust the advertised support and regression expectations. -- 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]
