You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/11/04 08:01:47 UTC
[GitHub] [flink] JingGe commented on a diff in pull request #21219: [FLINK-29849][table-planner] Fix event time temporal join on an upsert source may produce incorrect execution plan
JingGe commented on code in PR #21219:
URL: https://github.com/apache/flink/pull/21219#discussion_r1013714868
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/rules/logical/FlinkFilterJoinRule.java:
##########
@@ -449,6 +456,34 @@ protected FlinkFilterIntoJoinRule(FlinkFilterIntoJoinRule.Config config) {
super(config);
}
+ @Override
+ public boolean matches(RelOptRuleCall call) {
+ Join join = call.rel(1);
+ boolean existTemporalJoinCondition = existTemporalJoinCondition(join.getCondition());
+ return !existTemporalJoinCondition && super.matches(call);
+ }
+
+ private boolean existTemporalJoinCondition(RexNode joinCondition) {
Review Comment:
NIT: I would suggest replacing the the naming of `existTemporalJoinCondition` with `isEventTimeTemoralJoin` (or `isNotEventTimeTemoralJoin`) to improve the code readability. The return on #463 could become:
`return !isEventTimeTemoralJoin && super.matches(call);`
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/rules/logical/FlinkFilterJoinRule.java:
##########
@@ -440,6 +444,9 @@ default FlinkJoinConditionPushRule toRule() {
* Rule that tries to push filter expressions into a join condition and into the inputs of the
* join.
*
+ * <p>Note: It never pushes a filter into join which is a temporal join using event time in
Review Comment:
NIT: Using official glossary[1] in javadoc.
[1] https://nightlies.apache.org/flink/flink-docs-master/docs/dev/table/sql/queries/joins/#event-time-temporal-join
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/rules/logical/FlinkFilterJoinRule.java:
##########
@@ -440,6 +444,9 @@ default FlinkJoinConditionPushRule toRule() {
* Rule that tries to push filter expressions into a join condition and into the inputs of the
* join.
*
+ * <p>Note: It never pushes a filter into join which is a temporal join using event time in
Review Comment:
```suggestion
* <p>Note: For event time temporal join, filter will never be pushed into join.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org