Copilot commented on code in PR #4840:
URL: https://github.com/apache/calcite/pull/4840#discussion_r2958280680
##########
core/src/main/java/org/apache/calcite/rel/rules/FilterJoinRule.java:
##########
@@ -250,7 +277,7 @@ protected void perform(RelOptRuleCall call, @Nullable
Filter filter,
// create a FilterRel on top of the join if needed
relBuilder.filter(
- filter == null ? ImmutableSet.of() : filter.getVariablesSet(),
+ variablesSet,
RexUtil.fixUp(rexBuilder, aboveFilters,
Review Comment:
variablesSet passed to the (remaining) top filter is being expanded with
variables discovered in left/right pushed filters. That can cause the top
Filter to claim correlations that are no longer present in aboveFilters, which
may block later rules that require getVariablesSet() to be accurate. Consider
keeping variablesSet scoped to the predicates that remain in aboveFilters (and
using leftVariablesSet/rightVariablesSet only for the new child filters).
##########
core/src/main/java/org/apache/calcite/rel/rules/FilterJoinRule.java:
##########
@@ -198,14 +200,39 @@ protected void perform(RelOptRuleCall call, @Nullable
Filter filter,
return;
}
+ Set<CorrelationId> variablesSet;
+ Set<CorrelationId> leftVariablesSet = new LinkedHashSet<>();
+ Set<CorrelationId> rightVariablesSet = new LinkedHashSet<>();
+ if (filter == null) {
+ variablesSet = ImmutableSet.of();
+ } else {
+ variablesSet = new LinkedHashSet<>(filter.getVariablesSet());
+ for (RexNode condition : leftFilters) {
+ RexSubQuery rexSubQuery = RexUtil.SubQueryFinder.find(condition);
+ if (rexSubQuery != null) {
+
leftVariablesSet.addAll(RelOptUtil.getVariablesUsed(rexSubQuery.rel));
+ }
+ }
+
+ for (RexNode condition : rightFilters) {
+ RexSubQuery rexSubQuery = RexUtil.SubQueryFinder.find(condition);
+ if (rexSubQuery != null) {
+
rightVariablesSet.addAll(RelOptUtil.getVariablesUsed(rexSubQuery.rel));
+ }
+ }
Review Comment:
SubQueryFinder.find(condition) returns only the first RexSubQuery in an
expression. If a pushed filter contains multiple sub-queries, this logic will
miss correlation variables from the other sub-queries and the resulting Filter
variablesSet may be incomplete. Consider collecting all RexSubQuery instances
in the condition (via a small RexVisitor) and unioning
RelOptUtil.getVariablesUsed(subQ.rel) for each.
##########
core/src/main/java/org/apache/calcite/rel/rules/FilterJoinRule.java:
##########
@@ -198,14 +200,39 @@ protected void perform(RelOptRuleCall call, @Nullable
Filter filter,
return;
}
+ Set<CorrelationId> variablesSet;
+ Set<CorrelationId> leftVariablesSet = new LinkedHashSet<>();
+ Set<CorrelationId> rightVariablesSet = new LinkedHashSet<>();
+ if (filter == null) {
+ variablesSet = ImmutableSet.of();
+ } else {
+ variablesSet = new LinkedHashSet<>(filter.getVariablesSet());
+ for (RexNode condition : leftFilters) {
+ RexSubQuery rexSubQuery = RexUtil.SubQueryFinder.find(condition);
+ if (rexSubQuery != null) {
+
leftVariablesSet.addAll(RelOptUtil.getVariablesUsed(rexSubQuery.rel));
+ }
+ }
+
+ for (RexNode condition : rightFilters) {
+ RexSubQuery rexSubQuery = RexUtil.SubQueryFinder.find(condition);
+ if (rexSubQuery != null) {
+
rightVariablesSet.addAll(RelOptUtil.getVariablesUsed(rexSubQuery.rel));
+ }
+ }
+
+ variablesSet.addAll(leftVariablesSet);
+ variablesSet.addAll(rightVariablesSet);
+ }
+
Review Comment:
In JoinConditionPushRule (filter == null), this code never computes
variables sets for pushed-down filters, so any correlation variables coming
from sub-queries inside join conditions will be lost when creating left/right
Filters. Consider computing leftVariablesSet/rightVariablesSet from
leftFilters/rightFilters regardless of whether an above Filter exists (and
passing them into relBuilder.filter accordingly).
##########
core/src/test/java/org/apache/calcite/sql2rel/RelDecorrelatorTest.java:
##########
@@ -1830,4 +1830,80 @@ public static Frameworks.ConfigBuilder config() {
+ " LogicalTableScan(table=[[scott, EMP]])\n";
assertThat(after, hasTree(planAfter));
}
+
+/** Test case for <a
href="https://issues.apache.org/jira/browse/CALCITE-7442";>[CALCITE-7442]
+ * Getting Wrong index of Correlated variable inside Subquery after
FilterJoinRule</a>. */
Review Comment:
Javadoc indentation and formatting is inconsistent with the surrounding
tests (e.g., most test Javadocs in this file are aligned with two-space
indentation). Consider indenting this new Javadoc to match the file’s style,
and tightening the wording/capitalization (e.g., “wrong index” rather than
“Wrong index”).
##########
core/src/main/java/org/apache/calcite/plan/RelOptUtil.java:
##########
@@ -4818,22 +4831,55 @@ public RexInputConverter(
null,
leftDestFields,
rightDestFields,
- adjustments);
+ adjustments,
+ 0,
+ null);
}
public RexInputConverter(
RexBuilder rexBuilder,
@Nullable List<RelDataTypeField> srcFields,
@Nullable List<RelDataTypeField> destFields,
int[] adjustments) {
- this(rexBuilder, srcFields, destFields, null, null, adjustments);
+ this(rexBuilder, srcFields, destFields, null, null, adjustments, 0,
null);
}
public RexInputConverter(
RexBuilder rexBuilder,
@Nullable List<RelDataTypeField> srcFields,
int[] adjustments) {
- this(rexBuilder, srcFields, null, null, null, adjustments);
+ this(rexBuilder, srcFields, null, null, null, adjustments, 0, null);
+ }
+
+ public RexInputConverter(
+ RexBuilder rexBuilder,
+ @Nullable List<RelDataTypeField> srcFields,
+ @Nullable List<RelDataTypeField> destFields,
+ int[] adjustments,
+ int offset,
+ RelNode child) {
+ this(rexBuilder, srcFields, destFields, null, null, adjustments, offset,
child);
+ }
+
+ @Override public RexNode visitSubQuery(RexSubQuery subQuery) {
+ boolean[] update = {false};
+ List<RexNode> clonedOperands = visitList(subQuery.operands, update);
+ if (update[0]) {
+ subQuery = subQuery.clone(subQuery.getType(), clonedOperands);
+ final Set<CorrelationId> variablesSet =
RelOptUtil.getVariablesUsed(subQuery.rel);
+ if (!variablesSet.isEmpty() && correlateVariableChild != null) {
+ CorrelationId id = Iterables.getOnlyElement(variablesSet);
+ RelNode newSubQueryRel = subQuery.rel.accept(new
RelHomogeneousShuttle() {
+ @Override public RelNode visit(RelNode other) {
+ RelNode node =
+ RexUtil.shiftFieldAccess(rexBuilder, other, id,
correlateVariableChild, offset);
+ return super.visit(node);
+ }
+ });
+ subQuery = subQuery.clone(newSubQueryRel);
+ }
+ }
+ return subQuery;
Review Comment:
RexInputConverter.visitSubQuery only applies correlated-field shifting when
a sub-query operand was modified (update[0] == true). But shiftFilter may need
to adjust correlation field accesses inside subQuery.rel even when operands are
unchanged (e.g., EXISTS sub-queries often have no operands). Consider
performing the RelOptUtil.getVariablesUsed/shiftFieldAccess step whenever
offset != 0 (and correlateVariableChild is set), independent of whether
operands changed, cloning the sub-query only if something actually changed.
--
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]
