You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2022/06/30 14:34:42 UTC
[GitHub] [doris] morningman commented on a diff in pull request #10521: [feature-wip](multi-catalog) end to end to support multi-catalog
morningman commented on code in PR #10521:
URL: https://github.com/apache/doris/pull/10521#discussion_r910840777
##########
fe/fe-core/src/main/java/org/apache/doris/qe/ShowExecutor.java:
##########
@@ -647,7 +650,13 @@ private void handleShowPartitionId() throws AnalysisException {
private void handleShowDb() throws AnalysisException {
ShowDbStmt showDbStmt = (ShowDbStmt) stmt;
List<List<String>> rows = Lists.newArrayList();
- List<String> dbNames = ctx.getCatalog().getInternalDataSource().getClusterDbNames(ctx.getClusterName());
+ // external catalog have on cluster semantics
+ List<String> dbNames;
+ if (ctx.getDefaultCatalog().equals(InternalDataSource.INTERNAL_DS_NAME)) {
Review Comment:
We can just use `ctx.getCurrentDataSource().getDbNames();` and ignore the `cluster` info.
Because `cluster` feature is deprecated.
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/FromClause.java:
##########
@@ -77,17 +72,10 @@ private void checkFromHiveTable(Analyzer analyzer) throws AnalysisException {
}
TableName tableName = tblRef.getName();
Review Comment:
This method `checkFromHiveTable()` can be removed
##########
fe/fe-core/src/main/java/org/apache/doris/policy/Policy.java:
##########
@@ -84,11 +84,10 @@ public static Policy fromCreateStmt(CreatePolicyStmt stmt) throws AnalysisExcept
return storagePolicy;
case ROW:
default:
- String curDb = stmt.getTableName().getDb();
- if (curDb == null) {
- curDb = ConnectContext.get().getDatabase();
- }
- DatabaseIf db = Catalog.getCurrentCatalog().getCurrentDataSource().getDbOrAnalysisException(curDb);
+ // stmt must be analyzed.
+ DatabaseIf db = Catalog.getCurrentCatalog().getDataSourceMgr()
+ .getCatalog(stmt.getTableName().getCtl())
Review Comment:
The `getCatalog()` method may return null here if catalog does not exist.
We can use `Optional` to do this.
##########
fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java:
##########
@@ -221,7 +221,9 @@ public TGetTablesResult getTableNames(TGetTablesParams params) throws TException
} else {
currentUser = UserIdentity.createAnalyzedUserIdentWithIp(params.user, params.user_ip);
}
- DatabaseIf<TableIf> db = Catalog.getCurrentCatalog().getCurrentDataSource().getDbNullable(params.db);
+ String catalog = Strings.isNullOrEmpty(params.catalog) ? InternalDataSource.INTERNAL_DS_NAME : params.catalog;
+ DatabaseIf<TableIf> db = Catalog.getCurrentCatalog().getDataSourceMgr()
Review Comment:
`getCatalog` may return null
##########
fe/fe-core/src/main/java/org/apache/doris/qe/ShowExecutor.java:
##########
@@ -1005,8 +1013,9 @@ private void handleHelp() {
private void handleShowLoad() throws AnalysisException {
ShowLoadStmt showStmt = (ShowLoadStmt) stmt;
- Catalog catalog = Catalog.getCurrentCatalog();
- DatabaseIf db = catalog.getCurrentDataSource().getDbOrAnalysisException(showStmt.getDbName());
+ Util.prohibitExternalCatalog(ctx.getDefaultCatalog(), stmt.getClass().getSimpleName());
+ Catalog catalog = ctx.getCatalog();
Review Comment:
Unused?
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/FunctionCallExpr.java:
##########
@@ -936,7 +936,8 @@ public void analyzeImpl(Analyzer analyzer) throws AnalysisException {
.checkDbPriv(ConnectContext.get(), dbName, PrivPredicate.SELECT)) {
ErrorReport.reportAnalysisException(ErrorCode.ERR_SPECIFIC_ACCESS_DENIED_ERROR, "SELECT");
}
- DatabaseIf db = Catalog.getCurrentCatalog().getCurrentDataSource().getDbNullable(dbName);
+ // TODO(gaoxin): ExternalDatabase not implement udf yet.
+ DatabaseIf db = Catalog.getCurrentCatalog().getInternalDataSource().getDbNullable(dbName);
Review Comment:
Maybe we should modify `fnName` too, same as `TableName`
##########
fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/MetaInfoAction.java:
##########
@@ -90,8 +90,9 @@ public Object getAllDatabases(
return ResponseEntityBuilder.badRequest("Only support 'default_cluster' now");
}
+ // TODO(gaoxin): Implement multi-catalog for http restful api.
// 1. get all database with privilege
- List<String> dbNames = Catalog.getCurrentCatalog().getCurrentDataSource().getDbNames();
Review Comment:
we can use `NS_KEY`.
but `NS_KEY`'s default value is `default_cluster`, we need to do some compatible things.
For example, if `NS_KEY` is `default_cluster`, change it to `internal` catalog implicitly, otherwise, use it as catalog name.
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/DescribeStmt.java:
##########
@@ -75,6 +79,13 @@ public class DescribeStmt extends ShowStmt {
.addColumn(new Column("Table", ScalarType.createVarchar(30)))
.build();
+ private static final ShowResultSetMetaData HMS_EXTERNAL_TABLE_META_DATA =
Review Comment:
I think we should use columns defined in `IndexSchemaProcNode`.
To be compatible with mysql style
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org