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