silundong commented on code in PR #4840:
URL: https://github.com/apache/calcite/pull/4840#discussion_r3038600779
##########
core/src/main/java/org/apache/calcite/rel/core/Join.java:
##########
@@ -355,6 +355,30 @@ public static RelDataType createJoinType(
public abstract Join copy(RelTraitSet traitSet, RexNode conditionExpr,
RelNode left, RelNode right, JoinRelType joinType, boolean semiJoinDone);
+ /**
+ * Creates a copy of this join, overriding condition, system fields and
+ * inputs.
+ *
+ * <p>General contract as {@link RelNode#copy}.
+ *
+ * @param traitSet Traits
+ * @param conditionExpr Condition
+ * @param left Left input
+ * @param right Right input
+ * @param joinType Join type
+ * @param semiJoinDone Whether this join has been translated to a
+ * semi-join
+ * @param variablesSet Set of variables that are set by the
+ * LHS and used by the RHS and are not available to
+ * nodes above this LogicalJoin in the tree
+ * @return Copy of this join
+ */
+ public Join copy(RelTraitSet traitSet, RexNode conditionExpr,
Review Comment:
IMO, this `copy` seems unnecessary and carries risk: any `Join` subclass
that doesn't override this method will silently lose `variablesSet`. Maybe you
can use `RelBuilder.join` instead of `copy` in `FilterJoinRule`.
##########
core/src/main/java/org/apache/calcite/rel/rules/FilterJoinRule.java:
##########
@@ -198,14 +201,35 @@ protected void perform(RelOptRuleCall call, @Nullable
Filter filter,
return;
}
+ Set<CorrelationId> leftVariablesSet = new LinkedHashSet<>();
+ Set<CorrelationId> rightVariablesSet = new LinkedHashSet<>();
+
+ for (RexNode condition : leftFilters) {
+ condition.accept(new RexVisitorImpl<Void>(true) {
+ @Override public Void visitSubQuery(RexSubQuery subQuery) {
+ leftVariablesSet.addAll(RelOptUtil.getVariablesUsed(subQuery.rel));
+ return super.visitSubQuery(subQuery);
+ }
+ });
+ }
+
+ for (RexNode condition : rightFilters) {
+ condition.accept(new RexVisitorImpl<Void>(true) {
+ @Override public Void visitSubQuery(RexSubQuery subQuery) {
+ rightVariablesSet.addAll(RelOptUtil.getVariablesUsed(subQuery.rel));
+ return super.visitSubQuery(subQuery);
+ }
+ });
+ }
+
// create Filters on top of the children if any filters were
// pushed to them
final RexBuilder rexBuilder = join.getCluster().getRexBuilder();
final RelBuilder relBuilder = call.builder();
final RelNode leftRel =
- relBuilder.push(join.getLeft()).filter(leftFilters).build();
+ relBuilder.push(join.getLeft()).filter(leftVariablesSet,
leftFilters).build();
final RelNode rightRel =
- relBuilder.push(join.getRight()).filter(rightFilters).build();
+ relBuilder.push(join.getRight()).filter(rightVariablesSet,
rightFilters).build();
Review Comment:
If I understand correctly, you solved the problem with the for-loop added
above:
```
ImmutableSet.Builder<CorrelationId> newJoinCorrelationIds =
ImmutableSet.builder();
for (CorrelationId correlationId : join.getVariablesSet()) {
if (!leftVariablesSet.contains(correlationId)
&& !rightVariablesSet.contains(correlationId)) {
newJoinCorrelationIds.add(correlationId);
}
}
```
But this causes the Join to lose the `variablesSet` it should have kept. As
in the example above, a correlated subquery still remains in the join
condition. I think the correct fix is to generate a new `CorrelationId` for the
`Filter` being pushed down and replace the corresponding `CorrelationId` inside
the pushed correlated subquery.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
