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-17902][python] Support the new interfaces about temporary functions in PyFlink #12476
[FLINK-17902][python] Support the new interfaces about temporary functions in PyFlink #12476
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit b874a61 (Thu Jun 04 05:47:46 UTC 2020) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
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.
@SteNicholas Thanks a lot for the PR. I think we should support both the Java and Python functions for these interfaces just like register_function
and register_java_function
does. What do you think?
@@ -147,30 +147,185 @@ def get_catalog(self, catalog_name): | |||
else: | |||
return None | |||
|
|||
def load_module(self, module_name: str, module: Module): |
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.
What's the reason of this change? We are to support type hint in 1.12 and so it will be good if the newly introduced interfaces uses type hint.
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.
@dianfu I followed the code style from previous, and unified the parameter style. Therefore, I removed the type hint.
@@ -154,7 +155,9 @@ def load_module(self, module_name: str, module: Module): | |||
ValidationException is thrown when there is already a module with the same name. | |||
|
|||
:param module_name: Name of the :class:`~pyflink.table.Module`. | |||
:type module_name: str |
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.
could be removed since the type information is already available in the typehint.
@@ -166,11 +169,254 @@ def unload_module(self, module_name: str): | |||
ValidationException is thrown when there is no module with the given name. | |||
|
|||
:param module_name: Name of the :class:`~pyflink.table.Module`. | |||
:type module_name: str |
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
""" | ||
Registers a java user defined function class as a temporary system function. | ||
|
||
Compared to .. seealso:: :func:`create_temporary_function`, system functions are identified |
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.
create_temporary_function -> create_java_temporary_function
""" | ||
Registers a java user defined function class as a temporary catalog function. | ||
|
||
Compared to .. seealso:: :func:`create_temporary_system_function` with a globally defined |
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.
create_temporary_system_function -> create_java_temporary_system_function
""" | ||
return self._j_tenv.dropTemporarySystemFunction(name) | ||
|
||
def create_java_function(self, path: str, function_class_name: str, |
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.
should also add create_function
?
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.
@dianfu You are right, this method for python should be supported. I think we could new a pull request for this. In this pull request, just consistent with Java interface.
…tions in PyFlink This closes apache#12476.
What is the purpose of the change
TableEnvironment
in PyFlink should be consistent with the interfaces such ascreateTemporarySystemFunction
,dropTemporarySystemFunction
,createFunction
,dropFunction
,createTemporaryFunction
,dropTemporaryFunction
in the JavaTableEnvironment
.Brief change log
TableEnvironment
add the interfaces includingcreateTemporarySystemFunction
,dropTemporarySystemFunction
,createFunction
,dropFunction
,createTemporaryFunction
,dropTemporaryFunction
.Verifying this change
TableEnvironmentTest
andBatchTableEnvironmentTests
add methodtest_create_drop_java_function
to verify whether interfacecreateTemporarySystemFunction
,dropTemporarySystemFunction
,createFunction
,dropFunction
,createTemporaryFunction
,dropTemporaryFunction
could work.Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation