-
Notifications
You must be signed in to change notification settings - Fork 13k
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
[FLINK-29000][python] Support python UDF in the SQL Gateway #21725
Conversation
1af7133
to
6f3dc26
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 for your great work. I left some suggestions.
@@ -1097,6 +1097,12 @@ under the License. | |||
<version>${project.version}</version> | |||
<scope>test</scope> | |||
</dependency> | |||
<dependency> |
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.
Why add python dependencies here?
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.
if we don't add the python jar as test dependency, HiveServer2EndpointITCase
and HiveServer2EndpointStatementITCase
will failed.
dependencies.add(location); | ||
} | ||
} catch (URISyntaxException | ClassNotFoundException e) { | ||
throw new SqlExecutionException("Failed to find flink-python jar.", e); |
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.
throw SqlGatewayException. In the SqlGateway#startSqlGateway
, we will notice users it's a bug if the type of the exception is not SqlGatewayException.
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.
Make sense.
final List<URL> dependencies = new ArrayList<>(); | ||
// add python dependencies by default | ||
try { | ||
URL location = | ||
Class.forName( | ||
"org.apache.flink.python.PythonFunctionRunner", | ||
false, | ||
Thread.currentThread().getContextClassLoader()) | ||
.getProtectionDomain() | ||
.getCodeSource() | ||
.getLocation(); | ||
if (Paths.get(location.toURI()).toFile().isFile()) { | ||
dependencies.add(location); | ||
} | ||
} catch (URISyntaxException | ClassNotFoundException e) { | ||
throw new SqlExecutionException("Failed to find flink-python jar.", e); |
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 we move this code block to DefaultContext#load
? I think we will introduce -l/-j comand line paramters in the future.
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.
If it is the -j approach, I guess that you want to make the loading of python jar an optional behavior?
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.
I think we can modify the constructor of the DefaultContext to pass the dependencies. During the loading, we always try to find the python dependencies and add it into the dependencies.
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.
LGTM.
…1725) * [FLINK-29000][python] Support python UDF in the SQL Gateway * fix comments * fix checkstyle
…1725) * [FLINK-29000][python] Support python UDF in the SQL Gateway * fix comments * fix checkstyle
What is the purpose of the change
This pull request will support python UDF in the SQL Gateway
Brief change log
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (no)Documentation