[GitHub] [doris] morningman commented on pull request #10320: [feature-wip](multi-catalog) refactor catalog interface

[GitBox <gi...@apache.org>]

morningman commented on PR #10320:
URL: https://github.com/apache/doris/pull/10320#issuecomment-1165140701

   > Why invoke getCurrentInternalCatalog outside the catalog such as in InsertStmt? getCurrentInternalCatalog should be an internal function in catalo. In insertStmt we should get a DatabaseIf or TableIf from catalog and add a datasource checker for check whether is it a supported datasource table.
   > 
   > disadvantage:
   > 
   > 1. Catalog.getCurrentInternalCatalog(). getDbOrDdlException will throw an exception even though the db is exist in some external datasource. This will confuses users. Such as in CreateRoutineLoadStmt.java,  When create routineLoad job for an external datasource table, It should throw do not support external datasource table  instead of table not exist.
   > 2. catalog api caller such analyzer just want to resolve the db or table identifier instead of perceive the catalog internal interface such getCurrentInternalCatalog.
   > 
   > Suggestion:
   > 
   > 1. catalog api caller such analyzer use Catalog. getDbOrDdlException apis to get a DatabaseIf or TableIf object.
   > 2. In catalog internal, it can use current datasource or catalog indicator from api parameter to get a Database or table object.
   > 3. if we support insert external datasource table in the future,  just remove the datasource checker in InsertStmt.
   
   1. First, I deleted the `getDbXXX` methods from `Catalog.java`, because in essence these `getDb` methods should be the methods of DataSourceMgr. The `Catalog` class will be renamed later, such as `Env` or `DorisRuntime`. It is only responsible for managing all Mgrs, and each Mgr is responsible for its own affairs, such as `DataSourceMgr` is responsible for managing `DataSource` (subsequently renamed `Catalog`) and `getDb`.
   
   2. All are currently modified to `getCurrentInternalCatalog`. Mainly because the function of multi catalog is still under development, and there is currently no context of external catalog, so I first use `InternalCatalog` to ensure that the existing functions are completely unaffected.
   All subsequent `Catalog.getCurrentInternalCatalog().getDbxxx` will be uniformly changed to, for example, `Env.getCatalog().getDbxxx()`.
   Currently this is only an intermediate state, the purpose is to remove some methods in the Catalog.
   
   3. Whether a function supports the external catalog will be determined later in the analyze phase of Stmt. Then, when the switch catalog is developed, you can obtain the context of the catalog through the context for unified judgment.


-- 
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