morningman commented on code in PR #41510:
URL: https://github.com/apache/doris/pull/41510#discussion_r1832136816
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/jdbc/JdbcExternalTable.java:
##########
@@ -102,21 +103,33 @@ public TTableDescriptor toThrift() {
@Override
public Optional<SchemaCacheValue> initSchema() {
- return Optional.of(new SchemaCacheValue(((JdbcExternalCatalog)
catalog).getJdbcClient()
- .getColumnsFromJdbc(dbName, name)));
+ return Optional.of(new SchemaCacheValue(((JdbcExternalCatalog)
catalog).listColumns(dbName, name)));
}
private JdbcTable toJdbcTable() {
List<Column> schema = getFullSchema();
JdbcExternalCatalog jdbcCatalog = (JdbcExternalCatalog) catalog;
- String fullDbName = this.dbName + "." + this.name;
- JdbcTable jdbcTable = new JdbcTable(this.id, fullDbName, schema,
TableType.JDBC_EXTERNAL_TABLE);
- jdbcCatalog.configureJdbcTable(jdbcTable, fullDbName);
+ String fullTableName = this.dbName + "." + this.name;
+ JdbcTable jdbcTable = new JdbcTable(this.id, fullTableName, schema,
TableType.JDBC_EXTERNAL_TABLE);
+ jdbcCatalog.configureJdbcTable(jdbcTable, fullTableName);
// Set remote properties
-
jdbcTable.setRemoteDatabaseName(jdbcCatalog.getJdbcClient().getRemoteDatabaseName(this.dbName));
-
jdbcTable.setRemoteTableName(jdbcCatalog.getJdbcClient().getRemoteTableName(this.dbName,
this.name));
-
jdbcTable.setRemoteColumnNames(jdbcCatalog.getJdbcClient().getRemoteColumnNames(this.dbName,
this.name));
+
jdbcTable.setRemoteDatabaseName(jdbcCatalog.getRemoteDatabaseName(this.dbName));
+
jdbcTable.setRemoteTableName(jdbcCatalog.getRemoteTableName(this.dbName,
this.name));
+ Map<String, String> remoteColumnNames = Maps.newHashMap();
+ for (Column column : schema) {
+ String remoteColumnName =
jdbcCatalog.getRemoteColumnNames(this.dbName, this.name, column.getName());
+ remoteColumnNames.put(column.getName(), remoteColumnName);
+ }
+ if (!remoteColumnNames.isEmpty()) {
Review Comment:
the `remoteColumnNames` is always not empty
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java:
##########
@@ -376,14 +403,18 @@ private void init() {
}
@NotNull
- private List<String> getFilteredDatabaseNames() {
+ private List<Pair<String, String>> getFilteredDatabaseNames() {
Review Comment:
Add comment to explain the logic of this methods
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalTable.java:
##########
@@ -85,6 +87,7 @@ public class ExternalTable implements TableIf, Writable,
GsonPostProcessable {
protected long dbId;
protected boolean objectCreated;
protected ExternalCatalog catalog;
+ protected ExternalDatabase db;
Review Comment:
Should also mark `dbName` as deprecated?
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalTable.java:
##########
@@ -70,6 +70,8 @@ public class ExternalTable implements TableIf, Writable,
GsonPostProcessable {
protected long id;
@SerializedName(value = "name")
protected String name;
+ @SerializedName(value = "remoteName")
+ protected String remoteName;
Review Comment:
`remoteName` maybe null when upgrading from old version. How to handle it?
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java:
##########
@@ -246,15 +257,29 @@ public final synchronized void makeSureInitialized() {
if (!initialized) {
if (useMetaCache.get()) {
if (metaCache == null) {
+ List<Pair<String, String>> remoteToLocalPairs =
getFilteredDatabaseNames();
+
metaCache =
Env.getCurrentEnv().getExtMetaCacheMgr().buildMetaCache(
name,
OptionalLong.of(86400L),
OptionalLong.of(Config.external_cache_expire_time_minutes_after_access * 60L),
Config.max_meta_object_cache_num,
- ignored -> getFilteredDatabaseNames(),
- dbName -> Optional.ofNullable(
- buildDbForInit(dbName,
Util.genIdByName(name, dbName), logType, true)),
- (key, value, cause) -> value.ifPresent(v ->
v.setUnInitialized(invalidCacheInInit)));
+ ignored ->
remoteToLocalPairs.stream().map(Pair::value).collect(Collectors.toList()),
Review Comment:
This is not right.
If you do this, the `dbnames` cache will always be `remoteToLocalPairs`,
even if there are new dbs added.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java:
##########
@@ -149,6 +150,9 @@ public abstract class ExternalCatalog
protected Optional<Boolean> useMetaCache = Optional.empty();
protected MetaCache<ExternalDatabase<? extends ExternalTable>> metaCache;
+ protected IdentifierMapping identifierMapping;
+ private boolean mappingsInitialized = false;
Review Comment:
1. Add comment in code to explain these 2 fields.
2. the `identifierMapping` is initialized in `initLocalObjectsImpl()`, but
filled in `buildDatabaseMapping()`. I think all these should be done in
`buildDatabaseMapping()`
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java:
##########
@@ -352,21 +355,24 @@ private void init() {
InitCatalogLog initCatalogLog = new InitCatalogLog();
initCatalogLog.setCatalogId(id);
initCatalogLog.setType(logType);
- List<String> filteredDatabases = getFilteredDatabaseNames();
- for (String dbName : filteredDatabases) {
+ List<Pair<String, String>> remoteToLocalPairs =
getFilteredDatabaseNames();
+ for (Pair<String, String> pair : remoteToLocalPairs) {
+ String remoteDbName = pair.key();
+ String localDbName = pair.value();
long dbId;
- if (dbNameToId != null && dbNameToId.containsKey(dbName)) {
- dbId = dbNameToId.get(dbName);
- tmpDbNameToId.put(dbName, dbId);
+ if (dbNameToId != null && dbNameToId.containsKey(localDbName)) {
+ dbId = dbNameToId.get(localDbName);
+ tmpDbNameToId.put(localDbName, dbId);
Review Comment:
Need to set `remoteDbName` for this ExternalDatabase?
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/mapping/IdentifierMapping.java:
##########
@@ -17,314 +17,17 @@
package org.apache.doris.datasource.mapping;
-import org.apache.doris.catalog.Column;
-import org.apache.doris.qe.GlobalVariable;
+public interface IdentifierMapping {
-import com.fasterxml.jackson.core.JsonProcessingException;
-import com.fasterxml.jackson.databind.JsonNode;
-import com.fasterxml.jackson.databind.ObjectMapper;
-import com.google.common.collect.Lists;
-import com.google.common.collect.Maps;
-import com.google.common.collect.Sets;
-import org.apache.logging.log4j.LogManager;
-import org.apache.logging.log4j.Logger;
+ String fromRemoteDatabaseName(String remoteDatabaseName);
-import java.util.Collections;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.atomic.AtomicBoolean;
+ String fromRemoteTableName(String remoteDatabaseName, String
remoteTableName);
-public abstract class IdentifierMapping {
- private static final Logger LOG =
LogManager.getLogger(IdentifierMapping.class);
+ String fromRemoteColumnName(String remoteDatabaseName, String
remoteTableName, String remoteColumnNames);
- private final ObjectMapper mapper = new ObjectMapper();
- private final ConcurrentHashMap<String, String> localDBToRemoteDB = new
ConcurrentHashMap<>();
- private final ConcurrentHashMap<String, ConcurrentHashMap<String, String>>
localTableToRemoteTable
- = new ConcurrentHashMap<>();
- private final ConcurrentHashMap<String, ConcurrentHashMap<String,
ConcurrentHashMap<String, String>>>
- localColumnToRemoteColumn = new ConcurrentHashMap<>();
+ String toRemoteDatabaseName(String localDatabaseName);
Review Comment:
Remove unused `toXXX` methods
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/jdbc/JdbcExternalCatalog.java:
##########
@@ -245,20 +241,48 @@ protected void initLocalObjectsImpl() {
jdbcClient = JdbcClient.createJdbcClient(jdbcClientConfig);
}
- protected List<String> listDatabaseNames() {
+ @Override
+ public void gsonPostProcess() throws IOException {
+ super.gsonPostProcess();
+ if (this.identifierMapping == null) {
+ identifierMapping = new
JdbcIdentifierMapping(Boolean.parseBoolean(getLowerCaseMetaNames()),
+ getMetaNamesMapping());
+ }
+ }
+
+ @Override
+ public List<String> listDatabaseNames() {
return jdbcClient.getDatabaseNameList();
}
+ @Override
+ public String fromRemoteDatabaseName(String remoteDatabaseName) {
+ return identifierMapping.fromRemoteDatabaseName(remoteDatabaseName);
+ }
+
@Override
public List<String> listTableNames(SessionContext ctx, String dbName) {
makeSureInitialized();
return jdbcClient.getTablesNameList(dbName);
}
+ @Override
+ public String fromRemoteTableName(String remoteDatabaseName, String
remoteTableName) {
+ return identifierMapping.fromRemoteTableName(remoteDatabaseName,
remoteTableName);
+ }
+
@Override
public boolean tableExist(SessionContext ctx, String dbName, String
tblName) {
makeSureInitialized();
- return jdbcClient.isTableExist(dbName, tblName);
+ ExternalTable tbl =
Objects.requireNonNull(this.getDbNullable(dbName)).getTableNullable(tblName);
Review Comment:
How to make sure `tbl` is always not null?
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalDatabase.java:
##########
@@ -76,6 +79,8 @@ public abstract class ExternalDatabase<T extends
ExternalTable>
protected long id;
@SerializedName(value = "name")
protected String name;
+ @SerializedName(value = "remoteName")
+ protected String remoteName;
Review Comment:
`remoteName` maybe null when upgrading from old version. How to handle it?
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java:
##########
@@ -627,8 +671,15 @@ private void removeAccessController() {
}
public void replayInitCatalog(InitCatalogLog log) {
+ if (log.getRemoteDbNames() == null ||
log.getRemoteDbNames().isEmpty()) {
Review Comment:
Strings.isNullOrEmpty(log.getRemoteDbNames())
--
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
