Hi everyone, I did a summary on the Jira issue page [1] since the discussion has achieved a consensus. If there is anything missed or not corrected, please let me know.
[1] https://issues.apache.org/jira/browse/FLINK-21045# Best, Jane On Wed, Feb 3, 2021 at 1:33 PM Jark Wu <imj...@gmail.com> wrote: > Hi Jane, > > Yes. I think we should fail fast. > > Best, > Jark > > On Wed, 3 Feb 2021 at 12:06, Jane Chan <qingyue....@gmail.com> wrote: > > > Hi everyone, > > > > Thanks for the discussion to make this improvement plan clearer. > > > > Hi, @Jark, @Rui, and @Timo, I'm collecting the final discussion summaries > > now and want to confirm one thing that for the statement `USE MODULES x > [, > > y, z, ...]`, if the module name list contains an unexsited module, shall > we > > #1 fail the execution for all of them or #2 enabled the rest modules and > > return a warning to users? My personal preference goes to #1 for > > simplicity. What do you think? > > > > Best, > > Jane > > > > On Tue, Feb 2, 2021 at 3:53 PM Timo Walther <twal...@apache.org> wrote: > > > > > +1 > > > > > > @Jane Can you summarize our discussion in the JIRA issue? > > > > > > Thanks, > > > Timo > > > > > > > > > On 02.02.21 03:50, Jark Wu wrote: > > > > Hi Timo, > > > > > > > >> Another question is whether a LOAD operation also adds the module to > > the > > > > enabled list by default? > > > > > > > > I would like to add the module to the enabled list by default, the > main > > > > reasons are: > > > > 1) Reordering is an advanced requirement, adding modules needs > > additional > > > > USE statements with "core" module > > > > sounds too burdensome. Most users should be satisfied with only > LOAD > > > > statements. > > > > 2) We should keep compatible for TableEnvironment#loadModule(). > > > > 3) We are using the LOAD statement instead of CREATE, so I think it's > > > fine > > > > that it does some implicit things. > > > > > > > > Best, > > > > Jark > > > > > > > > On Tue, 2 Feb 2021 at 00:48, Timo Walther <twal...@apache.org> > wrote: > > > > > > > >> Not the module itself but the ModuleManager should handle this case, > > > yes. > > > >> > > > >> Regards, > > > >> Timo > > > >> > > > >> > > > >> On 01.02.21 17:35, Jane Chan wrote: > > > >>> +1 to Jark's proposal > > > >>> > > > >>> To make it clearer, will `module#getFunctionDefinition()` > return > > > empty > > > >>> suppose the module is loaded but not enabled? > > > >>> > > > >>> Best, > > > >>> Jane > > > >>> > > > >>> On Mon, Feb 1, 2021 at 10:02 PM Timo Walther <twal...@apache.org> > > > wrote: > > > >>> > > > >>>> +1 to Jark's proposal > > > >>>> > > > >>>> I like the difference between just loading and actually enabling > > these > > > >>>> modules. > > > >>>> > > > >>>> @Rui: I would use the same behavior as catalogs here. You cannot > > > `USE` a > > > >>>> catalog without creating it before. > > > >>>> > > > >>>> Another question is whether a LOAD operation also adds the module > to > > > the > > > >>>> enabled list by default? > > > >>>> > > > >>>> Regards, > > > >>>> Timo > > > >>>> > > > >>>> On 01.02.21 13:52, Rui Li wrote: > > > >>>>> If `USE MODULES` implies unloading modules that are not listed, > > does > > > it > > > >>>>> also imply loading modules that are not previously loaded, > > especially > > > >>>> since > > > >>>>> we're mapping modules by name now? > > > >>>>> > > > >>>>> On Mon, Feb 1, 2021 at 8:20 PM Jark Wu <imj...@gmail.com> wrote: > > > >>>>> > > > >>>>>> I agree with Timo that the USE implies the specified modules are > > in > > > >> use > > > >>>> in > > > >>>>>> the specified order and others are not used. > > > >>>>>> This would be easier to know what's the result list and order > > after > > > >> the > > > >>>> USE > > > >>>>>> statement. > > > >>>>>> That means: if current modules in order are x, y, z. And `USE > > > MODULES > > > >>>> z, y` > > > >>>>>> means current modules in order are z, y. > > > >>>>>> > > > >>>>>> But I would like to not unload the unmentioned modules in the > USE > > > >>>>>> statement. Because it seems strange that USE > > > >>>>>> will implicitly remove modules. In the above example, the user > may > > > >> type > > > >>>> the > > > >>>>>> wrong modules list using USE by mistake > > > >>>>>> and would like to declare the list again, the user has to > > create > > > >> the > > > >>>>>> module again with some properties he may don't know. Therefore, > I > > > >>>> propose > > > >>>>>> the USE statement just specifies the current module lists and > > > doesn't > > > >>>>>> unload modules. > > > >>>>>> Besides that, we may need a new syntax to list all the modules > > > >> including > > > >>>>>> not used but loaded. > > > >>>>>> We can introduce SHOW FULL MODULES for this purpose with an > > > additional > > > >>>>>> `used` column. > > > >>>>>> > > > >>>>>> For example: > > > >>>>>> > > > >>>>>> Flink SQL> list modules: > > > >>>>>> ----------- > > > >>>>>> | modules | > > > >>>>>> ----------- > > > >>>>>> | x | > > > >>>>>> | y | > > > >>>>>> | z | > > > >>>>>> ----------- > > > >>>>>> Flink SQL> USE MODULES z, y; > > > >>>>>> Flink SQL> show modules: > > > >>>>>> ----------- > > > >>>>>> | modules | > > > >>>>>> ----------- > > > >>>>>> | z | > > > >>>>>> | y | > > > >>>>>> ----------- > > > >>>>>> Flink SQL> show FULL modules; > > > >>>>>> ------------------- > > > >>>>>> | modules | used | > > > >>>>>> ------------------- > > > >>>>>> | z | true | > > > >>>>>> | y | true | > > > >>>>>> | x | false | > > > >>>>>> ------------------- > > > >>>>>> Flink SQL> USE MODULES z, y, x; > > > >>>>>> Flink SQL> show modules; > > > >>>>>> ----------- > > > >>>>>> | modules | > > > >>>>>> ----------- > > > >>>>>> | z | > > > >>>>>> | y | > > > >>>>>> | x | > > > >>>>>> ----------- > > > >>>>>> > > > >>>>>> What do you think? > > > >>>>>> > > > >>>>>> Best, > > > >>>>>> Jark > > > >>>>>> > > > >>>>>> On Mon, 1 Feb 2021 at 19:02, Jane Chan <qingyue....@gmail.com> > > > wrote: > > > >>>>>> > > > >>>>>>> Hi Timo, thanks for the discussion. > > > >>>>>>> > > > >>>>>>> It seems to reach an agreement regarding #3 that <1> Module > name > > > >> should > > > >>>>>>> better be a simple identifier rather than a string literal. <2> > > > >>>> Property > > > >>>>>>> `type` is redundant and should be removed, and mapping will > rely > > on > > > >> the > > > >>>>>>> module name because loading a module multiple times just using > a > > > >>>>>> different > > > >>>>>>> module name doesn't make much sense. <3> We should migrate to > the > > > >> newer > > > >>>>>> API > > > >>>>>>> rather than the deprecated `TableFactory` class. > > > >>>>>>> > > > >>>>>>> Regarding #1, I think the point lies in whether changing the > > > >> resolution > > > >>>>>>> order implies an `unload` operation explicitly (i.e., users > could > > > >> sense > > > >>>>>>> it). What do others think? > > > >>>>>>> > > > >>>>>>> Best, > > > >>>>>>> Jane > > > >>>>>>> > > > >>>>>>> On Mon, Feb 1, 2021 at 6:41 PM Timo Walther < > twal...@apache.org> > > > >>>> wrote: > > > >>>>>>> > > > >>>>>>>> IMHO I would rather unload the not mentioned modules. The > > > statement > > > >>>>>>>> expresses `USE` that implicilty implies that the other modules > > are > > > >>>> "not > > > >>>>>>>> used". What do others think? > > > >>>>>>>> > > > >>>>>>>> Regards, > > > >>>>>>>> Timo > > > >>>>>>>> > > > >>>>>>>> > > > >>>>>>>> On 01.02.21 11:28, Jane Chan wrote: > > > >>>>>>>>> Hi Jark and Rui, > > > >>>>>>>>> > > > >>>>>>>>> Thanks for the discussions. > > > >>>>>>>>> > > > >>>>>>>>> Regarding #1, I'm fine with `USE MODULES` syntax, and > > > >>>>>>>>> > > > >>>>>>>>>> It can be interpreted as "setting the current order of > > modules", > > > >>>>>> which > > > >>>>>>>> is > > > >>>>>>>>>> similar to "setting the current catalog" for `USE CATALOG`. > > > >>>>>>>>>> > > > >>>>>>>>> I would like to confirm that the unmentioned modules remain > in > > > the > > > >>>>>> same > > > >>>>>>>>> relative order? E.g., if there are three loaded modules `X`, > > `Y`, > > > >>>>>> `Z`, > > > >>>>>>>> then > > > >>>>>>>>> `USE MODULES Y, Z` means shifting the order to `Y`, `Z`, `X`. > > > >>>>>>>>> > > > >>>>>>>>> Regarding #3, I'm fine with mapping modules purely by name, > > and I > > > >>>>>> think > > > >>>>>>>>> Jark raised a good point on making the module name a simple > > > >>>>>> identifier > > > >>>>>>>>> instead of a string literal. For backward compatibility, > since > > we > > > >>>>>>> haven't > > > >>>>>>>>> supported this syntax yet, the affected users are those who > > > defined > > > >>>>>>>> modules > > > >>>>>>>>> in the YAML configuration file. Maybe we can eliminate the > > 'type' > > > >>>>>> from > > > >>>>>>>> the > > > >>>>>>>>> 'requiredContext' to make it optional. Thus the proposed > > mapping > > > >>>>>>>> mechanism > > > >>>>>>>>> could use the module name to lookup the suitable factory, > and > > in > > > >> the > > > >>>>>>>>> meanwhile updating documentation to encourage users to > simplify > > > >> their > > > >>>>>>>> YAML > > > >>>>>>>>> configuration. And in the long run, we can deprecate the > > 'type'. > > > >>>>>>>>> > > > >>>>>>>>> Best, > > > >>>>>>>>> Jane > > > >>>>>>>>> > > > >>>>>>>>> On Mon, Feb 1, 2021 at 4:19 PM Rui Li <lirui.fu...@gmail.com > > > > > >> wrote: > > > >>>>>>>>> > > > >>>>>>>>>> Thanks Jane for starting the discussion. > > > >>>>>>>>>> > > > >>>>>>>>>> Regarding #1, I also prefer `USE MODULES` syntax. It can be > > > >>>>>>> interpreted > > > >>>>>>>> as > > > >>>>>>>>>> "setting the current order of modules", which is similar to > > > >> "setting > > > >>>>>>> the > > > >>>>>>>>>> current catalog" for `USE CATALOG`. > > > >>>>>>>>>> > > > >>>>>>>>>> Regarding #3, I'm fine to map modules purely by name > because I > > > >> think > > > >>>>>>> it > > > >>>>>>>>>> satisfies all the use cases we have at hand. But I guess we > > need > > > >> to > > > >>>>>>> make > > > >>>>>>>>>> sure we're backward compatible, i.e. users don't need to > > change > > > >>>>>> their > > > >>>>>>>> yaml > > > >>>>>>>>>> files to configure the modules. > > > >>>>>>>>>> > > > >>>>>>>>>> On Mon, Feb 1, 2021 at 3:10 PM Jark Wu <imj...@gmail.com> > > > wrote: > > > >>>>>>>>>> > > > >>>>>>>>>>> Thanks Jane for the summary and starting the discussion in > > the > > > >>>>>>> mailing > > > >>>>>>>>>>> list. > > > >>>>>>>>>>> > > > >>>>>>>>>>> Here are my thoughts: > > > >>>>>>>>>>> > > > >>>>>>>>>>> 1) syntax to reorder modules > > > >>>>>>>>>>> I agree with Rui Li it would be quite useful if we can have > > > some > > > >>>>>>> syntax > > > >>>>>>>>>> to > > > >>>>>>>>>>> reorder modules. > > > >>>>>>>>>>> I slightly prefer `USE MODULES x, y, z` than `RELOAD > MODULES > > x, > > > >> y, > > > >>>>>>> z`, > > > >>>>>>>>>>> because USE has a more sense of effective and specifying > > > >> ordering, > > > >>>>>>> than > > > >>>>>>>>>>> RELOAD. > > > >>>>>>>>>>> From my feeling, RELOAD just means we unregister and > > > register > > > >>>>>> x,y,z > > > >>>>>>>>>> modules > > > >>>>>>>>>>> again, > > > >>>>>>>>>>> it sounds like other registered modules are still in use > and > > in > > > >> the > > > >>>>>>>>>> order. > > > >>>>>>>>>>> > > > >>>>>>>>>>> 3) mapping modules purely by name > > > >>>>>>>>>>> This can definitely improve the usability of loading > modules, > > > >>>>>> because > > > >>>>>>>>>>> the 'type=' property > > > >>>>>>>>>>> looks really redundant. We can think of this as a syntax > > sugar > > > >> that > > > >>>>>>> the > > > >>>>>>>>>>> default type value is the module name. > > > >>>>>>>>>>> And we can support to specify 'type=' property in the > future > > to > > > >>>>>> allow > > > >>>>>>>>>>> multiple modules for one module type. > > > >>>>>>>>>>> > > > >>>>>>>>>>> Besides, I would like to mention one more change, that the > > > module > > > >>>>>>> name > > > >>>>>>>>>>> proposed in FLIP-68 is a string literal. > > > >>>>>>>>>>> But I think we are all on the same page to change it into a > > > >> simple > > > >>>>>>>>>>> (non-compound) identifier. > > > >>>>>>>>>>> > > > >>>>>>>>>>> LOAD/UNLOAD MODULE 'core' > > > >>>>>>>>>>> ==> > > > >>>>>>>>>>> LOAD/UNLOAD MODULE core > > > >>>>>>>>>>> > > > >>>>>>>>>>> > > > >>>>>>>>>>> Best, > > > >>>>>>>>>>> Jark > > > >>>>>>>>>>> > > > >>>>>>>>>>> > > > >>>>>>>>>>> On Sat, 30 Jan 2021 at 04:00, Jane Chan < > > qingyue....@gmail.com > > > > > > > >>>>>>> wrote: > > > >>>>>>>>>>> > > > >>>>>>>>>>>> Hi everyone, > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> I would like to start a discussion on FLINK-21045 [1] > about > > > >>>>>>> supporting > > > >>>>>>>>>>>> `LOAD MODULE` and `UNLOAD MODULE` SQL syntax. It's first > > > >> proposed > > > >>>>>> by > > > >>>>>>>>>>>> FLIP-68 [2] as following. > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> -- load a module with the given name and append it to the > > end > > > of > > > >>>>>> the > > > >>>>>>>>>>> module > > > >>>>>>>>>>>> list > > > >>>>>>>>>>>> LOAD MODULE 'name' [WITH ('type'='xxx', 'prop'='myProp', > > ...)] > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> --unload a module by name from the module list and other > > > modules > > > >>>>>>>> remain > > > >>>>>>>>>>> in > > > >>>>>>>>>>>> the same relative positions > > > >>>>>>>>>>>> UNLOAD MODULE 'name' > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> After a round of discussion on the Jira ticket, it seems > > some > > > >>>>>>>>>> unanswered > > > >>>>>>>>>>>> questions need more opinions and suggestions. > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> 1. The way to redefine resolution order easily > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> Rui Li suggested introducing `USE MODULES` and > > adding > > > >>>> similar > > > >>>>>>>>>>>> functionality to the API because > > > >>>>>>>>>>>> > > > >>>>>>>>>>>>> 1) It's very tedious to unload old modules just to > > > reorder > > > >>>>>> them. > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> 2) Users may not even know how to "re-load" an old > > module > > > >> if it > > > >>>>>>> was > > > >>>>>>>>>> not > > > >>>>>>>>>>>>> initially loaded by the user, e.g. don't know which type > to > > > >> use. > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> Jane Chan wondered that module is not like the > > catalog > > > >> which > > > >>>>>>> has > > > >>>>>>>> a > > > >>>>>>>>>>>> concept of namespace could specify, and `USE` sounds like > a > > > >>>>>>>>>>>> mutual-exclusive concept. > > > >>>>>>>>>>>> Maybe `RELOAD MODULES` can express upgrading the > > > >> priority of > > > >>>>>>> the > > > >>>>>>>>>>> loaded > > > >>>>>>>>>>>> module(s). > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> 2. `LOAD/UNLOAD MODULE` v.s. `CREATE/DROP MODULE` syntax > > > >>>>>>>>>>>> Jark Wu and Nicholas Jiang proposed to use > > > `CREATE/DROP > > > >>>>>> MODULE` > > > >>>>>>>>>>> instead > > > >>>>>>>>>>>> of `LOAD/UNLOAD MODULE` because > > > >>>>>>>>>>>> > > > >>>>>>>>>>>>> 1) From a pure SQL user's perspective, maybe `CREATE > > > >> MODULE + > > > >>>>>> USE > > > >>>>>>>>>>>> MODULE` > > > >>>>>>>>>>>>> is easier to use rather than `LOAD/UNLOAD`. > > > >>>>>>>>>>>>> 2) This will be very similar to what the catalog > used > > > now. > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> Timo Walther would rather stick to the agreed design > > > >> because > > > >>>>>>>>>>>> loading/unloading modules is a concept known from kernels > > etc. > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> 3. Simplify the module design by mapping modules purely by > > > name > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> LOAD MODULE geo_utils > > > >>>>>>>>>>>> LOAD MODULE hive WITH ('version'='2.1') -- no dedicated > > > >>>>>>>>>>> 'type='/'module=' > > > >>>>>>>>>>>> but allow only 1 module to be loaded parameterized > > > >>>>>>>>>>>> UNLOAD hive > > > >>>>>>>>>>>> USE MODULES hive, core > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> Please find more details in the reference link. Looking > > > forward > > > >> to > > > >>>>>>>> your > > > >>>>>>>>>>>> feedback. > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> [1] https://issues.apache.org/jira/browse/FLINK-21045# > > > >>>>>>>>>>>> < > > > >>>>>>>>>>>> > > > >>>>>>>>>>> > > > >>>>>>>>>> > > > >>>>>>>> > > > >>>>>>> > > > >>>>>> > > > >>>> > > > >> > > > > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-68%3A+Extend+Core+Table+System+with+Pluggable+Modules > > > >>>>>>>>>>>>> > > > >>>>>>>>>>>> [2] > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> > > > >>>>>>>>>>> > > > >>>>>>>>>> > > > >>>>>>>> > > > >>>>>>> > > > >>>>>> > > > >>>> > > > >> > > > > > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-68%3A+Extend+Core+Table+System+with+Pluggable+Modules > > > >>>>>>>>>>>> > > > >>>>>>>>>>>> Best, > > > >>>>>>>>>>>> Jane > > > >>>>>>>>>>>> > > > >>>>>>>>>>> > > > >>>>>>>>>> > > > >>>>>>>>>> > > > >>>>>>>>>> -- > > > >>>>>>>>>> Best regards! > > > >>>>>>>>>> Rui Li > > > >>>>>>>>>> > > > >>>>>>>>> > > > >>>>>>>> > > > >>>>>>>> > > > >>>>>>> > > > >>>>>> > > > >>>>> > > > >>>>> > > > >>>> > > > >>>> > > > >>> > > > >> > > > >> > > > > > > > > > > > > >
