HI Kurt, +1 to remove these methods.
But one concern is that some of the current TableSource/TableSink may not be ready, such as the JDBCUpsertTableSink, which accepts a JDBCDialect, but through the TableFactory, there is no way to pass in the JDBCDialect at present. But I also believe we have enough time on 1.11 to prepare them. Then unified to TableFactory. I think there may be complaints from users because they used to only implement a TableSource or TableSink, but now they have to implement TableFactory. But I think it's also good for conceptual clarity to force them to implement TableFactory. Another idea is can we provide some utils to help user implement TableFactory? The current method may be a little bit complex, and it needs to be added to the "META_INF/services/org.apache.flink.table.factories.TableFactory" file. Best, Jingsong Lee On Wed, Feb 5, 2020 at 3:58 PM Dawid Wysakowicz <dwysakow...@apache.org> wrote: > Hi Kurt, > > I fully agree with the proposal. Yes it was an omission that we did not > deprecate the TableEnvironment#fromTableSource in the previous version. > > I would vote to remove all those methods altogether. > > Best, > > Dawid > > On 05/02/2020 07:36, Kurt Young wrote: > > Hi all, > > > > I'd like to bring up a discussion about removing registration of > > TableSource and > > TableSink in TableEnvironment as well as in ConnectTableDescriptor. The > > affected > > method would be: > > > > TableEnvironment::registerTableSource > > TableEnvironment::fromTableSource > > TableEnvironment::registerTableSink > > ConnectTableDescriptor::registerTableSource > > ConnectTableDescriptor::registerTableSink > > ConnectTableDescriptor::registerTableSourceAndSink > > > > (Most of them are already deprecated, except for > > TableEnvironment::fromTableSource, > > which was intended to deprecate but missed by accident). > > > > FLIP-64 [1] already explained why we want to deprecate TableSource & > > TableSink from > > user's interface. In a short word, these interfaces should only read & > > write the physical > > representation of the table, and they are not fitting well after we > already > > introduced some > > logical table fields such as computed column, watermarks. > > > > Another reason is the exposure of registerTableSource in Table Env just > > make the whole > > SQL protocol opposite. TableSource should be used as a reader of table, > it > > should rely on > > other metadata information held by framework, which eventually comes from > > DDL or > > ConnectDescriptor. But if we register a TableSource to Table Env, we have > > no choice but > > have to rely on TableSource::getTableSchema. It will make the design > > obscure, sometimes > > TableSource should trust the information comes from framework, but > > sometimes it should > > also generate its own schema information. > > > > Furthermore, if the authority about schema information is not clear, it > > will make things much > > more complicated if we want to improve the table api usability such as > > introducing automatic > > schema inference in the near future. > > > > Since this is an API break change, I've also included user mailing list > to > > gather more feedbacks. > > > > Best, > > Kurt > > > > [1] > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-64%3A+Support+for+Temporary+Objects+in+Table+module > > > > -- Best, Jingsong Lee
