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-23115][python] Expose new APIs about TableDescriptor in PyFlink #16516
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 59da1ae (Thu Sep 23 18:07:16 UTC 2021) 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.
Thank you @HuangXingBo for taking on this issue, I appreciate it. Will someone from your team review it? I'm not too familiar with PyFlink, but if needed I can also review this. Just let me know. Thanks!
@@ -733,9 +734,22 @@ def get_schema(self) -> TableSchema: | |||
Get the schema of the table. | |||
|
|||
:return: Schema of the table/view. | |||
|
|||
. note:: Deprecated in 1.14. This method returns the deprecated ableSchema class. The old |
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: missing "T" in TableSchema"
class ConfigOptions(object): | ||
""" | ||
{@code ConfigOptions} are used to build a :class:`~pyflink.table.ConfigOption`. The option is | ||
typically built in one of the following pattern: |
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: "pattern" → "patterns"
Thanks @Airblader for the review. @dianfu is familiar with PyFlink and he will help review this PR too. |
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.
@HuangXingBo Thanks for the PR! Have left a few minor comments. Besides, is there any documentation needs to be updated?
|
||
class ConfigOptions(object): | ||
""" | ||
{@code ConfigOptions} are used to build a :class:`~pyflink.table.ConfigOption`. The option is |
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.
{@code ConfigOptions} are used to build a :class:`~pyflink.table.ConfigOption`. The option is | |
{@code ConfigOptions} are used to build a :class:`~pyflink.common.ConfigOption`. The option is |
|
||
T = TypeVar('T') | ||
|
||
__all__ = ['ConfigOptions', 'ConfigOption'] |
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 about also adding it in init.py? otherwise, I'm afraid that it will not appear in the Python doc
from pyflink.table.types import DataType, _to_java_data_type | ||
from pyflink.util.java_utils import to_jarray | ||
|
||
__all__ = ['Schema'] |
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.
also added in init.py?
flink-python/pyflink/table/schema.py
Outdated
""" | ||
Adopts all members from the given unresolved schema. | ||
""" | ||
self._j_builder.fromSchema(unresolved_schema) |
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.
self._j_builder.fromSchema(unresolved_schema) | |
self._j_builder.fromSchema(unresolved_schema._j_schema) |
flink-python/pyflink/table/schema.py
Outdated
""" | ||
Adopts all fields of the given row as physical columns of the schema. | ||
""" | ||
self._j_builder.fromRowDataType(data_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.
ditto
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.
It would be great to improve the test case to also cover this method
Examples: | ||
:: | ||
|
||
>>>stmt_set = table_env.create_statement_set() |
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.
>>>stmt_set = table_env.create_statement_set() | |
>>> stmt_set = table_env.create_statement_set() |
@dianfu Thanks a lot for the review. I have addressed the comments at the corresponding commits. About the documentation part, I plan to add the content after the java doc has been updated in https://issues.apache.org/jira/browse/FLINK-23116 |
…tor/Schema in Python Table API
…tor/Schema in Python Table API-fix-1
…tor/Schema in Python Table API-fix-2
…_descriptor of TableEnvironment in Python Table API
…_descriptor of TableEnvironment in Python Table API-fix-1
…d_insert in Python Table API
…d_insert in Python Table API-fix-1
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. Just two minor comments and +1 from my side.
""" | ||
Defines that the value of the option should be of float type | ||
(4-byte single precision floating point number). | ||
:return: |
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.
:return: |
def add_insert(self, target_path: str, table, overwrite: bool = False) -> 'StatementSet': | ||
def add_insert(self, | ||
target_path_or_descriptor: Union[str, TableDescriptor], | ||
table, |
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.
table, | |
table: Table, |
…_descriptor of TableEnvironment in Python Table API This closes #16516.
…d_insert in Python Table API This closes #16516.
…tor/Schema in Python Table API This closes apache#16516.
…_descriptor of TableEnvironment in Python Table API This closes apache#16516.
…d_insert in Python Table API This closes apache#16516.
…tor/Schema in Python Table API This closes apache#16516.
…_descriptor of TableEnvironment in Python Table API This closes apache#16516.
…d_insert in Python Table API This closes apache#16516.
What is the purpose of the change
This pull request will expose new APIs about TableDescriptor in PyFlink
Brief change log
Verifying this change
This change added tests and can be verified as follows:
test_schema.py
,test_table_descriptor.py
,test_table_environment_api.py
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (no)Documentation