Re: [PR] [FLINK-34055][table] Introduce a new annotation for named parameters [flink]
fsk119 merged PR #24106: URL: https://github.com/apache/flink/pull/24106 -- 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
Re: [PR] [FLINK-34055][table] Introduce a new annotation for named parameters [flink]
fsk119 commented on code in PR #24106:
URL: https://github.com/apache/flink/pull/24106#discussion_r1466079035
##
flink-table/flink-table-common/src/test/java/org/apache/flink/table/types/extraction/TypeInferenceExtractorTest.java:
##
@@ -1585,4 +1659,143 @@ public void eval(CompletableFuture f, int[] i) {}
private static class DataTypeHintOnScalarFunctionAsync extends
AsyncScalarFunction {
public void eval(@DataTypeHint("ROW")
CompletableFuture f) {}
}
+
+private static class ArgumentHintScalarFunction extends ScalarFunction {
+@FunctionHint(
+argument = {
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "f1"),
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "f2")
+})
+public String eval(String f1, int f2) {
+return "";
+}
+}
+
+private static class ArgumentHintMissingTypeScalarFunction extends
ScalarFunction {
+@FunctionHint(argument = {@ArgumentHint(name = "f1"),
@ArgumentHint(name = "f2")})
+public String eval(String f1, int f2) {
+return "";
+}
+}
+
+private static class ArgumentHintMissingNameScalarFunction extends
ScalarFunction {
+@FunctionHint(
+argument = {
+@ArgumentHint(type = @DataTypeHint("STRING")),
+@ArgumentHint(type = @DataTypeHint("INTEGER"))
+})
+public String eval(String f1, int f2) {
+return "";
+}
+}
+
+private static class ArgumentHintNameConflictScalarFunction extends
ScalarFunction {
+@FunctionHint(
+argument = {
+@ArgumentHint(name = "in1", type =
@DataTypeHint("STRING")),
+@ArgumentHint(name = "in1", type =
@DataTypeHint("INTEGER"))
+})
+public String eval(String f1, int f2) {
+return "";
+}
+}
+
+private static class ArgumentHintOnParameterScalarFunction extends
ScalarFunction {
+public String eval(
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "in1")
String f1,
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "in2")
int f2) {
+return "";
+}
+}
+
+private static class ArgumentsAndInputsScalarFunction extends
ScalarFunction {
+@FunctionHint(
+argument = {
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "f1"),
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "f2")
+},
+input = {@DataTypeHint("STRING"), @DataTypeHint("INTEGER")})
+public String eval(String f1, int f2) {
+return "";
+}
+}
+
+private static class ArgumentsHintAndDataTypeHintScalarFunction extends
ScalarFunction {
+
+public String eval(
+@DataTypeHint("STRING") @ArgumentHint(name = "f1", type =
@DataTypeHint("STRING"))
+String f1,
+@ArgumentHint(name = "f2", type = @DataTypeHint("INTEGER"))
int f2) {
+return "";
+}
+}
+
+@FunctionHint(
+argument = {
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "f1"),
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "f2")
+})
+private static class InvalidFunctionHintOnClassAndMethod extends
ScalarFunction {
+@FunctionHint(
+argument = {
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "f1"),
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "f2")
+},
+input = {@DataTypeHint("STRING"), @DataTypeHint("INTEGER")})
+public String eval(String f1, int f2) {
Review Comment:
@hackergin can you open a ticket as a future work.
--
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
Re: [PR] [FLINK-34055][table] Introduce a new annotation for named parameters [flink]
hackergin commented on code in PR #24106:
URL: https://github.com/apache/flink/pull/24106#discussion_r1466056607
##
flink-table/flink-table-common/src/test/java/org/apache/flink/table/types/extraction/TypeInferenceExtractorTest.java:
##
@@ -1585,4 +1659,143 @@ public void eval(CompletableFuture f, int[] i) {}
private static class DataTypeHintOnScalarFunctionAsync extends
AsyncScalarFunction {
public void eval(@DataTypeHint("ROW")
CompletableFuture f) {}
}
+
+private static class ArgumentHintScalarFunction extends ScalarFunction {
+@FunctionHint(
+argument = {
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "f1"),
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "f2")
+})
+public String eval(String f1, int f2) {
+return "";
+}
+}
+
+private static class ArgumentHintMissingTypeScalarFunction extends
ScalarFunction {
+@FunctionHint(argument = {@ArgumentHint(name = "f1"),
@ArgumentHint(name = "f2")})
+public String eval(String f1, int f2) {
+return "";
+}
+}
+
+private static class ArgumentHintMissingNameScalarFunction extends
ScalarFunction {
+@FunctionHint(
+argument = {
+@ArgumentHint(type = @DataTypeHint("STRING")),
+@ArgumentHint(type = @DataTypeHint("INTEGER"))
+})
+public String eval(String f1, int f2) {
+return "";
+}
+}
+
+private static class ArgumentHintNameConflictScalarFunction extends
ScalarFunction {
+@FunctionHint(
+argument = {
+@ArgumentHint(name = "in1", type =
@DataTypeHint("STRING")),
+@ArgumentHint(name = "in1", type =
@DataTypeHint("INTEGER"))
+})
+public String eval(String f1, int f2) {
+return "";
+}
+}
+
+private static class ArgumentHintOnParameterScalarFunction extends
ScalarFunction {
+public String eval(
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "in1")
String f1,
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "in2")
int f2) {
+return "";
+}
+}
+
+private static class ArgumentsAndInputsScalarFunction extends
ScalarFunction {
+@FunctionHint(
+argument = {
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "f1"),
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "f2")
+},
+input = {@DataTypeHint("STRING"), @DataTypeHint("INTEGER")})
+public String eval(String f1, int f2) {
+return "";
+}
+}
+
+private static class ArgumentsHintAndDataTypeHintScalarFunction extends
ScalarFunction {
+
+public String eval(
+@DataTypeHint("STRING") @ArgumentHint(name = "f1", type =
@DataTypeHint("STRING"))
+String f1,
+@ArgumentHint(name = "f2", type = @DataTypeHint("INTEGER"))
int f2) {
+return "";
+}
+}
+
+@FunctionHint(
+argument = {
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "f1"),
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "f2")
+})
+private static class InvalidFunctionHintOnClassAndMethod extends
ScalarFunction {
+@FunctionHint(
+argument = {
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "f1"),
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "f2")
+},
+input = {@DataTypeHint("STRING"), @DataTypeHint("INTEGER")})
+public String eval(String f1, int f2) {
Review Comment:
We can limit this when implementing optional arguments, currently there is
no process to extract the optionals.
--
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
Re: [PR] [FLINK-34055][table] Introduce a new annotation for named parameters [flink]
hackergin commented on code in PR #24106:
URL: https://github.com/apache/flink/pull/24106#discussion_r1466054255
##
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/functions/inference/TypeInferenceOperandChecker.java:
##
@@ -114,6 +119,40 @@ public boolean isOptional(int i) {
return false;
Review Comment:
Yes, supporting optional arguments will modify this part.
--
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
Re: [PR] [FLINK-34055][table] Introduce a new annotation for named parameters [flink]
hackergin commented on code in PR #24106:
URL: https://github.com/apache/flink/pull/24106#discussion_r1466053011
##
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/extraction/FunctionTemplate.java:
##
@@ -173,18 +183,47 @@ private static T defaultAsNull(
private static @Nullable FunctionSignatureTemplate createSignatureTemplate(
DataTypeFactory typeFactory,
-@Nullable DataTypeHint[] input,
+@Nullable DataTypeHint[] inputs,
@Nullable String[] argumentNames,
+@Nullable ArgumentHint[] argumentHints,
boolean isVarArg) {
-if (input == null) {
+
+String[] argumentHintNames;
+DataTypeHint[] argumentHintTypes;
+
+if (argumentHints != null && inputs != null) {
+throw extractionError(
+"Argument and input hints cannot be declared in the same
function hint.");
+}
+
+if (argumentHints != null) {
+argumentHintNames = new String[argumentHints.length];
+argumentHintTypes = new DataTypeHint[argumentHints.length];
+for (int i = 0; i < argumentHints.length; i++) {
+ArgumentHint argumentHint = argumentHints[i];
+argumentHintNames[i] = defaultAsNull(argumentHint,
ArgumentHint::name);
+argumentHintTypes[i] = defaultAsNull(argumentHint,
ArgumentHint::type);
+if (argumentHintNames[i] == null || argumentHintTypes[i] ==
null) {
+throw extractionError(
Review Comment:
Updated, the current logic is that the argument name must be either all set
or all not set.
--
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
Re: [PR] [FLINK-34055][table] Introduce a new annotation for named parameters [flink]
fsk119 commented on code in PR #24106:
URL: https://github.com/apache/flink/pull/24106#discussion_r1465797504
##
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/extraction/FunctionTemplate.java:
##
@@ -63,11 +64,13 @@ private FunctionTemplate(
* types.
*/
static FunctionTemplate fromAnnotation(DataTypeFactory typeFactory,
FunctionHint hint) {
+
Review Comment:
nit: revert this empty line
##
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/extraction/FunctionTemplate.java:
##
@@ -78,11 +81,13 @@ static FunctionTemplate fromAnnotation(DataTypeFactory
typeFactory, FunctionHint
* data types.
*/
static FunctionTemplate fromAnnotation(DataTypeFactory typeFactory,
ProcedureHint hint) {
+
Review Comment:
remove this empty line.
##
flink-table/flink-table-common/src/test/java/org/apache/flink/table/types/extraction/TypeInferenceExtractorTest.java:
##
@@ -1585,4 +1659,143 @@ public void eval(CompletableFuture f, int[] i) {}
private static class DataTypeHintOnScalarFunctionAsync extends
AsyncScalarFunction {
public void eval(@DataTypeHint("ROW")
CompletableFuture f) {}
}
+
+private static class ArgumentHintScalarFunction extends ScalarFunction {
+@FunctionHint(
+argument = {
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "f1"),
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "f2")
+})
+public String eval(String f1, int f2) {
+return "";
+}
+}
+
+private static class ArgumentHintMissingTypeScalarFunction extends
ScalarFunction {
+@FunctionHint(argument = {@ArgumentHint(name = "f1"),
@ArgumentHint(name = "f2")})
+public String eval(String f1, int f2) {
+return "";
+}
+}
+
+private static class ArgumentHintMissingNameScalarFunction extends
ScalarFunction {
+@FunctionHint(
+argument = {
+@ArgumentHint(type = @DataTypeHint("STRING")),
+@ArgumentHint(type = @DataTypeHint("INTEGER"))
+})
+public String eval(String f1, int f2) {
+return "";
+}
+}
+
+private static class ArgumentHintNameConflictScalarFunction extends
ScalarFunction {
+@FunctionHint(
+argument = {
+@ArgumentHint(name = "in1", type =
@DataTypeHint("STRING")),
+@ArgumentHint(name = "in1", type =
@DataTypeHint("INTEGER"))
+})
+public String eval(String f1, int f2) {
+return "";
+}
+}
+
+private static class ArgumentHintOnParameterScalarFunction extends
ScalarFunction {
+public String eval(
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "in1")
String f1,
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "in2")
int f2) {
+return "";
+}
+}
+
+private static class ArgumentsAndInputsScalarFunction extends
ScalarFunction {
+@FunctionHint(
+argument = {
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "f1"),
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "f2")
+},
+input = {@DataTypeHint("STRING"), @DataTypeHint("INTEGER")})
+public String eval(String f1, int f2) {
+return "";
+}
+}
+
+private static class ArgumentsHintAndDataTypeHintScalarFunction extends
ScalarFunction {
+
+public String eval(
+@DataTypeHint("STRING") @ArgumentHint(name = "f1", type =
@DataTypeHint("STRING"))
+String f1,
+@ArgumentHint(name = "f2", type = @DataTypeHint("INTEGER"))
int f2) {
+return "";
+}
+}
+
+@FunctionHint(
+argument = {
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "f1"),
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "f2")
+})
+private static class InvalidFunctionHintOnClassAndMethod extends
ScalarFunction {
+@FunctionHint(
+argument = {
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "f1"),
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "f2")
+},
+input = {@DataTypeHint("STRING"), @DataTypeHint("INTEGER")})
+public String eval(String f1, int f2) {
Review Comment:
BTW, if argument's DataType is not null, user can not specify optional =
true.
##
flink-table/flink-table-common/src/test/java/org/apache/flink/table/types/extraction/TypeInferenceExtractorTest.java:
##
@@ -1585,4 +1659,143 @@ public void eval(CompletableFuture f, int[] i) {}
private static class
Re: [PR] [FLINK-34055][table] Introduce a new annotation for named parameters [flink]
hackergin commented on code in PR #24106:
URL: https://github.com/apache/flink/pull/24106#discussion_r1465421196
##
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/functions/inference/TypeInferenceOperandChecker.java:
##
@@ -114,6 +117,24 @@ public boolean isOptional(int i) {
return false;
}
+@Override
+public List paramTypes(RelDataTypeFactory typeFactory) {
+throw new IllegalStateException("Should not be called");
+}
+
+@Override
+public List paramNames() {
+return typeInference
Review Comment:
The current limitation is that all functions or procedures with overloaded
functions do not support named arguments, it is not because the function
signature does not meet the requirements. I think that it may not be possible
to print the function signature here.
--
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
Re: [PR] [FLINK-34055][table] Introduce a new annotation for named parameters [flink]
hackergin commented on code in PR #24106:
URL: https://github.com/apache/flink/pull/24106#discussion_r1465418253
##
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/functions/inference/TypeInferenceOperandChecker.java:
##
@@ -114,6 +117,24 @@ public boolean isOptional(int i) {
return false;
}
+@Override
+public List paramTypes(RelDataTypeFactory typeFactory) {
+throw new IllegalStateException("Should not be called");
Review Comment:
The current usage does not involve this method, but the optional argument
will need to be implemented. I have already made modifications to this part.
--
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
Re: [PR] [FLINK-34055][table] Introduce a new annotation for named parameters [flink]
hackergin commented on code in PR #24106:
URL: https://github.com/apache/flink/pull/24106#discussion_r1465417066
##
flink-table/flink-table-planner/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##
@@ -3730,7 +3733,11 @@ private void checkRollUp(
// can be another SqlCall, or an SqlIdentifier.
checkRollUp(grandParent, parent, stripDot, scope,
contextClause);
} else {
-List children = ((SqlCall)
stripDot).getOperandList();
+// - FLINK MODIFICATION BEGIN -
+SqlCall call = (SqlCall) stripDot;
+List children =
+new SqlCallBinding(this, scope, call).operands();
Review Comment:
I'll create a calcite jira later.
--
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
Re: [PR] [FLINK-34055][table] Introduce a new annotation for named parameters [flink]
hackergin commented on code in PR #24106:
URL: https://github.com/apache/flink/pull/24106#discussion_r1465416608
##
flink-table/flink-table-common/src/test/java/org/apache/flink/table/types/extraction/TypeInferenceExtractorTest.java:
##
@@ -1585,4 +1629,106 @@ public void eval(CompletableFuture f, int[] i) {}
private static class DataTypeHintOnScalarFunctionAsync extends
AsyncScalarFunction {
public void eval(@DataTypeHint("ROW")
CompletableFuture f) {}
}
+
+private static class ArgumentHintScalarFunction extends ScalarFunction {
+@FunctionHint(
+arguments = {
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "f1"),
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "f2")
+})
+public String eval(String f1, int f2) {
+return "";
+}
+}
+
+private static class ArgumentsAndInputsScalarFunction extends
ScalarFunction {
+@FunctionHint(
+arguments = {
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "f1"),
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "f2")
+},
+input = {@DataTypeHint("STRING"), @DataTypeHint("INTEGER")})
+public String eval(String f1, int f2) {
+return "";
+}
+}
+
+private static class ArgumentsHintAndDataTypeHintScalarFunction extends
ScalarFunction {
+
+public String eval(
+@DataTypeHint("STRING") @ArgumentHint(name = "f1", type =
@DataTypeHint("STRING"))
+String f1,
+@ArgumentHint(name = "f2", type = @DataTypeHint("INTEGER"))
int f2) {
+return "";
+}
+}
+
+@FunctionHint(
+arguments = {
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "f1"),
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "f2")
+})
+private static class InvalidFunctionHintOnClassAndMethod extends
ScalarFunction {
+@FunctionHint(
+arguments = {
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "f1"),
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "f2")
+},
+input = {@DataTypeHint("STRING"), @DataTypeHint("INTEGER")})
+public String eval(String f1, int f2) {
+return "";
+}
+}
+
+@FunctionHint(
+arguments = {
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "f1"),
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "f2")
+})
+private static class ValidFunctionHintOnClassAndMethod extends
ScalarFunction {
+@FunctionHint(
+arguments = {
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "f1"),
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "f2")
+})
+public String eval(String f1, int f2) {
+return "";
+}
+}
+
+@FunctionHint(
+arguments = {
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "f1"),
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "f2")
+})
+@FunctionHint(
+arguments = {
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "f1"),
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "f2")
+})
+private static class ScalarFunctionWithFunctionHintConflictMethod extends
ScalarFunction {
+public String eval(String f1, int f2) {
+return "";
+}
+}
+
+private static class ArgumentsHintScalarFunctionWithOverloadedFunction
extends ScalarFunction {
+@FunctionHint(
+arguments = {
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "f1"),
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "f2")
+})
+public String eval(String f1, int f2) {
+return "";
+}
+
+@FunctionHint(
+arguments = {
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "f1"),
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "f2")
+})
+public String eval(String f1, String f2) {
Review Comment:
Yes, I thinks we can supported this. Updated.
--
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
Re: [PR] [FLINK-34055][table] Introduce a new annotation for named parameters [flink]
hackergin commented on code in PR #24106:
URL: https://github.com/apache/flink/pull/24106#discussion_r1465416116
##
flink-table/flink-table-common/src/test/java/org/apache/flink/table/types/extraction/TypeInferenceExtractorTest.java:
##
@@ -1585,4 +1629,106 @@ public void eval(CompletableFuture f, int[] i) {}
private static class DataTypeHintOnScalarFunctionAsync extends
AsyncScalarFunction {
public void eval(@DataTypeHint("ROW")
CompletableFuture f) {}
}
+
+private static class ArgumentHintScalarFunction extends ScalarFunction {
Review Comment:
I added a conflict check.
--
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
Re: [PR] [FLINK-34055][table] Introduce a new annotation for named parameters [flink]
hackergin commented on code in PR #24106:
URL: https://github.com/apache/flink/pull/24106#discussion_r1465413612
##
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/extraction/BaseMappingExtractor.java:
##
@@ -362,7 +363,17 @@ static Optional
tryExtractInputGroupArgument(
Method method, int paramPos) {
final Parameter parameter = method.getParameters()[paramPos];
final DataTypeHint hint = parameter.getAnnotation(DataTypeHint.class);
-if (hint != null) {
+final ArgumentHint argumentHint =
parameter.getAnnotation(ArgumentHint.class);
+if (hint != null && argumentHint != null) {
+throw extractionError(
+"ArgumentHint and DataTypeHint cannot be declared at the
same time.");
Review Comment:
parameters.getName can't get the real param name. I use position index
instread.
--
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
Re: [PR] [FLINK-34055][table] Introduce a new annotation for named parameters [flink]
hackergin commented on code in PR #24106:
URL: https://github.com/apache/flink/pull/24106#discussion_r1465412817
##
flink-table/flink-table-common/src/main/java/org/apache/flink/table/annotation/FunctionHint.java:
##
@@ -148,6 +148,18 @@
*/
String[] argumentNames() default {""};
+/**
+ * Explicitly lists the argument that a function takes as input, including
their names, types,
+ * and whether they are optional.
+ *
+ * By default, it is recommended to use this parameter instead of
{@link #input()}. If the
+ * type of argumentHint is not defined, it will be considered an invalid
argument and an
+ * exception will be thrown. Additionally, both this parameter and {@link
#input()} cannot be
+ * defined at the same time. If neither arguments nor {@link #input()} are
defined,
+ * reflection-based extraction will be used.
+ */
+ArgumentHint[] arguments() default {};
Review Comment:
Changed to `argument`
--
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
Re: [PR] [FLINK-34055][table] Introduce a new annotation for named parameters [flink]
fsk119 commented on code in PR #24106:
URL: https://github.com/apache/flink/pull/24106#discussion_r1464215746
##
flink-table/flink-table-common/src/main/java/org/apache/flink/table/annotation/ProcedureHint.java:
##
@@ -139,6 +139,18 @@
*/
String[] argumentNames() default {""};
+/**
+ * Explicitly lists the argument that a procedure takes as input,
including their names, types,
+ * and whether they are optional.
+ *
+ * By default, it is recommended to use this parameter instead of
{@link #input()}. If the
+ * type of argumentHint is not defined, it will be considered an invalid
argument and an
+ * exception will be thrown. Additionally, both this parameter and {@link
#input()} cannot be
+ * defined at the same time. If neither arguments nor {@link #input()} are
defined,
+ * reflection-based extraction will be used.
+ */
+ArgumentHint[] arguments() default {};
Review Comment:
ditto
##
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/extraction/BaseMappingExtractor.java:
##
@@ -362,7 +363,17 @@ static Optional
tryExtractInputGroupArgument(
Method method, int paramPos) {
final Parameter parameter = method.getParameters()[paramPos];
final DataTypeHint hint = parameter.getAnnotation(DataTypeHint.class);
-if (hint != null) {
+final ArgumentHint argumentHint =
parameter.getAnnotation(ArgumentHint.class);
+if (hint != null && argumentHint != null) {
+throw extractionError(
+"ArgumentHint and DataTypeHint cannot be declared at the
same time.");
Review Comment:
nit: it's better to notify user which parameter is not correct.
```
throw extractionError(
String.format(
"ArgumentHint and DataTypeHint cannot be
declared at the same time for parameter '%s'.",
parameter.getName()));
```
##
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/functions/inference/TypeInferenceOperandChecker.java:
##
@@ -114,6 +117,24 @@ public boolean isOptional(int i) {
return false;
}
+@Override
+public List paramTypes(RelDataTypeFactory typeFactory) {
+throw new IllegalStateException("Should not be called");
+}
+
+@Override
+public List paramNames() {
+return typeInference
Review Comment:
Add more details exception. Print current required method method signature
and the candidate method signatures.
BTW, do you mean user can not use like this?
```
public static class NamedArgumentsScalarFunction extends ScalarFunction {
@FunctionHint(
output = @DataTypeHint("STRING"),
arguments = {
@ArgumentHint(name = "in1", type = @DataTypeHint("int")),
@ArgumentHint(name = "in2", type = @DataTypeHint("int"),
isOptional = true)
})
public String eval(Integer arg1, Integer arg2) {
return (arg1 + ": " + arg2);
}
@FunctionHint(
output = @DataTypeHint("STRING"),
arguments = {
@ArgumentHint(name = "in1", type =
@DataTypeHint("int")),
@ArgumentHint(name = "in2", type =
@DataTypeHint("int"), isOptional = true),
@ArgumentHint(name = "in3", type =
@DataTypeHint("int"), isOptional = true)
})
public String eval(Integer arg1, Integer arg2, Integer arg3) {
return (arg1 + ": " + arg2);
}
}
```
```
SELECT udf(in1 => 1);
```
##
flink-table/flink-table-common/src/test/java/org/apache/flink/table/types/extraction/TypeInferenceExtractorTest.java:
##
@@ -1585,4 +1629,106 @@ public void eval(CompletableFuture f, int[] i) {}
private static class DataTypeHintOnScalarFunctionAsync extends
AsyncScalarFunction {
public void eval(@DataTypeHint("ROW")
CompletableFuture f) {}
}
+
+private static class ArgumentHintScalarFunction extends ScalarFunction {
+@FunctionHint(
+arguments = {
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "f1"),
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "f2")
+})
+public String eval(String f1, int f2) {
+return "";
+}
+}
+
+private static class ArgumentsAndInputsScalarFunction extends
ScalarFunction {
+@FunctionHint(
+arguments = {
+@ArgumentHint(type = @DataTypeHint("STRING"), name = "f1"),
+@ArgumentHint(type = @DataTypeHint("INTEGER"), name = "f2")
+},
+input = {@DataTypeHint("STRING"),
Re: [PR] [FLINK-34055][table] Introduce a new annotation for named parameters [flink]
hackergin commented on code in PR #24106:
URL: https://github.com/apache/flink/pull/24106#discussion_r1464193652
##
flink-table/flink-table-common/src/test/java/org/apache/flink/table/types/extraction/TypeInferenceExtractorTest.java:
##
@@ -427,8 +428,7 @@ private static Stream functionSpecs() {
"Could not find a publicly accessible method
named 'eval'."),
// named arguments with overloaded function
-TestSpec.forScalarFunction(NamedArgumentsScalarFunction.class)
-.expectNamedArguments("n"),
+TestSpec.forScalarFunction(NamedArgumentsScalarFunction.class),
Review Comment:
I have added relevant comments. We expect namedArgument to be null, so there
is no expectXXX here.
--
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
Re: [PR] [FLINK-34055][table] Introduce a new annotation for named parameters [flink]
xuyangzhong commented on PR #24106: URL: https://github.com/apache/flink/pull/24106#issuecomment-1905675166 cc @fsk119 for the double check. -- 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
Re: [PR] [FLINK-34055][table] Introduce a new annotation for named parameters [flink]
xuyangzhong commented on code in PR #24106:
URL: https://github.com/apache/flink/pull/24106#discussion_r1462969824
##
flink-table/flink-table-common/src/test/java/org/apache/flink/table/types/extraction/TypeInferenceExtractorTest.java:
##
@@ -535,7 +535,43 @@ private static Stream functionSpecs() {
new String[] {}, new
ArgumentTypeStrategy[] {}),
TypeStrategies.explicit(
DataTypes.ROW(DataTypes.FIELD("i",
DataTypes.INT()))
-.bridgedTo(RowData.class;
+.bridgedTo(RowData.class))),
+TestSpec.forScalarFunction(
+"Scalar function with arguments hints",
+ArgumentHintScalarFunction.class)
+.expectNamedArguments("f1", "f2")
+.expectTypedArguments(DataTypes.STRING(),
DataTypes.INT())
+.expectOutputMapping(
+InputTypeStrategies.sequence(
+new String[] {"f1", "f2"},
+new ArgumentTypeStrategy[] {
+
InputTypeStrategies.explicit(DataTypes.STRING()),
+
InputTypeStrategies.explicit(DataTypes.INT())
+}),
+TypeStrategies.explicit(DataTypes.STRING())),
+TestSpec.forScalarFunction(
+"Scalar function with arguments hints and
inputs hints both defined",
+ArgumentsAndInputsScalarFunction.class)
+.expectErrorMessage(
+"Unable to support specifying both inputs and
arguments at the same time"),
+TestSpec.forScalarFunction(
+"Scalar function with ArgumentHint and
DataTypeHint hints both defined",
+
ArgumentsHintAndDataTypeHintScalarFunction.class)
+.expectErrorMessage(
+"ArgumentHint and DataTypeHint cannot be
declared at the same time."),
+TestSpec.forScalarFunction(
+"Scalar function with FunctionHint on class
and method",
+FunctionHintOnClassAndMethod.class)
+.expectErrorMessage(
+"Unable to support specifying both inputs and
arguments at the same time."),
+TestSpec.forScalarFunction(
+"Scalar function with FunctionHint on class
and method",
+MultiFunctionHintOnClass.class)
+.expectErrorMessage(
+"Considering all hints, the method should
comply with the signature"),
+TestSpec.forScalarFunction(
Review Comment:
ditto
##
flink-table/flink-table-common/src/test/java/org/apache/flink/table/types/extraction/TypeInferenceExtractorTest.java:
##
@@ -535,7 +535,43 @@ private static Stream functionSpecs() {
new String[] {}, new
ArgumentTypeStrategy[] {}),
TypeStrategies.explicit(
DataTypes.ROW(DataTypes.FIELD("i",
DataTypes.INT()))
-.bridgedTo(RowData.class;
+.bridgedTo(RowData.class))),
+TestSpec.forScalarFunction(
+"Scalar function with arguments hints",
+ArgumentHintScalarFunction.class)
+.expectNamedArguments("f1", "f2")
+.expectTypedArguments(DataTypes.STRING(),
DataTypes.INT())
+.expectOutputMapping(
+InputTypeStrategies.sequence(
+new String[] {"f1", "f2"},
+new ArgumentTypeStrategy[] {
+
InputTypeStrategies.explicit(DataTypes.STRING()),
+
InputTypeStrategies.explicit(DataTypes.INT())
+}),
+TypeStrategies.explicit(DataTypes.STRING())),
+TestSpec.forScalarFunction(
+"Scalar function with arguments hints and
inputs hints both defined",
+ArgumentsAndInputsScalarFunction.class)
+.expectErrorMessage(
+"Unable to support specifying both inputs
Re: [PR] [FLINK-34055][table] Introduce a new annotation for named parameters [flink]
hackergin commented on PR #24106: URL: https://github.com/apache/flink/pull/24106#issuecomment-1905479204 @xuyangzhong Thank you for the review. I have already updated the code. Please take a look when you have time. -- 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
Re: [PR] [FLINK-34055][table] Introduce a new annotation for named parameters [flink]
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.
+ *
+ * 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.
+ *
+ * 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.
+ *
+ * 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.
##
Re: [PR] [FLINK-34055][table] Introduce a new annotation for named parameters [flink]
flinkbot commented on PR #24106: URL: https://github.com/apache/flink/pull/24106#issuecomment-1894315428 ## CI report: * a248bc76db79b94730b2107e05572f6874f7eb95 UNKNOWN Bot commands The @flinkbot bot supports the following commands: - `@flinkbot run azure` re-run the last Azure build -- 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
[PR] [FLINK-34055][table] Introduce a new annotation for named parameters [flink]
hackergin opened a new pull request, #24106: URL: https://github.com/apache/flink/pull/24106 ## What is the purpose of the change *1. Declare a new ArgumentHint to specify the name, type, and whether it is optional for an argument. *2. Support named parameters for functions. *3. Support named parameters for procedures. ## Brief change log - ntroduce a new ArgumentHint annotation - Support named parameters for functions - Support named parameters for producers ## Verifying this change test cases in FunctionITCase and ProcedureITCase to verify named parameters. ## Does this pull request potentially affect one of the following parts: - Dependencies (does it add or upgrade a dependency): (no) - The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (no) - The serializers: (no) - The runtime per-record code paths (performance sensitive): (no) - Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no) - The S3 file system connector: (no) ## Documentation - Does this pull request introduce a new feature? (yes) - If yes, how is the feature documented? (not documented, will be added in a separator pr) -- 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
