-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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-36488] [TABLE SQL/API] Remove deprecated methods StreamTableEnvironment.toAppendStream from flink-table-api-java-bridge module #25528
base: master
Are you sure you want to change the base?
Conversation
…vironment.toAppendStream from flink-table-api-java-bridge module
b14a9b3
to
ee7e6b9
Compare
ee7e6b9
to
4a70d1f
Compare
@sn-12-3 do you mean to have this PR in draft ? |
Yes @davidradl , This PR CI is failing due to a python failure which still needs investigation. |
Hi, @sn-12-3 Is there any new progress? |
91b315b
to
6da38a6
Compare
…est dependent on the method
f0ac0a3
to
8c869c8
Compare
@flinkbot run azure |
Hi @xuyangzhong , This PR is ready for review now. I have removed the |
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 updating this pr! I just left some comments.
BTW, it seems that the cleanup hasn't been completely done. When I search for toAppendStream
and to_append_stream
in the IDE, I still find many references that need to be removed, such as in doc/content/docks/dev/table/data_stream_api.md
, table_environment.py
, etc.
@@ -260,57 +260,6 @@ def test_execute_async(self): | |||
execution_result = job_client.get_job_execution_result().result() | |||
self.assertEqual(str(job_id), str(execution_result.get_job_id())) | |||
|
|||
def test_add_python_file(self): |
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 don't think we can directly remove this test, as it seems to verify other functionalities.
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.
Used to_data_stream
instead of to_append_stream
.
@@ -1290,47 +1290,6 @@ class WindowAggregateITCase( | |||
.isEqualTo(expected.sorted.mkString("\n")) | |||
} | |||
|
|||
@TestTemplate | |||
def testDistinctAggWithMergeOnEventTimeSessionWindow(): Unit = { |
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.
Used toDataStream
instead of toAppendStream
.
<T> DataStream<T> toAppendStream(Table table, TypeInformation<T> typeInfo); | ||
|
||
/** | ||
* Converts the given {@link Table} into a {@link DataStream} of add and retract messages. The |
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.
You removed the comment for toRetractStream but kept the comment for toAppendStream.
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.
My bad. Apologies, corrected now.
@flinkbot run azure |
Converts the given Table into a DataStream. | ||
|
||
Since the DataStream API does not support changelog processing natively, this method | ||
assumes append-only/insert-only semantics during the table-to-stream conversion. The records |
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: the documentation only refers to insert-only. Can we remove append-only or is this indicating another semantic?
Maybe involve parts of the original comment "Converts the given Table into a DataStream of a specified type. Since the DataStream API does not support changelog processing natively, the table must only have insert (append) changes. If the Table is also modified by update or delete changes, the conversion will fail."
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 seems to indicate another semantic fit for insert-only
operation. Elsewhere in the codebase, the same documentation is maintained.
After the changes based on the review comments, this is outdated as no new function has been added in this file. Only to_append_stream
relevant function has been removed.
Types.STRING()])) | ||
test_sink = DataStreamTestSinkFunction() | ||
ds.add_sink(test_sink) | ||
self.env.execute("test_to_data_stream") |
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 test deletes and updates error cases?
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'm afraid I understand, this is adding the sink
operator to the dataStream
function, there is no functions related to delete
or update
.
After the changes based on the review comments, this is outdated as no new test has been added in this file. Only toAppendStream
relevant test case has been removed.
@flinkbot run azure |
@xuyangzhong , @davidradl , Thanks for the review. Incorporated the review comments; PR is ready for re-review. |
@xuyangzhong , @davidradl , Gentle reminder. |
@flinkbot run azure |
JIRA issue
FLINK-36488
What is the purpose of the change
Remove deprecated method from
StreamTableEnvironment.toAppendStream
. This links to parent issue FLINK-36476Brief change log
Remove deprecated methods
StreamTableEnvironment.toAppendStream
fromflink-table-api-java-bridge
module.Verifying this change
Use mvn spotless:apply to verify.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation