Hi Bowen,
Thanks for the proposal. I have two thoughts:
1) Regarding to "loadModule", how about
tableEnv.loadModule("xxx" [, propertiesMap]);
tableEnv.unloadModule(“xxx”);
This makes the API similar to SQL. IMO, instance of Module is not needed
and verbose as parameter.
And this makes it easier to load a simple module without any additional
properties, e.g. tEnv.loadModule("GEO"), tEnv.unloadModule("GEO")
2) In current design, the module interface only defines function metadata,
but no implementations.
I'm wondering how to call/map the implementations in runtime? Am I missing
something?
Besides, I left some minor comments in the doc.
Best,
Jark
On Sat, 5 Oct 2019 at 08:42, Xuefu Z <usxu...@gmail.com> wrote:
> I agree with Timo that the new table APIs need to be consistent. I'd go
> further that an name (or id) is needed for module definition in YAML file.
> In the current design, name is skipped and type has binary meanings.
>
> Thanks,
> Xuefu
>
> On Fri, Oct 4, 2019 at 5:24 AM Timo Walther <twal...@apache.org> wrote:
>
> > Hi everyone,
> >
> > first, I was also questioning my proposal. But Bowen's proposal of
> > `tEnv.offloadToYaml(<file_path>)` would not work with the current design
> > because we don't know how to serialize a catalog or module into
> > properties. Currently, there is no converter from instance to
> > properties. It is a one way conversion. We can add a `toProperties`
> > method to both Catalog and Module class in the future to solve this.
> > Solving the table environment serializability can be future work.
> >
> > However, I find the current proposal for the TableEnvironment methods is
> > contradicting:
> >
> > tableEnv.loadModule(new Yyy());
> > tableEnv.unloadModule(“Xxx”);
> >
> > The loading is specified programmatically whereas the unloading requires
> > a string that is not specified in the module itself. But is defined in
> > the factory according to the current design.
> >
> > SQL does it more consistently. There, the name `xxx` is used when
> > loading and unloading the module:
> >
> > LOAD MODULE 'xxx' [WITH ('prop'='myProp', ...)]
> > UNLOAD MODULE 'xxx’
> >
> > How about:
> >
> > tableEnv.loadModule("xxx", new Yyy());
> > tableEnv.unloadModule(“xxx”);
> >
> > This would be similar to the catalog interfaces. The name is not part of
> > the instance itself.
> >
> > What do you think?
> >
> > Thanks,
> > Timo
> >
> >
> >
> >
> > On 01.10.19 21:17, Bowen Li wrote:
> > > If something like the yaml file is the way to go and achieve such
> > > motivation, we would cover that with current design.
> > >
> > > On Tue, Oct 1, 2019 at 12:05 Bowen Li <bowenl...@gmail.com> wrote:
> > >
> > >> Hi Timo, Dawid,
> > >>
> > >> I've added the suggested SQL and related changes to TableEnvironment
> API
> > >> and other classes to the google doc. Also removed "USE MODULE" and its
> > >> APIs. Will update FLIP wiki once we have a consensus.
> > >>
> > >> W.r.t. descriptor approach, my gut feeling is similar to Dawid's.
> > Besides,
> > >> I feel yaml file would be a better solution to persist serializable
> > state
> > >> of an environment as the file itself is in serializable format
> already.
> > >> Though yaml file only serves SQL CLI at this moment, we may be able to
> > >> extend its reach to Table API and allow users to load/offload a
> > >> TableEnvironment from/to yaml files, as something like
> "TableEnvironment
> > >> tEnv = TableEnvironment.loadFromYaml(<file_path>)" and
> > >> "tEnv.offloadToYaml(<file_path>)" to restore and persist state, and
> try
> > to
> > >> make yaml file more expressive.
> > >>
> > >>
> > >> On Tue, Oct 1, 2019 at 6:47 AM Dawid Wysakowicz <
> dwysakow...@apache.org
> > >
> > >> wrote:
> > >>
> > >>> Hi Timo, Bowen,
> > >>>
> > >>> Unfortunately I did not have enough time to go through all the
> > >>> suggestions in details so I can not comment on the whole FLIP.
> > >>>
> > >>> I just wanted to give my opinion on the "descriptor approach in
> > >>> loadModule" part. I am not sure if we need it here. We might be
> > >>> overthinking this a bit. It definitely makes sense for objects like
> > >>> TableSource/TableSink etc. as they are logical definitions that
> nearly
> > >>> always have to be persisted in a Catalog. I'm not sure if we really
> > need
> > >>> the same for a whole session. If we need a resume session feature,
> the
> > >>> way to go would probably be to keep the session in memory on the
> server
> > >>> side. I fear we will never be able to serialize the whole session
> > >>> entirely (temporary objects, objects derived from DataStream etc.)
> > >>>
> > >>> I think it is ok to use instances for objects like Catalogs or
> Modules
> > >>> and have an overlay on top of that that can create instances from
> > >>> properties.
> > >>>
> > >>> Best,
> > >>>
> > >>> Dawid
> > >>>
> > >>> On 01/10/2019 11:28, Timo Walther wrote:
> > >>>> Hi Bowen,
> > >>>>
> > >>>> thanks for your response.
> > >>>>
> > >>>> Re 2) I also don't have a better approach for this issue. It is
> > >>>> similar to changing the general TableConfig between two statements.
> It
> > >>>> would be good to add your explanation to the design document.
> > >>>>
> > >>>> Re 3) It would be interesting to know about which "core" functions
> we
> > >>>> are actually talking about. Also for the overriding built-in
> functions
> > >>>> that we discussed in the other FLIP. But I'm fine with leaving it to
> > >>>> the user for now. How about we just introduce loadModule(),
> > >>>> unloadModule() methods instead of useModules()? This would ensure
> that
> > >>>> users don't forget to add the core module when adding an additional
> > >>>> module and they need to explicitly call "unloadModule('core')".
> > >>>>
> > >>>> Re 4) Every table environment feature should also be designed with
> SQL
> > >>>> statements in mind to verify the concept. SQL is also more popular
> > >>>> that Java/Scala API or YAML file. I would like to add it to 1.10 for
> > >>>> marking the feature as complete.
> > >>>>
> > >>>> SHOW MODULES -> sounds good to me, we should add a listModules():
> > >>>> List<String> method to table environment
> > >>>>
> > >>>> LOAD MODULE 'hive' [WITH ('prop'='myProp', ...)] --> we should add a
> > >>>> loadModule() method to table environment
> > >>>>
> > >>>> UNLOAD MODULE 'hive' --> we should add a unloadModule() method to
> > >>>> table environment
> > >>>>
> > >>>> I would not introduce `USE MODULES 'x' 'y' 'z'` for simplicity and
> > >>>> concise API. Users need to load the module anyway with properties.
> > >>>> They can also load them "in order" immediately. CREATE TABLE can
> also
> > >>>> not create multiple tables but only one at a time in that order.
> > >>>>
> > >>>> One thing that came to my mind, shall we use a descriptor approach
> for
> > >>>> loadModule()? The past has shown that passing instances causes
> > >>>> problems when persisting objects. That's why we also want to get rid
> > >>>> of registerTableSource. I could image that users might want to
> persist
> > >>>> a table environment's state for later use in the future. Even though
> > >>>> this is future work, we should already keep such use cases in mind
> > >>>> when adding new API methods. What do you think?
> > >>>>
> > >>>> Regards,
> > >>>> Timo
> > >>>>
> > >>>>
> > >>>> On 30.09.19 23:17, Bowen Li wrote:
> > >>>>> Hi Timo,
> > >>>>>
> > >>>>> Re 1) I agree. I renamed the title to "Extend Core Table System
> with
> > >>>>> Pluggable Modules" and all internal references
> > >>>>>
> > >>>>> Re 2) First, I'll rename the API to useModules(). The design
> doesn't
> > >>>>> forbid
> > >>>>> users to call useModules() multi times. Objects in modules are
> loaded
> > >>> on
> > >>>>> demand instead of eagerly, so there won't be inconsistency. Users
> > >>>>> have to
> > >>>>> be fully aware of the consequences of resetting modules as that
> might
> > >>>>> cause
> > >>>>> that some objects can not be referenced anymore or resolution order
> > >>>>> of some
> > >>>>> objects changes.
> > >>>>>
> > >>>>> Re 3) Yes, we'd leave that to users.
> > >>>>>
> > >>>>> Another approach can be to have a non-optional "Core" module for
> all
> > >>>>> objects that cannot be overrode like "CAST" and "AS" functions, and
> > >>>>> have an
> > >>>>> optional "ExtendedCore" module for other replaceable built-in
> > objects.
> > >>>>> "Core" should be positioned at the 1st in module list all the time.
> > >>>>>
> > >>>>> I'm fine with either solution.
> > >>>>>
> > >>>>> Re 4) It may sound like a nice-to-have advanced feature for 1.10,
> but
> > >>> we
> > >>>>> can surely fully discuss it for the sake of feature completeness.
> > >>>>>
> > >>>>> Unlike other configs, the order of modules would matter in Flink,
> and
> > >>> it
> > >>>>> implies the LOAD/UNLOAD commands would not be equal in operation
> > >>>>> positions.
> > >>>>> IIUYC, LOAD MODULE 'x' would be interpreted as appending x to the
> end
> > >>> of
> > >>>>> module list, and UNLOAD MODULE 'x' would be interpreted as
> removing x
> > >>>>> from
> > >>>>> any position in the list?
> > >>>>>
> > >>>>> I'm thinking of the following list of commands:
> > >>>>>
> > >>>>> SHOW MODULES - list modules in order
> > >>>>> LOAD MODULE 'hive' [WITH ('prop'='myProp', ...)] - load and append
> > the
> > >>>>> module to end of the module list
> > >>>>> UNLOAD MODULE 'hive' - remove the module from module list, and
> other
> > >>>>> modules remain the same relative positions
> > >>>>> USE MODULES 'x' 'y' 'z' (wondering can parser take "'x' 'y' 'z'"?),
> > >>>>> or USE
> > >>>>> MODULES 'x,y,z' - to reorder module list completely
> > >>>>>
> > >>>
> >
> >
>
> --
> Xuefu Zhang
>
> "In Honey We Trust!"
>
