xuefuz commented on issue #8007: [FLINK-11474][table] Add ReadableCatalog, ReadableWritableCatalog, and other … URL: https://github.com/apache/flink/pull/8007#issuecomment-476812342 > Thanks for the PR, I think catalog is very important for Table/SQL. Great job @xuefuz! > > I quickly browsed the PR and left some comments. > There are two other suggestions as follows: > > * The overall PR does not have a test, it is recommended that it better to add the corresponding test case. > * This PR just adds some interfaces, and there is no specific implementation and corresponding feature. This will confuse reviewer who have no context background, IMHO, each PR preferably has a specific function, of the function is large, it can be divided into multiple commits in one PR. > > I am not sure whether my suggestion is reasonable or not, maybe @aljoscha and @twalthr can give us more advice. > > What do you think? > > Best, > Jincheng Thanks for the review, Jincheng! You have some good points. First all, for those missing the contact, please check [FLIP-30.](https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=100827366) Secondly, this one does have a test and the reason is that this is mostly new interfaces, for which, tests are not really applicable. Lastly, I don't believe that every merge needs to complete a feature or functionality. Incremental commits are preferable in my opinion because it makes review lighter and easier.
---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
