-
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-19077][table-runtime] Import process time temporal join operator. #13300
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 5364e72 (Tue Sep 01 15:48:18 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:
|
2ffa66b
to
47e4cd9
Compare
831c8b4
to
26d7664
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 the great work @leonardBang .
As we disccused offline, I think we should
- reuse as much code as possible. Currently, the physical node and
TemporalJoinUtil
are not shared, it would be a maintain burden in the future. Would be great if we can reuse them, could you create an issue for this purpose? - Currently, the processing time temporal join doesn't wait for the first complete snapshot of right stream. It would be problematic when used in production.
.../org/apache/flink/table/runtime/operators/join/temporal/TemporalProcessTimeJoinOperator.java
Outdated
Show resolved
Hide resolved
.../org/apache/flink/table/runtime/operators/join/temporal/TemporalProcessTimeJoinOperator.java
Outdated
Show resolved
Hide resolved
.../org/apache/flink/table/runtime/operators/join/temporal/TemporalProcessTimeJoinOperator.java
Show resolved
Hide resolved
...e/flink/table/runtime/operators/join/temporal/LegacyTemporalProcessTimeJoinOperatorTest.java
Outdated
Show resolved
Hide resolved
...e/flink/table/runtime/operators/join/temporal/LegacyTemporalProcessTimeJoinOperatorTest.java
Outdated
Show resolved
Hide resolved
...scala/org/apache/flink/table/planner/plan/nodes/physical/stream/StreamExecTemporalJoin.scala
Outdated
Show resolved
Hide resolved
...scala/org/apache/flink/table/planner/plan/nodes/physical/stream/StreamExecTemporalJoin.scala
Show resolved
Hide resolved
...lanner-blink/src/main/scala/org/apache/flink/table/planner/plan/utils/TemporalJoinUtil.scala
Show resolved
Hide resolved
...nk/src/test/scala/org/apache/flink/table/planner/runtime/stream/sql/TemporalJoinITCase.scala
Outdated
Show resolved
Hide resolved
...nk/src/test/scala/org/apache/flink/table/planner/runtime/stream/sql/TemporalJoinITCase.scala
Show resolved
Hide resolved
@wuchong I address your comments, could you take a look? |
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 the updating. I left some comments.
...che/flink/table/planner/plan/rules/logical/LogicalCorrelateToJoinFromTemporalTableRule.scala
Outdated
Show resolved
Hide resolved
...org/apache/flink/table/planner/plan/rules/logical/TemporalJoinRewriteWithUniqueKeyRule.scala
Outdated
Show resolved
Hide resolved
...org/apache/flink/table/planner/plan/rules/logical/TemporalJoinRewriteWithUniqueKeyRule.scala
Outdated
Show resolved
Hide resolved
...org/apache/flink/table/planner/plan/rules/logical/TemporalJoinRewriteWithUniqueKeyRule.scala
Outdated
Show resolved
Hide resolved
...scala/org/apache/flink/table/planner/plan/nodes/physical/stream/StreamExecTemporalJoin.scala
Outdated
Show resolved
Hide resolved
...scala/org/apache/flink/table/planner/plan/nodes/physical/stream/StreamExecTemporalJoin.scala
Outdated
Show resolved
Hide resolved
...scala/org/apache/flink/table/planner/plan/nodes/physical/stream/StreamExecTemporalJoin.scala
Outdated
Show resolved
Hide resolved
...scala/org/apache/flink/table/planner/plan/nodes/physical/stream/StreamExecTemporalJoin.scala
Outdated
Show resolved
Hide resolved
...scala/org/apache/flink/table/planner/plan/nodes/physical/stream/StreamExecTemporalJoin.scala
Outdated
Show resolved
Hide resolved
...lanner-blink/src/main/scala/org/apache/flink/table/planner/plan/utils/TemporalJoinUtil.scala
Outdated
Show resolved
Hide resolved
I have addressed your comments, appreciate if you can have a more look @wuchong |
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 this pull request has been in a very good shape. I only left some minor comments.
...lanner-blink/src/main/scala/org/apache/flink/table/planner/plan/utils/TemporalJoinUtil.scala
Outdated
Show resolved
Hide resolved
...lanner-blink/src/main/scala/org/apache/flink/table/planner/plan/utils/TemporalJoinUtil.scala
Outdated
Show resolved
Hide resolved
...org/apache/flink/table/planner/plan/rules/logical/TemporalJoinRewriteWithUniqueKeyRule.scala
Outdated
Show resolved
Hide resolved
...k/table/planner/plan/rules/logical/LogicalCorrelateToJoinFromTemporalTableFunctionRule.scala
Outdated
Show resolved
Hide resolved
...cala/org/apache/flink/table/planner/runtime/stream/sql/TemporalTableFunctionJoinITCase.scala
Outdated
Show resolved
Hide resolved
f5ab96a
to
a523781
Compare
@flinkbot run azure |
c777052
to
6bf5dc2
Compare
@flinkbot run azure |
Build is passed: https://dev.azure.com/apache-flink/apache-flink/_build/results?buildId=8452&view=results |
What is the purpose of the change
Brief change log
Verifying this change
Add ITCase
TemporalJoinITCase
andLegacyTemporalJoinITCase
to verify this change.Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (no)Documentation