lsyldliu commented on code in PR #20176:
URL: https://github.com/apache/flink/pull/20176#discussion_r934454226
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/api/TableEnvironmentTest.scala:
##########
@@ -706,6 +714,40 @@ class TableEnvironmentTest {
assertFalse(tableEnv.listUserDefinedFunctions().contains("f2"))
}
+ @Test
+ def testExecuteSqlWithCreateFunctionWithResource(): Unit = {
Review Comment:
I think this test is no need, it has been covered by tests in
`FunctionITCase`.
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/catalog/FunctionCatalog.java:
##########
@@ -690,6 +684,50 @@ private void registerFunctionJarResources(String
functionName, List<ResourceUri>
}
}
+ private void registerCatalogFunction(
+ ObjectIdentifier identifier, CatalogFunction catalogFunction,
boolean ignoreIfExists) {
+ final ObjectIdentifier normalizedIdentifier =
+ FunctionIdentifier.normalizeObjectIdentifier(identifier);
+
+ final Catalog catalog =
+ catalogManager
+ .getCatalog(normalizedIdentifier.getCatalogName())
+ .orElseThrow(IllegalStateException::new);
Review Comment:
throw new ValidationException(String.format("Catalog %s not exists",
catalogName));
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/runtime/stream/sql/FunctionITCase.java:
##########
@@ -237,6 +239,16 @@ public void testCreateTemporarySystemFunctionByUsingJar() {
assertThat(Arrays.asList(tEnv().listFunctions())).doesNotContain("f10");
}
+ @Test
+ public void testCreateTemporarySystemFunctionWithTableAPI() {
Review Comment:
Please refer to the `testUserDefinedTemporarySystemFunctionByUsingJar`, also
add some tests that use UDF registered by table api.
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/catalog/FunctionCatalog.java:
##########
@@ -198,45 +207,30 @@ public void registerCatalogFunction(
t);
}
- final Catalog catalog =
- catalogManager
- .getCatalog(normalizedIdentifier.getCatalogName())
- .orElseThrow(IllegalStateException::new);
- final ObjectPath path = identifier.toObjectPath();
-
- // we force users to deal with temporary catalog functions first
- if (tempCatalogFunctions.containsKey(normalizedIdentifier)) {
- if (ignoreIfExists) {
- return;
- }
- throw new ValidationException(
- String.format(
- "Could not register catalog function. A temporary
function '%s' does already exist. "
- + "Please drop the temporary function
first.",
- identifier.asSummaryString()));
- }
+ registerCatalogFunction(identifier, catalogFunction, ignoreIfExists);
+ }
- if (catalog.functionExists(path)) {
- if (ignoreIfExists) {
- return;
- }
- throw new ValidationException(
- String.format(
- "Could not register catalog function. A function
'%s' does already exist.",
- identifier.asSummaryString()));
- }
+ public void registerCatalogFunction(
+ UnresolvedIdentifier unresolvedIdentifier,
+ String className,
+ List<ResourceUri> resourceUris,
+ boolean ignoreIfExists) {
+ final ObjectIdentifier identifier =
catalogManager.qualifyIdentifier(unresolvedIdentifier);
final CatalogFunction catalogFunction =
- new CatalogFunctionImpl(functionClass.getName(),
FunctionLanguage.JAVA);
+ new CatalogFunctionImpl(className, FunctionLanguage.JAVA,
resourceUris);
+
try {
- catalog.createFunction(path, catalogFunction, ignoreIfExists);
+ validateAndPrepareFunction(className, catalogFunction);
Review Comment:
I think we shouldn't validate wether the class is can be load here, it is
just a metadata operation for `CatalogFunction`, we will loaded the class when
query use the UDF, this is consistent with the behavior in
https://github.com/apache/flink/blob/0bc50fffe4a331f45b72926c83e778b7ed7fc2e6/flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/internal/TableEnvironmentImpl.java#L1729
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
