-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[FLINK-37724] Adds AsyncTableFunction as a fully supported UDF type #26567
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
41f1eb3
to
e1b0ed6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Alan for the contribution! I made a brief pass and left some initial comments
@@ -0,0 +1,18 @@ | |||
<table class="configuration table table-bordered"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unrelated the async table function?
public static boolean isGenericOfClass(Class<?> clazz, Type type) { | ||
Optional<ParameterizedType> parameterized = getParameterizedType(type); | ||
return clazz.equals(type) | ||
|| parameterized.isPresent() && clazz.equals(parameterized.get().getRawType()); | ||
} | ||
|
||
/** | ||
* Returns an optional of a ParameterizedType, if that's what the type is. | ||
* | ||
* @param type The type to check | ||
* @return optional which is present if the type is a ParameterizedType | ||
*/ | ||
public static Optional<ParameterizedType> getParameterizedType(Type type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add unit test for these two function?
@@ -373,4 +374,28 @@ public static void validateLambdaType(Class<?> baseClass, Type t) { | |||
+ "Otherwise the type has to be specified explicitly using type information."); | |||
} | |||
} | |||
|
|||
/** | |||
* Will return true if the type of the given generic class type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Will return true if the type of the given generic class type. | |
* Will return true if the type of the given generic class type matches clazz. |
.durationType() | ||
.defaultValue(Duration.ofMinutes(3)) | ||
.withDescription( | ||
"The async timeout for the asynchronous operation to complete."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the timeout for each retry or total timeout for all retries? I suppose it's for each retry
/** Implementation method isn't void. */ | ||
public static class NoFutureAsyncScalarFunction extends AsyncScalarFunction { | ||
public void eval(int i) {} | ||
} | ||
|
||
/** Implementation method isn't void. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first param isn't CompletableFuture<Collection>
?
public void eval(int i) {} | ||
} | ||
|
||
/** Implementation method isn't void. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
minPlanVersion = FlinkVersion.v1_19, | ||
minStateVersion = FlinkVersion.v1_19) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not exist yet in 1.19?
@@ -71,24 +72,35 @@ public class AsyncCalcSplitRule { | |||
* org.apache.flink.table.functions.AsyncScalarFunction}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as well as AsyncTableFunction
?
public AsyncRemoteCalcCallFinder() { | ||
this(FunctionKind.ASYNC_SCALAR); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Not sure if we want to default to scalar function given both are supported now. Requiring functionKind is probably fine
String sqlQuery = "select * FROM MyTable, LATERAL TABLE(asyncTableFunc(scalar(a)))"; | ||
util.verifyRelPlan(sqlQuery); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add
@Test
public void testCorrelateWithCast() {
String sqlQuery =
"select * FROM MyTable, LATERAL TABLE(asyncTableFunc(cast(cast(a as int) as int)))";
util.verifyRelPlan(sqlQuery);
}
There was a fix I made internally around cast
What is the purpose of the change
This aims to implement FLIP-498 https://cwiki.apache.org/confluence/display/FLINK/FLIP-498%3A+AsyncTableFunction+for+async+table+function+support.
In particular
AsyncTableFunction
s can now be defined as:They can be registered in the catalog as with other UDFs, such as with:
There are a few new configs:
Brief change log
AsyncTableFunction
support to type system, codegen, and runtimeVerifying this change
This change added tests and can be verified as follows:
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)AsyncTableFunction
use with lookup joinsDocumentation