Re: [PR] [FLINK-34055][table] Introduce a new annotation for named parameters [flink]

2024-01-25 Thread via GitHub


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]

2024-01-25 Thread via GitHub


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]

2024-01-25 Thread via GitHub


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]

2024-01-25 Thread via GitHub


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]

2024-01-25 Thread via GitHub


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]

2024-01-24 Thread via GitHub


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 DataTyp

Re: [PR] [FLINK-34055][table] Introduce a new annotation for named parameters [flink]

2024-01-24 Thread via GitHub


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]

2024-01-24 Thread via GitHub


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]

2024-01-24 Thread via GitHub


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]

2024-01-24 Thread via GitHub


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]

2024-01-24 Thread via GitHub


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]

2024-01-24 Thread via GitHub


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]

2024-01-24 Thread via GitHub


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]

2024-01-23 Thread via GitHub


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"), @Dat

Re: [PR] [FLINK-34055][table] Introduce a new annotation for named parameters [flink]

2024-01-23 Thread via GitHub


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]

2024-01-23 Thread via GitHub


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]

2024-01-23 Thread via GitHub


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 an

Re: [PR] [FLINK-34055][table] Introduce a new annotation for named parameters [flink]

2024-01-22 Thread via GitHub


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]

2024-01-22 Thread via GitHub


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.



##
flink-table/flink-table-planner/src/main/java/org/apache/calc

Re: [PR] [FLINK-34055][table] Introduce a new annotation for named parameters [flink]

2024-01-16 Thread via GitHub


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]

2024-01-16 Thread via GitHub


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