jamesstarr commented on a change in pull request #2385:
URL: https://github.com/apache/calcite/pull/2385#discussion_r682803633
##########
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:
The call is reused in assert in matchJoin. So moving to the first one
would mean, that it is repeated twice the `&& containsSubQuery` twice while
still having a helper function. Concerning
`!join.getJoinType().generatesNullsOnLeft()`, the whole expression would have
to be repeated twice, and while unlikely to change, it could allow for drift if
some one wants to change the restrictiveness. Furthermore, the phrasing is not
particular intuitive.
I will rename hasSubQueryAndJoinTypeIsRewritable to
containsSubQueryAndJoinTypeIsRewritable to match the existing verbs and change
it use generatesNullsOnLeft() so it will likely work if any further join types
are added.
##########
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:
I am normally not a big fan of final in this localized of context when
all variables are treated as immutable, but when some variables are mutated,
having final on all the immutable variables signals that the mutable variables
are in fact mutable.
##########
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:
I am normally not a big fan of final in this localized of context when
all variables are treated as immutable, but when some variables are mutated,
having final on all the immutable variables signals that the mutable variables
are in fact mutable.
I will try to shift things to a single where possible.
--
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]
