mihaibudiu commented on code in PR #4382:
URL: https://github.com/apache/calcite/pull/4382#discussion_r2107928969
##########
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java:
##########
@@ -840,6 +857,29 @@ private static List<RexNode> fields(RelBuilder builder,
int fieldCount) {
return projects;
}
+ /**
+ * Replace every {@link RexFieldAccess} in the given {@link RelNode}
+ * that reference the specified {@link CorrelationId} with another {@link
CorrelationId}.
+ */
+ public static RelNode replaceCorrelationId(RexBuilder rexBuilder, RelNode
node,
+ final CorrelationId from, final CorrelationId to, RelDataType type) {
+ return node.accept(new RelHomogeneousShuttle() {
Review Comment:
does this correctly recurse into subqueries that appear in expressions?
##########
core/src/main/java/org/apache/calcite/rex/RexUtil.java:
##########
@@ -1867,6 +1869,29 @@ public static RexNode shift(RexNode node, final int
start, final int offset) {
});
}
+ /**
+ * Shifts every {@link RexFieldAccess} with {@link CorrelationId}
+ * in an {@link RelNode} by {@code offset}.
+ */
+ public static RelNode shiftFieldAccess(RexBuilder rexBuilder, RelNode node,
+ final CorrelationId id, RelNode outer, final int offset) {
Review Comment:
I assume that offset can be negative as well.
Does the builder check that the resulting offset is legal given the row type?
##########
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java:
##########
@@ -725,10 +731,21 @@ private static RexNode rewriteIn(RexSubQuery e,
Set<CorrelationId> variablesSet,
builder.aggregate(builder.groupKey(),
builder.count(false, "c"),
builder.count(builder.fields()).as("ck"));
- builder.as(ctAlias);
+
if (!variablesSet.isEmpty()) {
- builder.join(JoinRelType.LEFT, trueLiteral, variablesSet);
+ // The same variablesSet will generate a Correlate with the
corresponding CorrelationId,
Review Comment:
this is called "capture-avoiding substitution"
https://en.wikipedia.org/wiki/Lambda_calculus#Capture-avoiding_substitutions
##########
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java:
##########
@@ -840,6 +857,29 @@ private static List<RexNode> fields(RelBuilder builder,
int fieldCount) {
return projects;
}
+ /**
+ * Replace every {@link RexFieldAccess} in the given {@link RelNode}
+ * that reference the specified {@link CorrelationId} with another {@link
CorrelationId}.
Review Comment:
can you document the `type` parameter?
##########
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java:
##########
@@ -960,13 +1001,45 @@ private static void matchJoin(SubQueryRemoveRule rule,
RelOptRuleCall call) {
.union(ImmutableBitSet.range(nFields - nFieldsRight, nFields)));
builder.project(fields);
} else {
- builder.push(join.getLeft());
builder.push(join.getRight());
+ final RelOptUtil.Logic logic =
+ LogicVisitor.find(join.getJoinType().generatesNullsOnLeft()
+ ? RelOptUtil.Logic.TRUE_FALSE_UNKNOWN :
RelOptUtil.Logic.TRUE,
+ ImmutableList.of(join.getCondition()), e);
+
+ RexSubQuery subQuery = e;
+
+ if (!variablesSet.isEmpty()) {
+ // Original correlates reference joint row type, but we are about to
create
Review Comment:
this comment could be even more helpful if you showed a fragment of a plan
before and after
##########
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java:
##########
@@ -840,6 +857,29 @@ private static List<RexNode> fields(RelBuilder builder,
int fieldCount) {
return projects;
}
+ /**
+ * Replace every {@link RexFieldAccess} in the given {@link RelNode}
+ * that reference the specified {@link CorrelationId} with another {@link
CorrelationId}.
Review Comment:
references
##########
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java:
##########
@@ -960,13 +1001,45 @@ private static void matchJoin(SubQueryRemoveRule rule,
RelOptRuleCall call) {
.union(ImmutableBitSet.range(nFields - nFieldsRight, nFields)));
builder.project(fields);
} else {
- builder.push(join.getLeft());
builder.push(join.getRight());
+ final RelOptUtil.Logic logic =
+ LogicVisitor.find(join.getJoinType().generatesNullsOnLeft()
+ ? RelOptUtil.Logic.TRUE_FALSE_UNKNOWN :
RelOptUtil.Logic.TRUE,
+ ImmutableList.of(join.getCondition()), e);
+
+ RexSubQuery subQuery = e;
+
+ if (!variablesSet.isEmpty()) {
+ // Original correlates reference joint row type, but we are about to
create
+ // new join of original right side and correlated sub-query. Therefore
we have
+ // to adjust correlated variables int following way:
+ // 1) new correlation variable must reference row type of right
side only
+ // 2) field index must be shifted on the size of the left side
+ CorrelationId id = Iterables.getOnlyElement(variablesSet);
+ RexBuilder rexBuilder = builder.getRexBuilder();
+
+ RelNode newSubQueryRel = e.rel.accept(new RelHomogeneousShuttle() {
+ @Override public RelNode visit(RelNode other) {
Review Comment:
does this work correctly if there are additional subqueries in the subquery?
##########
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java:
##########
@@ -840,6 +857,29 @@ private static List<RexNode> fields(RelBuilder builder,
int fieldCount) {
return projects;
}
+ /**
+ * Replace every {@link RexFieldAccess} in the given {@link RelNode}
+ * that reference the specified {@link CorrelationId} with another {@link
CorrelationId}.
+ */
+ public static RelNode replaceCorrelationId(RexBuilder rexBuilder, RelNode
node,
+ final CorrelationId from, final CorrelationId to, RelDataType type) {
+ return node.accept(new RelHomogeneousShuttle() {
+ @Override public RelNode visit(RelNode other) {
+ other = other.accept(new RexShuttle() {
+ @Override public RexNode visitFieldAccess(RexFieldAccess
fieldAccess) {
+ if (fieldAccess.getReferenceExpr() instanceof RexCorrelVariable
+ && ((RexCorrelVariable)
fieldAccess.getReferenceExpr()).id.equals(from)) {
+ RexNode correl = rexBuilder.makeCorrel(type, to);
Review Comment:
it looks to me that to and from must have the same type.
Can this be checked here?
--
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]
