Hi Aljoscha, Timo Thanks for the reminder. I've update the details in FLIP wiki, and will kick off a voting thread.
On Fri, Oct 4, 2019 at 1:51 PM Timo Walther <[email protected]> wrote: > Hi, > > I agree with Aljoscha. It is not transparent to me which votes are > binding to the current status of the FLIP. > > Some other minor comments from my side: > > - We don't need to deprecate methods in FunctionCatalog. This class is > internal. We can simply change the method signatures. > - `String name` is missing in the FunctionIdentifier code example; can > we call FunctionIdentifier.getSimpleName() just > FunctionIdentifier.getName()? > - Add the methods that we discussed to the example: `of(String)`, > `of(ObjectIdentifier)` > > Other than that, I'm happy to give my +1 to this proposal. > > Thanks for the productive discussion, > Timo > > > On 04.10.19 13:29, Aljoscha Krettek wrote: > > Hi, > > > > I see there was quite some discussion and changes on the FLIP after this > VOTE was started. I would suggest to start a new voting thread on the > current state of the FLIP (keeping in mind that a FLIP vote needs at least > three committer/PMC votes). > > > > For the future, we should probably keep discussion to the [DISCUSS] > thread and use the vote thread only for voting. > > > > Best, > > Aljoscha > > > >> On 3. Oct 2019, at 21:17, Bowen Li <[email protected]> wrote: > >> > >> I'm glad to announce that the community has accepted the design of > FLIP-57, > >> and we are moving forward to implementing it. > >> > >> Thanks everyone! > >> > >> On Wed, Oct 2, 2019 at 11:01 AM Bowen Li <[email protected]> wrote: > >> > >>> Introducing a new term "path" to APIs like > >>> "getShortPath(Identifier)/getLongPath(Identifier)" would be confusing > to > >>> users, thus I feel "getSimpleName/getIdentifier" is fine. > >>> > >>> To summarize the discussion result. > >>> > >>> - categorize functions into 2 dimensions - system v.s. catalog, > >>> non-temp v.s. temp - and that give us 4 combinations > >>> - definition of FunctionIdentifier > >>> > >>> @PublicEvolving > >>> > >>> Class FunctionIdentifier { > >>> > >>> String name; > >>> > >>> ObjectIdentifier oi; > >>> > >>> // for temporary/non-temporary system function > >>> public FunctionIdentifier of(String name) { } > >>> // for temporary/non-temporary catalog function > >>> public FunctionIdentifier of(ObjectIdentifier identifier) { } > >>> > >>> > >>> Optional<ObjectIdentifier> getIdentifier() {} > >>> > >>> Optional<String> getSimpleName() {} > >>> > >>> } > >>> > >>> > >>> I've updated them to FLIP wiki. Please take a final look. I'll close > the > >>> voting if there's no other concern raised within 24 hours. > >>> > >>> Cheers > >>> > >>> On Wed, Oct 2, 2019 at 4:54 AM Dawid Wysakowicz < > [email protected]> > >>> wrote: > >>> > >>>> Hi, > >>>> > >>>> I very much agree with Xuefu's summary of the two points, especially > on > >>>> the "functionIdentifier doesn't need to reflect the categories". > >>>> > >>>> For the factory methods I think methods of should be enough: > >>>> > >>>> // for temporary/non-temporary system function > >>>> public FunctionIdentifier of(String name) { } > >>>> // for temporary/non-temporary catalog function > >>>> public FunctionIdentifier of(ObjectIdentifier identifier){ } > >>>> > >>>> In case of the getters I did not like the method name `getName` in the > >>>> original proposal, as in my opinion it could imply that it can return > >>>> also just the name part of an ObjectIdentifier, which should not be > the > >>>> case. > >>>> > >>>> I'm fine with getSimpleName/getIdentifier, but want to throw in few > >>>> other suggestions: > >>>> > >>>> * getShortPath(Identifier)/getLongPath(Identifier), > >>>> > >>>> * getSystemPath(Identifier)/getCatalogPath(Identifier) > >>>> > >>>> +1 to any of the 3 options. > >>>> > >>>> One additional thing the FunctionIdentifier should be a PublicEvolving > >>>> class, as it is part of a PublicEvolving APIs e.g. CallExpression, > which > >>>> user might need to access e.g. in a filter pushdown. > >>>> > >>>> I also support the Xuefu's suggestion not to support the "ALL" keyword > >>>> in the "SHOW [TEMPORARY] FUNCTIONS" statement, but as the exact design > >>>> of it is not part of the FLIP-57, we do not need to agree on that in > >>>> this thread. > >>>> > >>>> Overall I think after updating the FLIP with the outcome of the > >>>> discussion I vote +1 for it. > >>>> > >>>> Best, > >>>> > >>>> Dawid > >>>> > >>>> > >>>> On 02/10/2019 00:21, Xuefu Z wrote: > >>>>> Here are some of my thoughts on the minor debates above: > >>>>> > >>>>> 1. +1 for 4 categories of functions. They are categorized along two > >>>>> dimensions of binary values: X: *temporary* vs non-temporary > >>>> (persistent); > >>>>> Y: *system* vs non-system (so said catalog). > >>>>> 2. In my opinion, class functionIdentifier doesn't really need to > >>>> reflect > >>>>> the categories of the functions. Instead, we should decouple them to > >>>> make > >>>>> the API more stable. Thus, my suggestion is: > >>>>> > >>>>> @Internal > >>>>> class FunctionIdentifier { > >>>>> // for temporary/non-temporary system function > >>>>> public FunctionIdentifier ofSimpleName(String name) { } > >>>>> // for temporary/non-temporary catalog function > >>>>> public FunctionIdentifier ofIdentifier(ObjectIdentifier > >>>>> identifier){ } > >>>>> public Optional<String> getSimpleName() { } > >>>>> public Optional<ObjectIdentifier> getIdentifier() { } > >>>>> } > >>>>> 3. DDLs -- I don't think we need "ALL" keyword. The grammar can just > be: > >>>>> > >>>>> SHOW [TEMPORARY] [SYSTEM] FUNCTIONS. > >>>>> > >>>>> When either keyword is missing, "ALL" is implied along that > dimension. > >>>> We > >>>>> should always limit the search to the system function catalog and the > >>>>> current catalog/DB. I don't see a need of listing functions across > >>>>> different catalogs and databases. (It can be added later if that > >>>> arises.) > >>>>> Thanks, > >>>>> Xuefu > >>>>> > >>>>> On Tue, Oct 1, 2019 at 11:12 AM Bowen Li <[email protected]> > wrote: > >>>>> > >>>>>> Hi Dawid, > >>>>>> > >>>>>> Thanks for bringing the suggestions up. I was prototyping yesterday > and > >>>>>> found out those places exactly as what you suggested. > >>>>>> > >>>>>> For CallExpression and UnresolvedCallExpression, I've added them to > >>>>>> FLIP-57. We will replace ObjectIdentifier with FunctionIdentifier > and > >>>> mark > >>>>>> that as a breaking change > >>>>>> > >>>>>> For FunctionIdentifier, the suggested changes LGTM. Just want to > bring > >>>> up > >>>>>> an issue on naming. It seems to me how we now name functions > >>>> categories is > >>>>>> a bit unclear and confusing, which is reflected on the suggested > APIs > >>>> - in > >>>>>> FunctionIdentifier you lay out, "builtin function" would include > >>>> builtin > >>>>>> functions and temporary system functions as we are kind of using > >>>> "system" > >>>>>> and "built-in" interchangeably, and "catalog function" would include > >>>>>> catalog functions and temporary functions. I currently can see two > >>>>>> approaches to make it clearer to users. > >>>>>> > >>>>>> 1) Simplify FunctionIdentifier to be the following. As it's > internal, > >>>> we > >>>>>> add comments and explanation to devs on which case the APIs support. > >>>>>> However, I feel this approach would be somehow a bit conflicting to > >>>> what > >>>>>> you want to achieve for the clarity of APIs > >>>>>> > >>>>>> @Internal > >>>>>> class FunctionIdentifier { > >>>>>> // for built-in function and temporary system function > >>>>>> public FunctionIdentifier of(String name) { } > >>>>>> // for temporary function and catalog function > >>>>>> public FunctionIdentifier of(ObjectIdentifier identifier){ } > >>>>>> public Optional<String> getFunctionName() { } > >>>>>> public Optional<ObjectIdentifier> getObjectIdentifier() { } > >>>>>> } > >>>>>> > >>>>>> 2) We can rename our function categories as following so there'll be > >>>> mainly > >>>>>> just two categories of functions, "system functions" and "catalog > >>>>>> functions", either of which can have temporary ones > >>>>>> > >>>>>> - built-in functions -> officially rename to "system functions" > and > >>>> note > >>>>>> to users that "system" and "built-in" can be used interchangeably. > We > >>>>>> prefer "system" because that's the keyword we decided to use in DDL > >>>> that > >>>>>> creates its temporary peers ("CREATE TEMPORARY SYSTEM FUNCTION") > >>>>>> - temporary system functions > >>>>>> - catalog functions > >>>>>> - temporary functions -> rename to "temporary catalog function" > >>>>>> > >>>>>> @Internal > >>>>>> class FunctionIdentifier { > >>>>>> // for temporary/non-temporary system function > >>>>>> public FunctionIdentifier ofSystemFunction(String name) { } > >>>>>> // for temporary/non-temporary catalog function > >>>>>> public FunctionIdentifier ofCatalogFunction(ObjectIdentifier > >>>>>> identifier){ } > >>>>>> public Optional<String> getSystemFunctionName() { } > >>>>>> public Optional<ObjectIdentifier> getCatalogFunctionIdentifier() > >>>> { } > >>>>>> } > >>>>>> > >>>>>> WDYT? > >>>>>> > >>>>>> > >>>>>> On Tue, Oct 1, 2019 at 5:48 AM Fabian Hueske <[email protected]> > >>>> wrote: > >>>>>>> Thanks for the summary Bowen! > >>>>>>> > >>>>>>> Looks good to me. > >>>>>>> > >>>>>>> Cheers, > >>>>>>> Fabian > >>>>>>> > >>>>>>> Am Mo., 30. Sept. 2019 um 23:24 Uhr schrieb Bowen Li < > >>>>>> [email protected] > >>>>>>>> : > >>>>>>>> Hi all, > >>>>>>>> > >>>>>>>> I've updated the FLIP wiki with the following changes: > >>>>>>>> > >>>>>>>> - Lifespan of temp functions are not tied to those of catalogs and > >>>>>>>> databases. Users can create temp functions even though > catalogs/dbs > >>>> in > >>>>>>>> their fully qualified names don't even exist. > >>>>>>>> - some new SQL commands > >>>>>>>> - "SHOW FUNCTIONS" - list names of temp and non-temp > >>>>>> system/built-in > >>>>>>>> functions, and names of temp and catalog functions in the current > >>>>>> catalog > >>>>>>>> and db > >>>>>>>> - "SHOW ALL FUNCTIONS" - list names of temp and non-temp > >>>>>> system/built > >>>>>>>> functions, and fully qualified names of temp and catalog > functions in > >>>>>> all > >>>>>>>> catalogs and dbs > >>>>>>>> - "SHOW ALL TEMPORARY FUNCTIONS" - list fully qualified names > of > >>>>>> temp > >>>>>>>> functions in all catalog and db > >>>>>>>> - "SHOW ALL TEMPORARY SYSTEM FUNCTIONS" - list names of all > temp > >>>>>>> system > >>>>>>>> functions > >>>>>>>> > >>>>>>>> Let me know if you have any questions. > >>>>>>>> > >>>>>>>> Seems we have resolved all concerns. If there's no more ones, I'd > >>>> like > >>>>>> to > >>>>>>>> close the vote by this time tomorrow. > >>>>>>>> > >>>>>>>> Cheers, > >>>>>>>> Bowen > >>>>>>>> > >>>>>>>> On Mon, Sep 30, 2019 at 11:59 AM Bowen Li <[email protected]> > >>>> wrote: > >>>>>>>>> Hi, > >>>>>>>>> > >>>>>>>>> I think above are some valid points, and we can adopt the > >>>>>> suggestions. > >>>>>>>>> To elaborate a bit on the new SQL syntax, it would imply that, > >>>> unlike > >>>>>>>>> "SHOW FUNCTION" which only return function names, "SHOW ALL > >>>>>> [TEMPORARY] > >>>>>>>>> FUNCTIONS" would return functions' fully qualified names with > >>>> catalog > >>>>>>> and > >>>>>>>>> db names. > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> On Mon, Sep 30, 2019 at 6:38 AM Timo Walther <[email protected] > > > >>>>>>> wrote: > >>>>>>>>>> Hi all, > >>>>>>>>>> > >>>>>>>>>> I support Fabian's arguments. In my opinion, temporary objects > >>>>>> should > >>>>>>>>>> just be an additional layer on top of the regular > catalog/database > >>>>>>>>>> lookup logic. Thus, a temporary table or function has always > >>>> highest > >>>>>>>>>> precedence and should be stable within the local session. > Otherwise > >>>>>> it > >>>>>>>>>> could magically disappear while someone else is performing > >>>>>>> modifications > >>>>>>>>>> in the catalog. > >>>>>>>>>> > >>>>>>>>>> Furthermore, this feature is very useful for prototyping as > users > >>>>>> can > >>>>>>>>>> simply express that a catalog/database is present even through > they > >>>>>>>>>> might not have access to it currently. > >>>>>>>>>> > >>>>>>>>>> Regards, > >>>>>>>>>> Timo > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> On 30.09.19 14:57, Fabian Hueske wrote: > >>>>>>>>>>> Hi all, > >>>>>>>>>>> > >>>>>>>>>>> Sorry for the late reply. > >>>>>>>>>>> > >>>>>>>>>>> I think it might lead to confusing situations if temporary > >>>>>> functions > >>>>>>>> (or > >>>>>>>>>>> any temporary db objects for that matter) are bound to the life > >>>>>>> cycle > >>>>>>>>>> of an > >>>>>>>>>>> (external) db/catalog. > >>>>>>>>>>> Imaging a situation where you create a temp function in a db > in an > >>>>>>>>>> external > >>>>>>>>>>> catalog and use it but at some point it does not work anymore > >>>>>>> because > >>>>>>>>>> some > >>>>>>>>>>> other dropped the database from the external catalog. > >>>>>>>>>>> Shouldn't temporary objects be only controlled by the owner of > a > >>>>>>>>>> session? > >>>>>>>>>>> I agree that creating temp objects in non-existing db/catalogs > >>>>>>> sounds > >>>>>>>> a > >>>>>>>>>> bit > >>>>>>>>>>> strange, but IMO the opposite (the db/catalog must exist for a > >>>>>> temp > >>>>>>>>>>> function to be created/exist) can have significant implications > >>>>>> like > >>>>>>>> the > >>>>>>>>>>> one I described. > >>>>>>>>>>> I think it would be quite easy for users to understand that > >>>>>>> temporary > >>>>>>>>>>> objects are solely owned by them (and their session). > >>>>>>>>>>> The problem of listing temporary objects could be solved by > >>>>>> adding a > >>>>>>>> ALL > >>>>>>>>>>> [TEMPORARY] clause: > >>>>>>>>>>> > >>>>>>>>>>> SHOW ALL FUNCTIONS; could show all functions regardless of the > >>>>>>>>>>> catalog/database including temporary functions. > >>>>>>>>>>> SHOW ALL TEMPORARY FUNCTIONS; could show all temporary > functions > >>>>>>>>>> regardless > >>>>>>>>>>> of the catalog/database. > >>>>>>>>>>> > >>>>>>>>>>> Best, > >>>>>>>>>>> Fabian > >>>>>>>>>>> > >>>>>>>>>>> Am Sa., 28. Sept. 2019 um 02:21 Uhr schrieb Bowen Li < > >>>>>>>>>> [email protected]>: > >>>>>>>>>>>> @Dawid, do you have any other concerns? If not, I hope we can > >>>>>> close > >>>>>>>> the > >>>>>>>>>>>> voting. > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> On Thu, Sep 26, 2019 at 8:14 PM Rui Li <[email protected] > > > >>>>>>>> wrote: > >>>>>>>>>>>>> I'm not sure how much benefit #1 can bring us. If users just > >>>>>> want > >>>>>>> to > >>>>>>>>>> try > >>>>>>>>>>>>> out temporary functions, they can create temporary system > >>>>>>> functions > >>>>>>>>>> which > >>>>>>>>>>>>> don't require a catalog/DB. IIUC, the main reason why we > allow > >>>>>>>>>> temporary > >>>>>>>>>>>>> catalog function is to let users override permanent catalog > >>>>>>>> functions. > >>>>>>>>>>>>> Therefore a temporary function in a non-existing catalog > won't > >>>>>>> serve > >>>>>>>>>> that > >>>>>>>>>>>>> purpose. Besides, each session is provided with a default > >>>>>> catalog > >>>>>>>> and > >>>>>>>>>> DB. > >>>>>>>>>>>>> So even if users simply want to create some catalog functions > >>>>>> they > >>>>>>>> can > >>>>>>>>>>>>> forget about after the session, wouldn't the default > catalog/DB > >>>>>> be > >>>>>>>>>> enough > >>>>>>>>>>>>> for such experiments? > >>>>>>>>>>>>> > >>>>>>>>>>>>> On Thu, Sep 26, 2019 at 4:38 AM Bowen Li < > [email protected]> > >>>>>>>> wrote: > >>>>>>>>>>>>>> Re 1) As described in the FLIP, a temp function lookup will > >>>>>> first > >>>>>>>>>> make > >>>>>>>>>>>>> sure > >>>>>>>>>>>>>> the db exists. If the db doesn't exist, a lazy drop is > >>>>>> triggered > >>>>>>> to > >>>>>>>>>>>>> remove > >>>>>>>>>>>>>> that temp function. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> I agree Hive doesn't handle it consistently, and we are not > >>>>>>> copying > >>>>>>>>>>>> Hive. > >>>>>>>>>>>>>> IMHO, allowing registering temp functions in nonexistent > >>>>>>> catalog/db > >>>>>>>>>> is > >>>>>>>>>>>>>> hacky and problematic. For instance, "SHOW FUNCTIONS" would > >>>>>> list > >>>>>>>>>> system > >>>>>>>>>>>>>> functions and functions in the current catalog/db, since > users > >>>>>>>> cannot > >>>>>>>>>>>>>> designate a nonexistent catalog/db as current ones, how can > >>>>>> they > >>>>>>>> list > >>>>>>>>>>>>>> functions in nonexistent catalog/db? They may end up never > >>>>>>> knowing > >>>>>>>>>> what > >>>>>>>>>>>>>> temp functions they've created unless trying out with > queries > >>>>>> or > >>>>>>> we > >>>>>>>>>>>>>> introducing some more nonstandard SQL statements. The same > >>>>>>> applies > >>>>>>>> to > >>>>>>>>>>>>> other > >>>>>>>>>>>>>> temp objects like temp tables. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Re 2) A standalone FunctionIdentifier sounds good to me > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> On Wed, Sep 25, 2019 at 4:46 AM Dawid Wysakowicz < > >>>>>>>>>>>>>> [email protected]> > >>>>>>>>>>>>>> wrote: > >>>>>>>>>>>>>> > >>>>>>>>>>>>>>> Ad. 1 > >>>>>>>>>>>>>>> I wouldn't say it is hacky. > >>>>>>>>>>>>>>> Moreover, how do you want ensure that the dB always exists > >>>>>> when > >>>>>>> a > >>>>>>>>>>>>>> temporary > >>>>>>>>>>>>>>> object is used?( in this particular case function). Do you > >>>>>> want > >>>>>>> to > >>>>>>>>>>>>> query > >>>>>>>>>>>>>>> for the database existence whenever e.g a temporary > function > >>>>>> is > >>>>>>>>>>>> used? I > >>>>>>>>>>>>>>> think important aspect here is that the database can be > >>>>>> dropped > >>>>>>>> from > >>>>>>>>>>>>>>> external system, not just flink or a different flink > session. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> E.g in case of hive, you cannot create a temporary table > in a > >>>>>>>>>>>> database > >>>>>>>>>>>>>> that > >>>>>>>>>>>>>>> does not exist, that's true. But if you create a temporary > >>>>>> table > >>>>>>>> in > >>>>>>>>>> a > >>>>>>>>>>>>>>> database and drop that database from a different session, > you > >>>>>>> can > >>>>>>>>>>>> still > >>>>>>>>>>>>>>> query the previously created temporary table from the > original > >>>>>>>>>>>> session. > >>>>>>>>>>>>>> It > >>>>>>>>>>>>>>> does not sound like a consistent behaviour to me. Why > don't we > >>>>>>>> make > >>>>>>>>>>>>> this > >>>>>>>>>>>>>>> behaviour of not binding a temporary objects to the > lifetime > >>>>>> of > >>>>>>> a > >>>>>>>>>>>>>> database > >>>>>>>>>>>>>>> explicit as part of the temporary objects contract? In the > end > >>>>>>>> they > >>>>>>>>>>>>> exist > >>>>>>>>>>>>>>> in different layers. Permanent objects & databases in a > >>>>>> catalog > >>>>>>>> (in > >>>>>>>>>>>>> case > >>>>>>>>>>>>>> of > >>>>>>>>>>>>>>> hive megastore) whereas temporary objects in flink > sessions. > >>>>>>>> That's > >>>>>>>>>>>>> also > >>>>>>>>>>>>>>> true for the original hive client. The temporary objects > live > >>>>>> in > >>>>>>>> the > >>>>>>>>>>>>> hive > >>>>>>>>>>>>>>> client whereas databases are created in the metastore. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> Ad.2 > >>>>>>>>>>>>>>> I'm open for suggestions here. The one thing I wanted to > >>>>>> achieve > >>>>>>>>>> here > >>>>>>>>>>>>> is > >>>>>>>>>>>>>> so > >>>>>>>>>>>>>>> that we do not change the contract of ObjectIdentifier. One > >>>>>>>>>> important > >>>>>>>>>>>>>> thing > >>>>>>>>>>>>>>> to remember here is that we need the function identifier > to be > >>>>>>>> part > >>>>>>>>>>>> of > >>>>>>>>>>>>>> the > >>>>>>>>>>>>>>> FunctionDefinition object and not only as the result of the > >>>>>>>> function > >>>>>>>>>>>>>>> lookup. At some point we want to be able to store > >>>>>>> QueryOperations > >>>>>>>> in > >>>>>>>>>>>>> the > >>>>>>>>>>>>>>> catalogs. They can contain function calls within which we > need > >>>>>>> to > >>>>>>>>>>>> have > >>>>>>>>>>>>>> the > >>>>>>>>>>>>>>> identifier. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> I agree my initial suggestion is over complicated. How > about > >>>>>> we > >>>>>>>> have > >>>>>>>>>>>>> just > >>>>>>>>>>>>>>> the FunctionIdentifier as top level class without making > the > >>>>>>>>>>>>>>> ObjectIdentifier extend from it? I think it's pretty much > the > >>>>>>> same > >>>>>>>>>>>> what > >>>>>>>>>>>>>> you > >>>>>>>>>>>>>>> suggested. The only difference is that it would be a top > level > >>>>>>>> class > >>>>>>>>>>>>>> with a > >>>>>>>>>>>>>>> more descriptive name. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> On Wed, 25 Sep 2019, 13:57 Bowen Li, <[email protected]> > >>>>>>> wrote: > >>>>>>>>>>>>>>>> Sorry, I missed some parts of the solution. The complete > >>>>>>>>>>>> alternative > >>>>>>>>>>>>> is > >>>>>>>>>>>>>>> the > >>>>>>>>>>>>>>>> following, basically having separate APIs in > FunctionLookup > >>>>>> for > >>>>>>>>>>>>>> ambiguous > >>>>>>>>>>>>>>>> and precise function lookup since planner is able to tell > >>>>>> which > >>>>>>>> API > >>>>>>>>>>>>> to > >>>>>>>>>>>>>>> call > >>>>>>>>>>>>>>>> with parsed queries, and have a unified result: > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> ``` > >>>>>>>>>>>>>>>> class FunctionLookup { > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> Optional<Result> lookupAmbiguousFunction(String name); > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> Optional<Result> lookupPreciseFunction(ObjectIdentifier > oi); > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> class Result { > >>>>>>>>>>>>>>>> Optional<ObjectIdentifier> getObjectIdentifier() { > ... > >>>>>> } > >>>>>>>>>>>>>>>> String getName() { ... } > >>>>>>>>>>>>>>>> // ... > >>>>>>>>>>>>>>>> } > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> } > >>>>>>>>>>>>>>>> ``` > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> Thanks, > >>>>>>>>>>>>>>>> Bowen > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> On Tue, Sep 24, 2019 at 9:42 PM Bowen Li < > >>>>>> [email protected]> > >>>>>>>>>>>>> wrote: > >>>>>>>>>>>>>>>>> Hi Dawid, > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> Re 1): I agree making it easy for users to run > experiments > >>>>>> is > >>>>>>>>>>>>>>> important. > >>>>>>>>>>>>>>>>> However, I'm not sure allowing users to register temp > >>>>>>> functions > >>>>>>>>>>>> in > >>>>>>>>>>>>>>>>> nonexistent catalog/db is the optimal way. It seems a bit > >>>>>>> hacky, > >>>>>>>>>>>>> and > >>>>>>>>>>>>>>>> breaks > >>>>>>>>>>>>>>>>> the current contract between Flink and users that > catalog/db > >>>>>>>> must > >>>>>>>>>>>>> be > >>>>>>>>>>>>>>>> valid > >>>>>>>>>>>>>>>>> in order to operate on. > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> How about we instead focus on making it convenient to > create > >>>>>>>>>>>>>> catalogs? > >>>>>>>>>>>>>>>>> Users actually can already do it with ease via program or > >>>>>> SQL > >>>>>>>> CLI > >>>>>>>>>>>>>> yaml > >>>>>>>>>>>>>>>> file > >>>>>>>>>>>>>>>>> for an in-memory catalog which has neither extra > dependency > >>>>>>> nor > >>>>>>>>>>>>>>> external > >>>>>>>>>>>>>>>>> connections. What we can further improve is DDL for > >>>>>> catalogs, > >>>>>>>>>>>> and I > >>>>>>>>>>>>>>>> raised > >>>>>>>>>>>>>>>>> it in discussion of [FLIP 69 - Flink SQL DDL Enhancement] > >>>>>>> driven > >>>>>>>>>>>> by > >>>>>>>>>>>>>>> Terry > >>>>>>>>>>>>>>>>> now. > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> In that case, if users would like to experiment via SQL, > >>>>>> they > >>>>>>>> can > >>>>>>>>>>>>>>> easily > >>>>>>>>>>>>>>>>> create an in memory catalog/database using DDL, then play > >>>>>> with > >>>>>>>>>>>> temp > >>>>>>>>>>>>>>>>> functions. > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> Re 2): For the assumption, IIUIC, function > ObjectIdentifier > >>>>>>> has > >>>>>>>>>>>> not > >>>>>>>>>>>>>>> been > >>>>>>>>>>>>>>>>> resolved when stack call reaches > >>>>>>>>>>>> FunctionCatalog#lookupFunction(), > >>>>>>>>>>>>>> but > >>>>>>>>>>>>>>>> only > >>>>>>>>>>>>>>>>> been parsed? > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> I agree keeping ObjectIdentifier as-is would be good. > I'm ok > >>>>>>>> with > >>>>>>>>>>>>> the > >>>>>>>>>>>>>>>>> suggested classes, though making ObjectIdentifier a > subclass > >>>>>>> of > >>>>>>>>>>>>>>>>> FunctionIdentifier seem a bit counter intuitive. > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> Another potentially simpler way is: > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> ``` > >>>>>>>>>>>>>>>>> // in class FunctionLookup > >>>>>>>>>>>>>>>>> class Result { > >>>>>>>>>>>>>>>>> Optional<ObjectIdentifier> getObjectIdentifier() { > >>>>>> ... } > >>>>>>>>>>>>>>>>> String getName() { ... } > >>>>>>>>>>>>>>>>> ... > >>>>>>>>>>>>>>>>> } > >>>>>>>>>>>>>>>>> ``` > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> WDYT? > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> On Tue, Sep 24, 2019 at 3:41 PM Dawid Wysakowicz < > >>>>>>>>>>>>>>>>> [email protected]> wrote: > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> Hi, > >>>>>>>>>>>>>>>>>> I really like the flip and think it clarifies important > >>>>>>> aspects > >>>>>>>>>>>> of > >>>>>>>>>>>>>> the > >>>>>>>>>>>>>>>>>> system. > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> I have two, I hope small suggestions, which will not > take > >>>>>>> much > >>>>>>>>>>>>> time > >>>>>>>>>>>>>> to > >>>>>>>>>>>>>>>>>> agree on. > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> 1. Could we follow the MySQL approach in regards to the > >>>>>>>>>>>> existence > >>>>>>>>>>>>> of > >>>>>>>>>>>>>>>>>> cat/db > >>>>>>>>>>>>>>>>>> for temporary functions? That means not to check it, so > >>>>>> e.g. > >>>>>>>>>>>> it's > >>>>>>>>>>>>>>>> possible > >>>>>>>>>>>>>>>>>> to create a temporary function in a database that does > not > >>>>>>>>>>>> exist. > >>>>>>>>>>>>> I > >>>>>>>>>>>>>>>> think > >>>>>>>>>>>>>>>>>> it's really useful e.g in cases when user wants to > perform > >>>>>>>>>>>>>> experiments > >>>>>>>>>>>>>>>> but > >>>>>>>>>>>>>>>>>> does not have access to the db yet or temporarily does > not > >>>>>>> have > >>>>>>>>>>>>>>>> connection > >>>>>>>>>>>>>>>>>> to a catalog. > >>>>>>>>>>>>>>>>>> 2. Could we not change the ObjectIdentifier? Could we > not > >>>>>>>> loosen > >>>>>>>>>>>>> the > >>>>>>>>>>>>>>>>>> requirements for all catalog objects such as tables, > views, > >>>>>>>>>>>> types > >>>>>>>>>>>>>> just > >>>>>>>>>>>>>>>> for > >>>>>>>>>>>>>>>>>> the functions? It's really important later on from e.g > the > >>>>>>>>>>>>>>>> serializability > >>>>>>>>>>>>>>>>>> perspective. The important aspect of the > ObjectIdentifier > >>>>>> is > >>>>>>>>>>>> that > >>>>>>>>>>>>> we > >>>>>>>>>>>>>>>> know > >>>>>>>>>>>>>>>>>> that it has been resolved. The suggested changes break > that > >>>>>>>>>>>>>>> assumption. > >>>>>>>>>>>>>>>>>> What do you think about adding an interface > >>>>>>> FunctionIdentifier > >>>>>>>> { > >>>>>>>>>>>>>>>>>> String getName(); > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> /** > >>>>>>>>>>>>>>>>>> Return 3-part identifier. Empty in case of a built-in > >>>>>>>>>>>> function. > >>>>>>>>>>>>>>>>>> */ > >>>>>>>>>>>>>>>>>> Optional<ObjectIdentifier> getObjectIdentifier() > >>>>>>>>>>>>>>>>>> } > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> class ObjectIdentifier implements FunctionIdentifier { > >>>>>>>>>>>>>>>>>> Optional<ObjectIdentifier> getObjectIdentifier() { > >>>>>>>>>>>>>>>>>> return Optional.of(this); > >>>>>>>>>>>>>>>>>> } > >>>>>>>>>>>>>>>>>> } > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> class SystemFunctionIdentifier implements > >>>>>> FunctionIdentifier > >>>>>>>>>>>> {...} > >>>>>>>>>>>>>>>>>> WDYT? > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> On Wed, 25 Sep 2019, 04:50 Xuefu Z, <[email protected]> > >>>>>>> wrote: > >>>>>>>>>>>>>>>>>>> +1. LGTM > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> On Tue, Sep 24, 2019 at 6:09 AM Terry Wang < > >>>>>>>>>>>> [email protected]> > >>>>>>>>>>>>>>>> wrote: > >>>>>>>>>>>>>>>>>>>> +1 > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> Best, > >>>>>>>>>>>>>>>>>>>> Terry Wang > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> 在 2019年9月24日，上午10:42，Kurt Young <[email protected]> > 写道： > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> +1 > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> Best, > >>>>>>>>>>>>>>>>>>>>> Kurt > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> On Tue, Sep 24, 2019 at 2:30 AM Bowen Li < > >>>>>>>>>>>>> [email protected] > >>>>>>>>>>>>>>>>>> wrote: > >>>>>>>>>>>>>>>>>>>>>> Hi all, > >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> I'd like to start a voting thread for FLIP-57 [1], > >>>>>> which > >>>>>>>>>>>>>> we've > >>>>>>>>>>>>>>>>>> reached > >>>>>>>>>>>>>>>>>>>>>> consensus in [2]. > >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> This voting will be open for minimum 3 days till > 6:30pm > >>>>>>>>>>>>> UTC, > >>>>>>>>>>>>>>> Sep > >>>>>>>>>>>>>>>>>> 26. > >>>>>>>>>>>>>>>>>>>>>> Thanks, > >>>>>>>>>>>>>>>>>>>>>> Bowen > >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> [1] > >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> > >>>> > https://cwiki.apache.org/confluence/display/FLINK/FLIP-57%3A+Rework+FunctionCatalog > >>>>>>>>>>>>>>>>>>>>>> [2] > >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> > >>>> > http://apache-flink-mailing-list-archive.1008284.n3.nabble.com/DISCUSS-FLIP-57-Rework-FunctionCatalog-td32291.html#a32613 > >>>>>>>>>>>>>>>>>>> -- > >>>>>>>>>>>>>>>>>>> Xuefu Zhang > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> "In Honey We Trust!" > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>> -- > >>>>>>>>>>>>> Best regards! > >>>>>>>>>>>>> Rui Li > >>>>>>>>>>>>> > >>>> > >
