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
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>>
> >>>>
> >>>>
> >>>
> >>
> >>
> >
>
>
