zabetak commented on a change in pull request #2385:
URL: https://github.com/apache/calcite/pull/2385#discussion_r682486062
##########
File path:
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java
##########
@@ -695,7 +715,7 @@ private static void matchJoin(SubQueryRemoveRule rule,
RelOptRuleCall call) {
Config JOIN = EMPTY
.withOperandSupplier(b ->
b.operand(Join.class)
- .predicate(RexUtil.SubQueryFinder::containsSubQuery)
+
.predicate(SubQueryRemoveRule::hasSubQueryAndJoinTypeIsRewritable)
Review comment:
Two other alternatives for the predicate; not sure if they are better
than the current proposal I just like when methods do only one thing:
```
(join) -> isRewritable(join) && RexUtil.SubQueryFinder.containsSubQuery(join)
(join) -> !join.getJoinType().generatesNullsOnLeft &&
RexUtil.SubQueryFinder.containsSubQuery(join)
```
the second is not 100% equivalent but I am not sure if we intent to cover
`CROSS`, `LEFT_SEMI_JOIN`, `COMMA`, if it ever reaches this rule.
Not necessarily requesting a change, leaving the final decision to
@jamesstarr !
##########
File path:
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java
##########
@@ -638,26 +639,45 @@ private static void matchFilter(SubQueryRemoveRule rule,
private static void matchJoin(SubQueryRemoveRule rule, RelOptRuleCall call) {
final Join join = call.rel(0);
- final RelBuilder builder = call.builder();
- final RexSubQuery e =
- RexUtil.SubQueryFinder.find(join.getCondition());
- assert e != null;
- final RelOptUtil.Logic logic =
- LogicVisitor.find(RelOptUtil.Logic.TRUE,
- ImmutableList.of(join.getCondition()), e);
- builder.push(join.getLeft());
- builder.push(join.getRight());
- final int fieldCount = join.getRowType().getFieldCount();
- final Set<CorrelationId> variablesSet =
- RelOptUtil.getVariablesUsed(e.rel);
- final RexNode target = rule.apply(e, variablesSet,
- logic, builder, 2, fieldCount);
- final RexShuttle shuttle = new ReplaceSubQueryShuttle(e, target);
- builder.join(join.getJoinType(), shuttle.apply(join.getCondition()));
- builder.project(fields(builder, join.getRowType().getFieldCount()));
+ assert hasSubQueryAndJoinTypeIsRewritable(join);
+ final RelBuilder builder = call.builder()
+ .push(join.getLeft())
+ .push(join.getRight());
+ final CorrelationId id = join.getCluster().createCorrel();
+ RexNode condition =
RelOptUtil.correlateLeftShiftRight(builder.getRexBuilder(),
+ join.getLeft(), id, join.getRight(), join.getCondition());
+ for (RexSubQuery subQuery = RexUtil.SubQueryFinder.find(condition);
subQuery != null;
+ subQuery = RexUtil.SubQueryFinder.find(condition)) {
Review comment:
I like the change from while (true) to for :)
##########
File path:
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java
##########
@@ -638,26 +639,45 @@ private static void matchFilter(SubQueryRemoveRule rule,
private static void matchJoin(SubQueryRemoveRule rule, RelOptRuleCall call) {
final Join join = call.rel(0);
- final RelBuilder builder = call.builder();
- final RexSubQuery e =
- RexUtil.SubQueryFinder.find(join.getCondition());
- assert e != null;
- final RelOptUtil.Logic logic =
- LogicVisitor.find(RelOptUtil.Logic.TRUE,
- ImmutableList.of(join.getCondition()), e);
- builder.push(join.getLeft());
- builder.push(join.getRight());
- final int fieldCount = join.getRowType().getFieldCount();
- final Set<CorrelationId> variablesSet =
- RelOptUtil.getVariablesUsed(e.rel);
- final RexNode target = rule.apply(e, variablesSet,
- logic, builder, 2, fieldCount);
- final RexShuttle shuttle = new ReplaceSubQueryShuttle(e, target);
- builder.join(join.getJoinType(), shuttle.apply(join.getCondition()));
- builder.project(fields(builder, join.getRowType().getFieldCount()));
+ assert hasSubQueryAndJoinTypeIsRewritable(join);
+ final RelBuilder builder = call.builder()
+ .push(join.getLeft())
+ .push(join.getRight());
+ final CorrelationId id = join.getCluster().createCorrel();
+ RexNode condition =
RelOptUtil.correlateLeftShiftRight(builder.getRexBuilder(),
+ join.getLeft(), id, join.getRight(), join.getCondition());
+ for (RexSubQuery subQuery = RexUtil.SubQueryFinder.find(condition);
subQuery != null;
+ subQuery = RexUtil.SubQueryFinder.find(condition)) {
+ final Set<CorrelationId> variablesSet =
+ RelOptUtil.getVariablesUsed(subQuery.rel);
+ final RelOptUtil.Logic logic =
+ LogicVisitor.find(RelOptUtil.Logic.TRUE,
ImmutableList.of(condition), subQuery);
+ int offset = builder.peek().getRowType().getFieldCount();
+ final RexNode target = rule.apply(subQuery, variablesSet, logic,
+ builder, 1, offset);
+ final RexShuttle shuttle = new ReplaceSubQueryShuttle(subQuery, target);
+ condition = shuttle.apply(condition);
Review comment:
NIT: The `final` at this level make the code more verbose with small
benefit given that the scope is very small. In some cases removing it could
make things fit in one line.
##########
File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
##########
@@ -305,6 +305,32 @@ public static boolean notContainsCorrelation(RelNode r,
}
}
+ /** Rewrites an {@link RexNode} on top of a correlated join to the rights
context by
+ * shifting {@link RexInputRef} if they are from the right side, and
rewriting
+ * {@link RexInputRef} to field accesses of {@link RexCorrelVariable}.
+ * @param left Rel fields to be correlated
+ * @param id id to correlate
+ * @param right Rel field to be shifted to the right
+ * @param rexNode expression to be rewritten
+ * @return An expression
+ */
Review comment:
Suggestion below:
```
Transforms a join condition over two inputs to a correlated condition over
the right input.
Replaces references on the left relation with correlating variables and
shifts references on the right relation as needed to be able to put the
condition over the right relation.
TODO: Add example similar to the one below.
<pre>{@code
Join[cond=..]
TableScan[EMP]
TableScan[DEPT]
}</pre>
<pre>{@code
Operator
TableScan[EMP]
Filter[cond=..]
TableScan[DEPT]
}</pre>
```
Rename method to either:
```
transformJoinConditionToCorrelate(RexNode condition, RelNode left, RelNode
right, CorrelationId id, RexBuilder builder)
transformJoinToCorrelateCondition(...)
transformConditionFromJoinToCorrelate(...)
```
--
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]
