I'm glad to hear that this is in your plan. Sorry that I overlooked the PoC link in the FLIP previously, I'll go over the code of PoC, and post here if there are any more concerns.
Xuyang <xyzhong...@163.com> 于2023年12月21日周四 10:39写道： > > Hi, Benchao. > > > When Feng Jin and I tried the poc together, we found that when using udaf, > Calcite directly using the function's input parameters from > SqlCall#getOperandList. But in fact, these input parameters may use named > arguments, the order of parameters may be wrong, and they may not include > optional parameters that need to set default values. Actually, we should use > new SqlCallBinding(this, scope, call).operands() to let this method correct > the order and add default values. (You can see the modification in > SqlToRelConverter in poc branch[1]) > > > We have not reported this bug to the calcite community yet. Our original plan > was to report this bug to the calcite community during the process of doing > this flip, and fix it separately in flink's own calcite file. Because the > time for Calcite to release the version is uncertain. And the time to upgrade > flink to the latest calcite version is also unknown. > > > The link to the poc code is at the bottom of the flip[2]. I'm post it here > again[1]. > > > [1] > https://cwiki.apache.org/confluence/display/FLINK/FLIP-387%3A+Support+named+parameters+for+functions+and+call+procedures > [2] > https://github.com/apache/flink/compare/master...hackergin:flink:poc_named_argument > > > > -- > > Best！ > Xuyang > > > > > > 在 2023-12-20 13:31:26，"Benchao Li" <libenc...@apache.org> 写道： > >I didn't see your POC code, so I assumed that you'll need to add > >SqlStdOperatorTable#DEFAULT and > >SqlStdOperatorTable#ARGUMENT_ASSIGNMENT to FlinkSqlOperatorTable, am I > >right? > > > >If yes, this would enable many builtin functions to allow default and > >optional arguments, for example, `select md5(DEFAULT)`, I guess this > >is not what we want to support right? If so, I would suggest to throw > >proper errors for these unexpected usages. > > > >Benchao Li <libenc...@apache.org> 于2023年12月20日周三 13:16写道： > >> > >> Thanks Feng for driving this, it's a very useful feature. > >> > >> In the FLIP, you mentioned that > >> > During POC verification, bugs were discovered in Calcite that caused > >> > issues during the validation phase. We need to modify the > >> > SqlValidatorImpl and SqlToRelConverter to address these problems. > >> > >> Could you log bugs in Calcite, and reference the corresponding Jira > >> number in your code. We want to upgrade Calcite version to latest as > >> much as possible, and maintaining many bugfixes in Flink will add > >> additional burdens for upgrading Calcite. By adding corresponding > >> issue numbers, we can easily make sure that we can remove these Flink > >> hosted bugfixes when we upgrade to a version that already contains the > >> fix. > >> > >> Feng Jin <jinfeng1...@gmail.com> 于2023年12月14日周四 19:30写道： > >> > > >> > Hi Timo > >> > Thanks for your reply. > >> > > >> > > 1) ArgumentNames annotation > >> > > >> > I'm sorry for my incorrect expression. argumentNames is a method of > >> > FunctionHints. We should introduce a new arguments method to replace this > >> > method and return Argument[]. > >> > I updated the FLIP doc about this part. > >> > > >> > > 2) Evolution of FunctionHint > >> > > >> > +1 define DataTypeHint as part of ArgumentHint. I'll update the FLIP doc. > >> > > >> > > 3) Semantical correctness > >> > > >> > I realized that I forgot to submit the latest modification of the FLIP > >> > document. Xuyang and I had a prior discussion before starting this > >> > discuss. > >> > Let's restrict it to supporting only one eval() function, which will > >> > simplify the overall design. > >> > > >> > Therefore, I also concur with not permitting overloaded named parameters. > >> > > >> > > >> > Best, > >> > Feng > >> > > >> > On Thu, Dec 14, 2023 at 6:15 PM Timo Walther <twal...@apache.org> wrote: > >> > > >> > > Hi Feng, > >> > > > >> > > thank you for proposing this FLIP. This nicely completes FLIP-65 which > >> > > is great for usability. > >> > > > >> > > I read the FLIP and have some feedback: > >> > > > >> > > > >> > > 1) ArgumentNames annotation > >> > > > >> > > > Deprecate the ArgumentNames annotation as it is not user-friendly > >> > > for > >> > > specifying argument names with optional configuration. > >> > > > >> > > Which annotation does the FLIP reference here? I cannot find it in the > >> > > Flink code base. > >> > > > >> > > 2) Evolution of FunctionHint > >> > > > >> > > Introducing @ArgumentHint makes a lot of sense to me. However, using it > >> > > within @FunctionHint looks complex, because there is both `input=` and > >> > > `arguments=`. Ideally, the @DataTypeHint can be defined inline as part > >> > > of the @ArgumentHint. It could even be the `value` such that > >> > > `@ArgumentHint(@DataTypeHint("INT"))` is valid on its own. > >> > > > >> > > We could deprecate `input=`. Or let both `input` and `arguments=` > >> > > coexist but never be defined at the same time. > >> > > > >> > > 3) Semantical correctness > >> > > > >> > > As you can see in the `TypeInference` class, named parameters are > >> > > prepared in the stack already. However, we need to watch out between > >> > > helpful explanation (see `InputTypeStrategy#getExpectedSignatures`) and > >> > > named parameters (see `TypeInference.Builder#namedArguments`) that can > >> > > be used in SQL. > >> > > > >> > > If I remember correctly, named parameters can be reordered and don't > >> > > allow overloading of signatures. Thus, only a single eval() should have > >> > > named parameters. Looking at the FLIP it seems you would like to > >> > > support > >> > > multiple parameter lists. What changes are you planning to > >> > > TypeInference > >> > > (which is also declared as @PublicEvoving)? This should also be > >> > > documented as the annotations should compile into this class. > >> > > > >> > > In general, I would prefer to keep it simple and don't allow > >> > > overloading > >> > > named parameters. With the optionality, users can add an arbitrary > >> > > number of parameters to the signature of the same eval method. > >> > > > >> > > Regards, > >> > > Timo > >> > > > >> > > On 14.12.23 10:02, Feng Jin wrote: > >> > > > Hi all, > >> > > > > >> > > > > >> > > > Xuyang and I would like to start a discussion of FLIP-387: Support > >> > > > named parameters for functions and call procedures [1] > >> > > > > >> > > > Currently, when users call a function or call a procedure, they must > >> > > > specify all fields in order. When there are a large number of > >> > > > parameters, it is easy to make mistakes and cannot omit specifying > >> > > > non-mandatory fields. > >> > > > > >> > > > By using named parameters, you can selectively specify the required > >> > > > parameters, reducing the probability of errors and making it more > >> > > > convenient to use. > >> > > > > >> > > > Here is an example of using Named Procedure. > >> > > > ``` > >> > > > -- for scalar function > >> > > > SELECT my_scalar_function(param1 => ‘value1’, param2 => ‘value2’’) > >> > > > FROM > >> > > [] > >> > > > > >> > > > -- for table function > >> > > > SELECT * FROM TABLE(my_table_function(param1 => 'value1', param2 => > >> > > 'value2')) > >> > > > > >> > > > -- for agg function > >> > > > SELECT my_agg_function(param1 => 'value1', param2 => 'value2') FROM > >> > > > [] > >> > > > > >> > > > -- for call procedure > >> > > > CALL procedure_name(param1 => ‘value1’, param2 => ‘value2’) > >> > > > ``` > >> > > > > >> > > > For UDX and Call procedure developers, we introduce a new annotation > >> > > > to specify the parameter name, indicate if it is optional, and > >> > > > potentially support specifying default values in the future > >> > > > > >> > > > ``` > >> > > > public @interface ArgumentHint { > >> > > > /** > >> > > > * The name of the parameter, default is an empty string. > >> > > > */ > >> > > > String name() default ""; > >> > > > > >> > > > /** > >> > > > * Whether the parameter is optional, default is true. > >> > > > */ > >> > > > boolean isOptional() default true; > >> > > > }} > >> > > > ``` > >> > > > > >> > > > ``` > >> > > > // Call Procedure Development > >> > > > > >> > > > public static class NamedArgumentsProcedure implements Procedure { > >> > > > > >> > > > // Example usage: CALL myNamedProcedure(in1 => 'value1', in2 => > >> > > 'value2') > >> > > > > >> > > > // Example usage: CALL myNamedProcedure(in1 => 'value1', in2 => > >> > > > 'value2', in3 => 'value3') > >> > > > > >> > > > @ProcedureHint( > >> > > > input = {@DataTypeHint(value = "STRING"), > >> > > > @DataTypeHint(value = "STRING"), @DataTypeHint(value = "STRING")}, > >> > > > output = @DataTypeHint("STRING"), > >> > > > arguments = { > >> > > > @ArgumentHint(name = "in1", isOptional = false), > >> > > > @ArgumentHint(name = "in2", isOptional = true) > >> > > > @ArgumentHint(name = "in3", isOptional = true)}) > >> > > > public String[] call(ProcedureContext procedureContext, String > >> > > > arg1, String arg2, String arg3) { > >> > > > return new String[]{arg1 + ", " + arg2 + "," + arg3}; > >> > > > } > >> > > > } > >> > > > ``` > >> > > > > >> > > > > >> > > > Currently, we offer support for two scenarios when calling a function > >> > > > or procedure: > >> > > > > >> > > > 1. The corresponding parameters can be specified using the parameter > >> > > > name, without a specific order. > >> > > > 2. Unnecessary parameters can be omitted. > >> > > > > >> > > > > >> > > > There are still some limitations when using Named parameters: > >> > > > 1. Named parameters do not support variable arguments. > >> > > > 2. UDX or procedure classes that support named parameters can only > >> > > > have one eval method. > >> > > > 3. Due to the current limitations of Calcite-947[2], we cannot > >> > > > specify > >> > > > a default value for omitted parameters, which is Null by default. > >> > > > > >> > > > > >> > > > > >> > > > Also, thanks very much for the suggestions and help provided by Zelin > >> > > > and Lincoln. > >> > > > > >> > > > > >> > > > > >> > > > > >> > > > 1. > >> > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-387%3A+Support+named+parameters+for+functions+and+call+procedures > >> > > . > >> > > > > >> > > > 2. https://issues.apache.org/jira/browse/CALCITE-947 > >> > > > > >> > > > > >> > > > > >> > > > Best, > >> > > > > >> > > > Feng > >> > > > > >> > > > >> > > > >> > >> > >> > >> -- > >> > >> Best, > >> Benchao Li > > > > > > > >-- > > > >Best, > >Benchao Li -- Best, Benchao Li
