morningman commented on code in PR #30198: URL: https://github.com/apache/doris/pull/30198#discussion_r1477743313
########## fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogDatabase.java: ########## @@ -0,0 +1,22 @@ +// 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; + +public interface CatalogDatabase { Review Comment: This name is confused. How about `DatabaseMetadata`? ########## fe/fe-core/src/main/java/org/apache/doris/catalog/DatabaseIf.java: ########## @@ -265,7 +265,9 @@ default OlapTable getOlapTableOrAnalysisException(String tableName) throws Analy return (OlapTable) table; } - void dropTable(String tableName); + boolean registerTable(TableIf table); Review Comment: Add comment for interface ########## fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java: ########## @@ -1261,11 +1248,6 @@ public void createTableAsSelect(CreateTableAsSelectStmt stmt) throws DdlExceptio default: throw new DdlException("Unsupported string type for ctas"); } - if (resultExpr.getSrcSlotRef() != null Review Comment: Why removing this? ########## fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java: ########## @@ -2608,27 +2590,7 @@ private void createOlapTable(Database db, CreateTableStmt stmt) throws UserExcep "Only support dynamic partition properties on range partition table"); } } - // check the interval same between dynamic & auto range partition - DynamicPartitionProperty dynamicProperty = olapTable.getTableProperty() Review Comment: Check this ########## fe/fe-core/src/main/java/org/apache/doris/datasource/hive/HiveCatalogDatabase.java: ########## @@ -0,0 +1,32 @@ +// 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.hive; + +import org.apache.doris.datasource.CatalogDatabase; + +import lombok.Data; + +import java.util.Map; + +@Data +public class HiveCatalogDatabase implements CatalogDatabase { Review Comment: ```suggestion public class HiveDatabaseMetadata implements CatalogDatabase { ``` ########## fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java: ########## @@ -2489,7 +2471,7 @@ private void createOlapTable(Database db, CreateTableStmt stmt) throws UserExcep if (!col.getType().isFixedPointType() && !col.getType().isDateType()) { throw new DdlException("Sequence type only support integer types and date types"); } - olapTable.setSequenceMapCol(col.getName()); Review Comment: Check this ########## fe/fe-core/src/main/java/org/apache/doris/catalog/external/IcebergExternalTable.java: ########## @@ -53,8 +66,66 @@ protected synchronized void makeSureInitialized() { @Override public List<Column> initSchema() { - makeSureInitialized(); - return IcebergUtils.getSchema(catalog, dbName, name); + return HiveMetaStoreClientHelper.ugiDoAs(catalog.getConfiguration(), () -> { Review Comment: Why not using `IcebergUtils.getSchema(catalog, dbName, name);`? ########## fe/fe-core/src/main/java/org/apache/doris/catalog/external/ExternalDatabase.java: ########## @@ -382,10 +391,18 @@ public CatalogIf getCatalog() { } // Only used for sync hive metastore event - public void createTable(String tableName, long tableId) { - throw new NotImplementedException("createTable() is not implemented"); + // 统一到DatabaseIf Review Comment: English ########## fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java: ########## @@ -816,7 +813,7 @@ public void createExternalTableFromEvent(String dbName, String tableName, } return; } - + // TODO：防止event和catalog建表的tableID冲突 Review Comment: English ########## fe/fe-core/src/main/java/org/apache/doris/catalog/external/IcebergExternalTable.java: ########## @@ -31,11 +34,21 @@ import org.apache.doris.thrift.TTableDescriptor; import org.apache.doris.thrift.TTableType; +import com.google.common.collect.Lists; +import org.apache.iceberg.Schema; +import org.apache.iceberg.types.Types; + import java.util.HashMap; import java.util.List; +import java.util.Locale; import java.util.Optional; public class IcebergExternalTable extends ExternalTable { + + // https://iceberg.apache.org/spec/#schemas-and-data-types + // All time and timestamp values are stored with microsecond precision + public static final int ICEBERG_DATETIME_SCALE_MS = 6; Review Comment: Already defined in [IcebergUtils.java](https://github.com/apache/doris/pull/30198/files#diff-a29ebd5606d28f7d6e46ec397512d19ce9a495c1c31066ba8697a31d68de536d) ########## fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java: ########## @@ -702,6 +701,10 @@ public void recoverPartition(RecoverPartitionStmt recoverStmt) throws DdlExcepti } } + public void replayEraseDatabase(long dbId) throws DdlException { Review Comment: Unused? ########## fe/fe-core/src/main/java/org/apache/doris/catalog/external/ExternalDatabase.java: ########## @@ -372,8 +372,17 @@ public void gsonPostProcess() throws IOException { } @Override - public void dropTable(String tableName) { - throw new NotImplementedException("dropTable() is not implemented"); + public void unregisterTable(String tableName) { + LOG.debug("unregister table [{}]", tableName); + Long tableId = tableNameToId.remove(tableName); + if (tableId == null) { + LOG.warn("unregister table [{}] failed", tableName); Review Comment: ```suggestion LOG.warn("table [{}] does not exist when drop", tableName); ``` ########## fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogTable.java: ########## @@ -0,0 +1,28 @@ +// 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; + +import java.util.Map; + +public interface CatalogTable { Review Comment: ```suggestion public interface TableMetadata { ``` ########## fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java: ########## @@ -848,8 +850,6 @@ public void dropTable(DropTableStmt stmt) throws DdlException { } db.writeLockOrDdlException(); - watch.split(); - costTimes.put("1:dbWriteLock", watch.getSplitTime()); Review Comment: Why remove this watch? ########## fe/fe-core/src/main/java/org/apache/doris/datasource/hive/HiveCatalogTable.java: ########## @@ -0,0 +1,94 @@ +// 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.hive; + +import org.apache.doris.catalog.Column; +import org.apache.doris.datasource.CatalogTable; + +import org.apache.hadoop.hive.metastore.api.FieldSchema; + +import java.util.List; +import java.util.Map; + +public class HiveCatalogTable implements CatalogTable { Review Comment: ```suggestion public class HiveTableMetadata implements CatalogTable { ``` ########## fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java: ########## @@ -744,16 +744,16 @@ public void replayRefreshExternalTable(ExternalObjectLog log) { } } - public void dropExternalTable(String dbName, String tableName, String catalogName, boolean ignoreIfExists) + public void unloadExternalTable(String dbName, String tableName, String catalogName, boolean ignoreIfExists) Review Comment: I think we should unify the name, there is load/unload, register/unregister, create/drop/remove. Too confuse. I suggest names in this class: 1. register/unregisterExternalTable 2. register/unregisterExternalDatabase ########## fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java: ########## @@ -1835,7 +1818,6 @@ public void replayDropPartition(DropPartitionInfo info) throws MetaNotFoundExcep Env.getCurrentRecycleBin().setRecycleTimeByIdForReplay(partition.getId(), info.getRecycleTime()); } } - olapTable.updateVisibleVersionAndTime(info.getVersion(), info.getVersionTime()); Review Comment: Check this ########## fe/fe-core/src/main/java/org/apache/doris/datasource/hive/ExternalMetadataOps.java: ########## @@ -0,0 +1,44 @@ +// 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.hive; + +import org.apache.doris.analysis.CreateDbStmt; +import org.apache.doris.analysis.CreateTableStmt; +import org.apache.doris.analysis.DropDbStmt; +import org.apache.doris.analysis.DropTableStmt; +import org.apache.doris.common.DdlException; +import org.apache.doris.common.UserException; + +import java.util.List; + +public interface ExternalMetadataOps { Review Comment: Add comment for this class and all interfaces ########## fe/fe-core/src/main/java/org/apache/doris/datasource/hive/HiveMetadataOps.java: ########## @@ -0,0 +1,185 @@ +// 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.hive; + +import org.apache.doris.analysis.CreateDbStmt; +import org.apache.doris.analysis.CreateTableStmt; +import org.apache.doris.analysis.DropDbStmt; +import org.apache.doris.analysis.DropTableStmt; +import org.apache.doris.catalog.Env; +import org.apache.doris.catalog.JdbcResource; +import org.apache.doris.catalog.external.ExternalDatabase; +import org.apache.doris.catalog.external.NamedExternalTable; +import org.apache.doris.common.Config; +import org.apache.doris.common.DdlException; +import org.apache.doris.common.UserException; +import org.apache.doris.datasource.ExternalMetaIdMgr; +import org.apache.doris.datasource.HMSExternalCatalog; +import org.apache.doris.datasource.jdbc.client.JdbcClient; +import org.apache.doris.datasource.jdbc.client.JdbcClientConfig; + +import com.google.common.base.Preconditions; +import org.apache.hadoop.hive.conf.HiveConf; +import org.apache.hadoop.hive.metastore.api.FieldSchema; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + +public class HiveMetadataOps implements ExternalMetadataOps { + private static final Logger LOG = LogManager.getLogger(HiveMetadataOps.class); + private static final int MIN_CLIENT_POOL_SIZE = 8; + private JdbcClientConfig jdbcClientConfig; + private HiveConf hiveConf; + private HMSExternalCatalog catalog; + private HMSCachedClient client; + + public HiveMetadataOps(HiveConf hiveConf, JdbcClientConfig jdbcClientConfig, HMSExternalCatalog catalog) { + this.hiveConf = hiveConf; + this.jdbcClientConfig = jdbcClientConfig; + this.client = createCachedClient(hiveConf, + Math.max(MIN_CLIENT_POOL_SIZE, Config.max_external_cache_loader_thread_pool_size), jdbcClientConfig); + } + + public HMSCachedClient getClient() { + return client; + } + + public static HMSCachedClient createCachedClient(HiveConf hiveConf, int thriftClientPoolSize, + JdbcClientConfig jdbcClientConfig) { + if (hiveConf != null) { + return new ThriftHMSCachedClient(hiveConf, thriftClientPoolSize); + } + Preconditions.checkNotNull(jdbcClientConfig, "hiveConf and jdbcClientConfig are both null"); + String dbType = JdbcClient.parseDbType(jdbcClientConfig.getJdbcUrl()); + switch (dbType) { + case JdbcResource.POSTGRESQL: + return new PostgreSQLJdbcHMSCachedClient(jdbcClientConfig); + default: + throw new IllegalArgumentException("Unsupported DB type: " + dbType); + } + } + + @Override + public void createDb(CreateDbStmt stmt) throws DdlException { + String fullDbName = stmt.getFullDbName(); + Map<String, String> properties = stmt.getProperties(); + long id = Env.getCurrentEnv().getNextId(); + try { + HiveCatalogDatabase catalogDatabase = new HiveCatalogDatabase(); + catalogDatabase.setDbName(fullDbName); + catalogDatabase.setProperties(properties); + if (properties.containsKey("location_uri")) { + catalogDatabase.setLocationUri(properties.get("location_uri")); + } + catalogDatabase.setComment(properties.getOrDefault("comment", "")); + client.createDatabase(catalogDatabase); + catalog.registerDatabase(id, fullDbName); + } catch (Exception e) { + throw new RuntimeException(e.getMessage(), e); + } + LOG.info("createDb dbName = " + fullDbName + ", id = " + id); + } + + @Override + public void dropDb(DropDbStmt stmt) throws DdlException { + String dbName = stmt.getDbName(); + try { + client.dropDatabase(dbName); + catalog.unregisterDatabase(dbName); + } catch (Exception e) { + throw new RuntimeException(e.getMessage(), e); + } + } + + @Override + public void createTable(CreateTableStmt stmt) throws UserException { + String dbName = stmt.getDbName(); + String tblName = stmt.getTableName(); + ExternalDatabase<?> db = catalog.getDbNullable(dbName); + if (db == null) { + throw new UserException("Failed to get database: '" + dbName + "' in catalog: " + catalog.getName()); + } + try { + Map<String, String> props = stmt.getExtProperties(); + String inputFormat = props.getOrDefault("input_format", Review Comment: English, please check all other comments ########## fe/fe-core/src/main/java/org/apache/doris/datasource/property/PropertyConverter.java: ########## @@ -264,7 +264,7 @@ private static void setS3FsAccess(Map<String, String> s3Properties, Map<String, s3Properties.put("fs.s3.impl.disable.cache", "true"); s3Properties.putIfAbsent("fs.s3.impl", S3AFileSystem.class.getName()); String defaultProviderList = String.join(",", S3Properties.AWS_CREDENTIALS_PROVIDERS); - String credentialsProviders = s3Properties Review Comment: Is this a bug? -- 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: commits-unsubscr...@doris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org For additional commands, e-mail: commits-h...@doris.apache.org
