xuyangzhong commented on code in PR #24106:
URL: https://github.com/apache/flink/pull/24106#discussion_r1461382300
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/annotation/ProcedureHint.java:
##########
@@ -139,6 +139,17 @@
*/
String[] argumentNames() default {""};
+ /**
+ * Explicitly lists the argument that a procedure takes as input,
including their names, types,
+ * and whether they are optional.
+ *
+ * <p>By default, argumentHint takes precedence over {@link #input()}. If
the type of argument
+ * is not defined in this method, the type specified in {@link #input()}
will be used instead.
+ * If {@link #input()} is also not defined, reflection-based extraction
will be used and this
+ * parameter will be ignored.
+ */
+ ArgumentHint[] arguments() default @ArgumentHint;
Review Comment:
ditto
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/annotation/FunctionHint.java:
##########
@@ -148,6 +148,17 @@
*/
String[] argumentNames() default {""};
+ /**
+ * Explicitly lists the argument that a function takes as input, including
their names, types,
+ * and whether they are optional.
+ *
+ * <p>By default, argumentHint takes precedence over {@link #input()}. If
the type of argument
+ * is not defined in this method, the type specified in {@link #input()}
will be used instead.
+ * If {@link #input()} is also not defined, reflection-based extraction
will be used and this
+ * parameter will be ignored.
+ */
+ ArgumentHint[] arguments() default {@ArgumentHint};
Review Comment:
Should we use `{}` as the default value?
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/annotation/ProcedureHint.java:
##########
@@ -139,6 +139,17 @@
*/
String[] argumentNames() default {""};
+ /**
+ * Explicitly lists the argument that a procedure takes as input,
including their names, types,
+ * and whether they are optional.
+ *
+ * <p>By default, argumentHint takes precedence over {@link #input()}. If
the type of argument
+ * is not defined in this method, the type specified in {@link #input()}
will be used instead.
+ * If {@link #input()} is also not defined, reflection-based extraction
will be used and this
+ * parameter will be ignored.
+ */
+ ArgumentHint[] arguments() default @ArgumentHint;
Review Comment:
Add changes to ProcedureHint back in the public interface in flip.
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/operations/SqlNodeToCallOperationTest.java:
##########
@@ -194,6 +206,24 @@ public int[] call(ProcedureContext context, int arg1, long
arg2) {
}
}
+ private static class ProcedureWithNamedArguments implements Procedure {
+ @ProcedureHint(
+ input = {@DataTypeHint("INT"), @DataTypeHint("BIGINT")},
+ output = @DataTypeHint("INT"),
+ argumentNames = {"c", "d"})
+ public int[] call(ProcedureContext context, int arg1, long arg2) {
Review Comment:
ditto, multi `call`.
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/runtime/stream/sql/ProcedureITCase.java:
##########
@@ -172,6 +175,24 @@ void testCallProcedure() {
Column.physical("age",
DataTypes.INT().notNull().bridgedTo(int.class))));
}
+ @Test
+ public void testNamedArguments() {
+ TableResult tableResult =
+ tEnv().executeSql("call `system`.named_args(d => 19, c =>
'yuxia')");
+ verifyTableResult(
+ tableResult,
+ Collections.singletonList(Row.of("yuxia, 19")),
+ ResolvedSchema.of(Column.physical("result",
DataTypes.STRING())));
+ }
+
+ @Test
+ public void testNamedArgumentsWithDefaultValue() {
+ // default value
+ Assertions.assertThatThrownBy(
+ () -> tEnv().executeSql("call `system`.named_args(c =>
'yuxia')"))
+ .isInstanceOf(ValidationException.class);
Review Comment:
It's better to also validate the error message here.
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/plan/nodes/logical/FlinkLogicalTableFunctionScan.scala:
##########
@@ -106,12 +106,15 @@ class FlinkLogicalTableFunctionScanConverter(config:
Config) extends ConverterRu
val scan = rel.asInstanceOf[LogicalTableFunctionScan]
val traitSet = rel.getTraitSet.replace(FlinkConventions.LOGICAL).simplify()
val newInputs = scan.getInputs.map(input => RelOptRule.convert(input,
FlinkConventions.LOGICAL))
+ val rexCall = scan.getCall.asInstanceOf[RexCall];
Review Comment:
It's better to add some comments here to let others know why we regenerate
the rex call here.
##########
flink-table/flink-table-planner/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##########
@@ -6056,7 +6056,8 @@ private void translateAgg(
try {
// switch out of agg mode
bb.agg = null;
- for (SqlNode operand : call.getOperandList()) {
+ for (SqlNode operand :
Review Comment:
ditto
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/annotation/ArgumentHint.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.table.annotation;
+
+import org.apache.flink.annotation.PublicEvolving;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+/**
+ * A hint that provides additional information about an argument.
+ *
+ * <p>An {@code ArgumentHint} can be used to provide hints about the name,
optionality, and data
+ * type of argument.
+ *
+ * <p>It combines the functionality of {@link FunctionHint#argumentNames()}
and {@link DataTypeHint}
+ * annotations to conveniently group argument-related information together in
function declarations.
+ *
+ * <p>{@code @ArgumentHint(name = "in1", type = @DataTypeHint("STRING"),
isOptional = false} an
+ * argument with the type String, named in1, and it is a required parameter.
+ */
+@PublicEvolving
+@Retention(RetentionPolicy.RUNTIME)
+@Target({ElementType.TYPE, ElementType.METHOD, ElementType.FIELD,
ElementType.PARAMETER})
+public @interface ArgumentHint {
+
+ /**
+ * The name of the argument.
+ *
+ * <p>This can be used to provide a descriptive name for the argument.
+ */
+ String name() default "";
+
+ /**
+ * Specifies whether the argument is optional or required.
+ *
+ * <p>If set to {@code true}, the argument is considered optional. If set
to {@code false}, the
Review Comment:
`If set to {@code true}, the argument is considered optional. And if the
user does not specify this parameter when calling, 'null' will be passed in.`
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/annotation/ArgumentHint.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.table.annotation;
+
+import org.apache.flink.annotation.PublicEvolving;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+/**
+ * A hint that provides additional information about an argument.
+ *
+ * <p>An {@code ArgumentHint} can be used to provide hints about the name,
optionality, and data
+ * type of argument.
+ *
+ * <p>It combines the functionality of {@link FunctionHint#argumentNames()}
and {@link DataTypeHint}
+ * annotations to conveniently group argument-related information together in
function declarations.
+ *
+ * <p>{@code @ArgumentHint(name = "in1", type = @DataTypeHint("STRING"),
isOptional = false} an
Review Comment:
missing `is` before 'an'.
`and it is a required parameter.` -> `cannot be omitted when calling.`
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/extraction/BaseMappingExtractor.java:
##########
@@ -362,11 +363,17 @@ static Optional<FunctionArgumentTemplate>
tryExtractInputGroupArgument(
Method method, int paramPos) {
final Parameter parameter = method.getParameters()[paramPos];
final DataTypeHint hint = parameter.getAnnotation(DataTypeHint.class);
+ final ArgumentHint argumentHint =
parameter.getAnnotation(ArgumentHint.class);
if (hint != null) {
final DataTypeTemplate template =
DataTypeTemplate.fromAnnotation(hint, null);
if (template.inputGroup != null) {
return
Optional.of(FunctionArgumentTemplate.of(template.inputGroup));
}
+ } else if (argumentHint != null) {
+ final DataTypeTemplate template =
DataTypeTemplate.fromAnnotation(argumentHint, null);
Review Comment:
These changes don't appear to have been tested.
And from the code, the priority of datatype hint (input) is greater than
argument hint, right? This seems to be inconsistent with the comments on the
interface
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/annotation/FunctionHint.java:
##########
@@ -148,6 +148,17 @@
*/
String[] argumentNames() default {""};
+ /**
+ * Explicitly lists the argument that a function takes as input, including
their names, types,
+ * and whether they are optional.
+ *
+ * <p>By default, argumentHint takes precedence over {@link #input()}. If
the type of argument
Review Comment:
Should we prohibit the use of arguments while the user is using input or
argumentNames just like what discussed in the maillist: `We could deprecate
'input='. Or let both 'input' and 'arguments='
coexist but never be defined at the same time.`
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/extraction/DataTypeTemplate.java:
##########
@@ -118,6 +119,22 @@ static DataTypeTemplate fromAnnotation(DataTypeFactory
typeFactory, DataTypeHint
* Creates an instance from the given {@link DataTypeHint} with a resolved
data type if
* available.
*/
+ static DataTypeTemplate fromAnnotation(ArgumentHint argumentHint,
@Nullable DataType dataType) {
+ DataTypeHint hint = argumentHint.type();
Review Comment:
`return fromAnnotation(argumentHint.type(), dataType);`
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/runtime/stream/sql/FunctionITCase.java:
##########
@@ -1509,6 +1618,25 @@ public void eval(Integer i) {
}
}
+ /** Function that returns a string or integer. */
+ public static class NamedArgumentsTableFunction extends
TableFunction<Object> {
+ @FunctionHint(
+ input = {@DataTypeHint("STRING"), @DataTypeHint("STRING")},
+ output = @DataTypeHint("STRING"),
+ argumentNames = {"in1", "in2"})
+ public void eval(String arg1, String arg2) {
+ collect(arg1 + ", " + arg2);
+ }
+
+ @FunctionHint(
+ input = {@DataTypeHint("INT"), @DataTypeHint("INT")},
+ output = @DataTypeHint("STRING"),
+ argumentNames = {"in1", "in2"})
+ public void eval(Integer arg1, Integer arg2) {
Review Comment:
From the flip, we should not support named argument for udx with multi
`eval`, right?
Ref -> Limitation: `the UDX or procedure class that support named
parameters can only have one eval method`
##########
flink-table/flink-table-planner/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -3730,7 +3730,9 @@ private void checkRollUp(
// can be another SqlCall, or an SqlIdentifier.
checkRollUp(grandParent, parent, stripDot, scope,
contextClause);
} else {
- List<? extends @Nullable SqlNode> children = ((SqlCall)
stripDot).getOperandList();
+ SqlCall call = (SqlCall) stripDot;
Review Comment:
Add comments in this class, and add `// ----- FLINK MODIFICATION BEGIN
-----` and `// ----- FLINK MODIFICATION END -----` here.
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/SqlProcedureCallConverter.java:
##########
@@ -133,9 +133,17 @@ private List<RexNode> reduceOperands(SqlCall sqlCall,
ConvertContext context) {
context.getSqlValidator().getTypeFactory());
List<RexNode> rexNodes = new ArrayList<>();
for (int i = 0; i < sqlCall.operandCount(); i++) {
- RexNode rexNode = context.toRexNode(sqlCall.operand(i),
inputRowType, null);
+ RexNode rexNode =
Review Comment:
At first I wanted to understand why this change was needed. When I reverted
this part of the change, I found that `ProcedureITCase#testNamedArguments`
could run, but `SqlNodeToCallOperationTest#testWithNamedArguments` did not.
When I dug deeper into the code, I found that another bug was in
TypeInferenceOperandChecker#insertImplicitCasts (Line 158).
`final LogicalType argumentType =
toLogicalType(callBinding.getOperandType(i));` should be replaced into
`final LogicalType argumentType =
toLogicalType(
SqlTypeUtil.deriveType(callBinding,
callBinding.operands().get(i)));`
You can re-produce this bug by `ProcedureITCase#testNamedArguments`.
Maybe we can unify the behavior of unwrapping more in advance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
