xuefuz commented on issue #7011: [FLINK-10768][Table & SQL] Move external catalog related code from TableEnvironment to CatalogManager URL: https://github.com/apache/flink/pull/7011#issuecomment-436088040 Thanks for the PR. I quickly went over the changes and have some high-level comments: 1. It would be better if there is clearer distinction of responsibility for the CatalogManager class. If it manages catalogs and temp tables/functions, then it should just expose those as needed. If we need something extra, then we need some justification. 2. Since we are encapsulating catalog, Calcite schema, and temp tables/functions in CatalogManager, we should hide them as much as possible. Exposing something like registerExternalCatalog() might trigger some questions. 3. Intuitively TableEnvironment depends on CatalogManager but not vise versa. Thus, I'm not sure if and why we need a circular reference between them.
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
