Hi,
Thanks for the nice proposal, Ran.
Regarding this api usage, I have some discussion with @twalthr before
as here <https://github.com/apache/flink/pull/15137#issuecomment-1356124138>
Personally, I think leaking the Catalog to the user side is not suitable,
since there are some read/write operations in the Catalog, the
TableEnvironment#getCatalog will expose all of them to the user side. So I
learned to add a new api in TableEnvironment to reduce reliance on the
current TableEnvironment#getCatalog.
Thanks,
Aitozi
Ran Tao <chucheng...@gmail.com> 于2023年2月23日周四 23:44写道：
> Hi, JingSong, Jing.
>
> thank for sharing your opinions.
>
> What you say makes sense, both approaches have pros and cons.
>
> If it is a modification of `TableEnvrionment`, such as
> listDatabases(catalog). It is actually consistent with the other overloaded
> methods before,
> and defining this method means that TableEnvrionment does provide this
> capability (rather than relying on the functionality of another class).
> The disadvantage is that api changes may be required, and may continue to
> be modified in the future.
> But from the TableEnvrionment itself, it really doesn't pay attention to
> how the underlying layer is implemented.
> (Although it is actually taken from the catalogManager at present, this is
> another question)
>
> Judging from the current dependencies, flink-table-api-java strongly relies
> on flink-table-common to use various common classes and interfaces,
> especially the catalog interface.
> So we can extract various metadata information in the catalog through
> `tEnv.getCatalog`.
> The advantage is that it will not cause api modification, but this method
> of use breaks LoD.
> In fact, the current flink-table-api-java design is completely bound to the
> catalog interface.
>
> If the mandatory modification of PublicApi is constrained (may be modified
> again and later), I tend to use `tEnv.getCatalog` directly, otherwise
> It would actually be more standard to add overloaded methods to
> `TableEnvrionment`.
>
> Another question, can the later capabilities of TableEnvrionment be
> implemented through SupportXXX?
> In order to solve the problem that the method needs to be added in the
> future. This kind of usage occurs frequently in flink.
>
> Looking forward to your other considerations,
> and also try to wait to see if there are other relevant API designers or
> committers to provide comments.
>
>
> Best Regards,
> Ran Tao
>
> Jing Ge <j...@ververica.com.invalid> 于2023年2月23日周四 18:58写道：
>
> > Hi Jingson,
> >
> > Thanks for sharing your thoughts. Please see my reply below.
> >
> > On Thu, Feb 23, 2023 at 10:16 AM Jingsong Li <jingsongl...@gmail.com>
> > wrote:
> >
> > > Hi Jing Ge,
> > >
> > > First, flink-table-common contains all common classes of Flink Table,
> > > I think it is hard to bypass its dependence.
> > >
> >
> > If any time when we use flink-table-api-java, we have to cross through
> > flink-table-api-java and use flink-table-common, we should reconsider the
> > design of these two modules and how interfaces/classes are classified
> into
> > those modules.
> >
> >
> > >
> > > Secondly, almost all methods in Catalog looks useful to me, so if we
> > > are following LoD, we should add all methods again to
> > > TableEnvironment. I think it is redundant.
> > >
> >
> > That is the enlarged issue I mentioned previously. A simple solution is
> to
> > move Catalog to the top level API. The fact is that a catalog package
> > already exists in flink-table-api-java but the Catalog interface is in
> > flink-table-common. I don't know the historical context of this design.
> > Maybe you could share some insight with us? Thanks in advance. Beyond
> that,
> > there should be other AOP options but need more time to figure it out.
> >
> >
> > >
> > > And, this API chain does not look deep.
> > > - "tEnv.getCatalog(tEnv.getCurrentCatalog()).get().listDatabases()"
> > > looks a little complicated. The complex part is ahead.
> > > - If we have a method to get Catalog directly, can be simplify to
> > > "tEnv.catalog().listDatabase()", this is simple.
> > >
> >
> > Commonly, it will need more effort to always follow LoD, but for the top
> > level facade API like TableEnvironment, both the API developer, API
> > consumer and the project itself from a long-term perspective will benefit
> > from sticking to LoD. Since we already have the getCatalog(String
> catalog)
> > method in TableEnvironment, it also makes sense to follow your
> suggestion,
> > if we only want to limit/avoid public API changes. But please be aware
> that
> > we will have all known long-term drawbacks because of LoD
> > violation, especially the cross modules violation. I just checked all
> > usages of getCatalog(String catalog) in the master branch. Currently
> there
> > are very limited calls. It is better to pay attention to it before it
> goes
> > worse. Just my 2 cents. :)
> >
> >
> > >
> > > Best,
> > > Jingsong
> > >
> > > On Thu, Feb 23, 2023 at 4:47 PM Jing Ge <j...@ververica.com.invalid>
> > > wrote:
> > > >
> > > > Hi Jingson,
> > > >
> > > > Thanks for the knowledge sharing. IMHO, it looks more like a design
> > > > guideline question than just avoiding public API change. Please
> correct
> > > me
> > > > if I'm wrong.
> > > >
> > > > Catalog is in flink-table-common module and TableEnvironment is in
> > > > flink-table-api-java. Depending on how and where those features
> > proposed
> > > in
> > > > this FLIP will be used, we'd better reduce the dependency chain and
> > > follow
> > > > the Law of Demeter(LoD, clean code) [1]. Adding a new method in
> > > > TableEnvironment is therefore better than calling an API chain. It is
> > > also
> > > > more user friendly for the caller, because there is no need to
> > understand
> > > > the internal structure of the called API. The downside of doing this
> is
> > > > that we might have another issue with the current TableEnvironment
> > > design -
> > > > the TableEnvironment interface got enlarged with more wrapper
> methods.
> > > This
> > > > is a different issue that could be solved with improved abstraction
> > > design
> > > > in the future. After considering pros and cons, if we want to add
> those
> > > > features now, I would prefer following LoD than API chain calls.
> WDYT?
> > > >
> > > > Best regards,
> > > > Jing
> > > >
> > > > [1]
> > > >
> > >
> >
> https://hackernoon.com/object-oriented-tricks-2-law-of-demeter-4ecc9becad85
> > > >
> > > > On Thu, Feb 23, 2023 at 6:26 AM Ran Tao <chucheng...@gmail.com>
> wrote:
> > > >
> > > > > Hi Jingsong. thanks. i got it.
> > > > > In this way, there is no need to introduce new API changes.
> > > > >
> > > > > Best Regards,
> > > > > Ran Tao
> > > > >
> > > > >
> > > > > Jingsong Li <jingsongl...@gmail.com> 于2023年2月23日周四 12:26写道：
> > > > >
> > > > > > Hi Ran,
> > > > > >
> > > > > > I mean we can just use
> > > > > >
> > TableEnvironment.getCatalog(getCurrentCatalog).get().listDatabases().
> > > > > >
> > > > > > We don't need to provide new apis just for utils.
> > > > > >
> > > > > > Best,
> > > > > > Jingsong
> > > > > >
> > > > > > On Thu, Feb 23, 2023 at 12:11 PM Ran Tao <chucheng...@gmail.com>
> > > wrote:
> > > > > > >
> > > > > > > Hi Jingsong, thanks.
> > > > > > >
> > > > > > > The implementation of these statements in TableEnvironmentImpl
> is
> > > > > called
> > > > > > > through the catalog api.
> > > > > > >
> > > > > > > but it does support some new override methods on the catalog
> api
> > > side,
> > > > > > and
> > > > > > > I will update it later. Thank you.
> > > > > > >
> > > > > > > e.g.
> > > > > > > TableEnvironmentImpl
> > > > > > > @Override
> > > > > > > public String[] listDatabases() {
> > > > > > > return catalogManager
> > > > > > > .getCatalog(catalogManager.getCurrentCatalog())
> > > > > > > .get()
> > > > > > > .listDatabases()
> > > > > > > .toArray(new String[0]);
> > > > > > > }
> > > > > > >
> > > > > > > Best Regards,
> > > > > > > Ran Tao
> > > > > > >
> > > > > > >
> > > > > > > Jingsong Li <jingsongl...@gmail.com> 于2023年2月23日周四 11:47写道：
> > > > > > >
> > > > > > > > Thanks for the proposal.
> > > > > > > >
> > > > > > > > +1 for the proposal.
> > > > > > > >
> > > > > > > > I am confused about "Proposed TableEnvironment SQL API
> > Changes",
> > > can
> > > > > > > > we just use catalog api for this requirement?
> > > > > > > >
> > > > > > > > Best,
> > > > > > > > Jingsong
> > > > > > > >
> > > > > > > > On Thu, Feb 23, 2023 at 10:48 AM Jacky Lau <
> > liuyong...@gmail.com
> > > >
> > > > > > wrote:
> > > > > > > > >
> > > > > > > > > Hi Ran:
> > > > > > > > > Thanks for driving the FLIP. the google doc looks really
> > good.
> > > it
> > > > > is
> > > > > > > > > important to improve user interactive experience. +1 to
> > support
> > > > > this
> > > > > > > > > feature.
> > > > > > > > >
> > > > > > > > > Jing Ge <j...@ververica.com.invalid> 于2023年2月23日周四
> 00:51写道：
> > > > > > > > >
> > > > > > > > > > Hi Ran,
> > > > > > > > > >
> > > > > > > > > > Thanks for driving the FLIP. It looks overall good.
> Would
> > > you
> > > > > > like to
> > > > > > > > add
> > > > > > > > > > a description of useLike and notLike? I guess useLike
> true
> > > is for
> > > > > > > > "LIKE"
> > > > > > > > > > and notLike true is for "NOT LIKE" but I am not sure if I
> > > > > > understood it
> > > > > > > > > > correctly. Furthermore, does it make sense to support
> > "ILIKE"
> > > > > too?
> > > > > > > > > >
> > > > > > > > > > Best regards,
> > > > > > > > > > Jing
> > > > > > > > > >
> > > > > > > > > > On Wed, Feb 22, 2023 at 1:17 PM Ran Tao <
> > > chucheng...@gmail.com>
> > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > > Currently flink sql auxiliary statements has supported
> > some
> > > > > good
> > > > > > > > features
> > > > > > > > > > > such as catalog/databases/table support.
> > > > > > > > > > >
> > > > > > > > > > > But these features are not very complete compared with
> > > other
> > > > > > popular
> > > > > > > > > > > engines such as spark, presto, hive and commercial
> > engines
> > > such
> > > > > > as
> > > > > > > > > > > snowflake.
> > > > > > > > > > >
> > > > > > > > > > > For example, many engines support show operation with
> > > filtering
> > > > > > > > except
> > > > > > > > > > > flink, and support describe other object(flink only
> > support
> > > > > > describe
> > > > > > > > > > > table).
> > > > > > > > > > >
> > > > > > > > > > > I wonder can we add these useful features for flink?
> > > > > > > > > > > You can find details in this doc.[1] or FLIP.[2]
> > > > > > > > > > >
> > > > > > > > > > > Also, please let me know if there is a mistake. Looking
> > > forward
> > > > > > to
> > > > > > > > your
> > > > > > > > > > > reply.
> > > > > > > > > > >
> > > > > > > > > > > [1]
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > >
> > > > > >
> > > > >
> > >
> >
> https://docs.google.com/document/d/1hAiOfPx14VTBTOlpyxG7FA2mB1k5M31VnKYad2XpJ1I/
> > > > > > > > > > > [2]
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > >
> > > > > >
> > > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/FLINK/FLIP-297%3A+Improve+Auxiliary+Sql+Statements
> > > > > > > > > > >
> > > > > > > > > > > Best Regards,
> > > > > > > > > > > Ran Tao
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > >
> > > > > >
> > > > >
> > >
> >
>
