Skip to content

[CALCITE-7442] Getting Wrong index of Correlated variable inside Subquery after FilterJoinRule #4840

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
yashlimbad wants to merge 1 commit into
base: main
Choose a base branch
from
+262 −12
Open

[CALCITE-7442] Getting Wrong index of Correlated variable inside Subquery after FilterJoinRule #4840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 80 additions & 10 deletions core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2957,7 +2957,8 @@ public static boolean classifyFilters(
joinFields,
nTotalFields,
leftFields,
filter);
filter,
joinRel.getInput(0));

leftFilters.add(shiftedFilter);
}
Expand All @@ -2975,7 +2976,8 @@ public static boolean classifyFilters(
joinFields,
nTotalFields,
rightFields,
filter);
filter,
joinRel.getInput(1));
rightFilters.add(shiftedFilter);
}
filtersToRemove.add(filter);
Expand Down Expand Up @@ -3079,7 +3081,8 @@ public static boolean classifyFilters(
joinFields,
nTotalFields,
leftFields,
filter);
filter,
joinRel.getInput(0));

leftFilters.add(shiftedFilter);
}
Expand All @@ -3105,7 +3108,8 @@ public static boolean classifyFilters(
joinFields,
nTotalFields,
rightFields,
filter);
filter,
joinRel.getInput(1));
rightFilters.add(shiftedFilter);
}
filtersToRemove.add(filter);
Expand Down Expand Up @@ -3140,7 +3144,8 @@ private static RexNode shiftFilter(
List<RelDataTypeField> joinFields,
int nTotalFields,
List<RelDataTypeField> rightFields,
RexNode filter) {
RexNode filter,
RelNode child) {
int[] adjustments = new int[nTotalFields];
for (int i = start; i < end; i++) {
adjustments[i] = offset;
Expand All @@ -3150,7 +3155,9 @@ private static RexNode shiftFilter(
rexBuilder,
joinFields,
rightFields,
adjustments));
adjustments,
offset,
child));
}

/**
Expand Down Expand Up @@ -4752,6 +4759,17 @@ public ImmutableBitSet build() {
}
return super.visitCall(call);
}

@Override public Void visitSubQuery(RexSubQuery subQuery) {
final Set<CorrelationId> variablesSet = RelOptUtil.getVariablesUsed(subQuery.rel);
for (CorrelationId id : variablesSet) {
ImmutableBitSet requiredColumns = RelOptUtil.correlationColumns(id, subQuery.rel);
for (int index : requiredColumns) {
bitBuilder.set(index);
}
}
return super.visitSubQuery(subQuery);
}
}

/**
Expand All @@ -4766,6 +4784,8 @@ public static class RexInputConverter extends RexShuttle {
private final @Nullable List<RelDataTypeField> rightDestFields;
private final int nLeftDestFields;
private final int[] adjustments;
private final int offset;
private final @Nullable RelNode correlateVariableChild;

/**
* Creates a RexInputConverter.
Expand All @@ -4784,14 +4804,23 @@ public static class RexInputConverter extends RexShuttle {
* @param rightDestFields in the case where the destination is a join,
* these are the fields from the right join input
* @param adjustments the amount to adjust each field by
* @param offset the amount to shift field accesses by when
* rewriting correlated subqueries
* @param correlateVariableChild the child relation providing the
Copy link
Copy Markdown
Member

@caicancai caicancai Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code comment format here is strange.

Copy link
Copy Markdown
Author

@yashlimbad yashlimbad Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the variable name is big here, that's why it's going into it's doc. any suggestions on formatting?

Copy link
Copy Markdown
Member

@caicancai caicancai Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I'll take a look at other Calcite code later to see if there are any good solutions.

Copy link
Copy Markdown
Member

@caicancai caicancai Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps changing to a shorter, more concise variable name would solve the problem. 🤔

Copy link
Copy Markdown
Author

@yashlimbad yashlimbad Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, will get back on this tomorrow

Copy link
Copy Markdown
Contributor

@mihaibudiu mihaibudiu Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using a shorter variable name to make formatting of comments nice is not a good reason.

Copy link
Copy Markdown
Member

@caicancai caicancai Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reminder, I'll look into whether there's a better way to handle this tomorrow.

* correlated variable; if non-null, subqueries
* referencing a correlation variable will have
* their field accesses shifted by {@code offset}
* relative to this child
*/
private RexInputConverter(
RexBuilder rexBuilder,
@Nullable List<RelDataTypeField> srcFields,
@Nullable List<RelDataTypeField> destFields,
@Nullable List<RelDataTypeField> leftDestFields,
@Nullable List<RelDataTypeField> rightDestFields,
int[] adjustments) {
int[] adjustments,
int offset,
@Nullable RelNode correlateVariableChild) {
this.rexBuilder = rexBuilder;
this.srcFields = srcFields;
this.destFields = destFields;
Expand All @@ -4804,6 +4833,8 @@ private RexInputConverter(
assert destFields == null;
nLeftDestFields = leftDestFields.size();
}
this.offset = offset;
this.correlateVariableChild = correlateVariableChild;
}

public RexInputConverter(
Expand All @@ -4818,22 +4849,61 @@ 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]) {
Copy link
Copy Markdown
Member

@caicancai caicancai Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect there might be an issue with the EXISTS subquery, but I'm not entirely sure. Could you add a similar test?

Copy link
Copy Markdown
Author

@yashlimbad yashlimbad Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checking

Copy link
Copy Markdown
Author

@yashlimbad yashlimbad Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EXISTS fails similar to IN clause, nice catch! thanks for this. will work on it

Copy link
Copy Markdown
Author

@yashlimbad yashlimbad Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed and added test

subQuery = subQuery.clone(subQuery.getType(), clonedOperands);
}
final Set<CorrelationId> variablesSet =
RelOptUtil.getVariablesUsed(subQuery.rel);
if (!variablesSet.isEmpty() && correlateVariableChild != null) {
for (CorrelationId id : 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);
}
});
if (newSubQueryRel != subQuery.rel) {
subQuery = subQuery.clone(newSubQueryRel);
}
}
}
return subQuery;
}

@Override public RexNode visitInputRef(RexInputRef var) {
Expand Down
28 changes: 26 additions & 2 deletions core/src/main/java/org/apache/calcite/rel/rules/FilterJoinRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.apache.calcite.plan.RelOptUtil;
import org.apache.calcite.plan.RelRule;
import org.apache.calcite.rel.RelNode;
import org.apache.calcite.rel.core.CorrelationId;
import org.apache.calcite.rel.core.Filter;
import org.apache.calcite.rel.core.Join;
import org.apache.calcite.rel.core.JoinRelType;
Expand All @@ -29,7 +30,9 @@
import org.apache.calcite.rex.RexCall;
import org.apache.calcite.rex.RexInputRef;
import org.apache.calcite.rex.RexNode;
import org.apache.calcite.rex.RexSubQuery;
import org.apache.calcite.rex.RexUtil;
import org.apache.calcite.rex.RexVisitorImpl;
import org.apache.calcite.sql.SqlKind;
import org.apache.calcite.sql.fun.SqlStdOperatorTable;
import org.apache.calcite.tools.RelBuilder;
Expand Down Expand Up @@ -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();

// create the new join node referencing the new children and
// containing its new join filters (if there are any)
Expand Down
Loading
Loading