asolimando commented on code in PR #4195:
URL: https://github.com/apache/calcite/pull/4195#discussion_r1957082992
##########
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java:
##########
@@ -915,43 +915,50 @@ private static void matchJoin(SubQueryRemoveRule rule,
RelOptRuleCall call) {
int nFieldsLeft = join.getLeft().getRowType().getFieldCount();
int nFieldsRight = join.getRight().getRowType().getFieldCount();
- if (inputSet.intersects(ImmutableBitSet.range(0, nFieldsLeft))
- && inputSet.intersects(ImmutableBitSet.range(nFieldsLeft, nFieldsLeft
+ nFieldsRight))) {
+
+ ImmutableBitSet inputIntersectsLeftSide = ImmutableBitSet.range(0,
nFieldsLeft);
Review Comment:
What is meant is to save the boolean value directly in a final variable, if
you prefer to keep this form let's at least adapt the name of the variables to
`leftInputFields`/`rightInputFields` and make them final.
##########
core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java:
##########
@@ -9119,9 +9119,9 @@ public interface Config extends RelRule.Config {
/**
* Test case for
* <a
href="https://issues.apache.org/jira/browse/CALCITE-6824";>[CALCITE-6824]
- * JOIN_SUB_QUERY_TO_CORRELATE rule produces an incorrect plan
- * when operands are not empty</a>. */
- @Test void testJoinSubQueryRemoveRule() {
+ * Subquery in join conditions rewrite fails if referencing a column
+ * from the right-hand side table</a>. */
+ @Test void testJoinSubQueryRemoveRuleWithQuantifierNotIn() {
Review Comment:
```suggestion
@Test void testJoinSubQueryRemoveRuleWithNotIn() {
```
##########
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java:
##########
@@ -915,43 +915,50 @@ private static void matchJoin(SubQueryRemoveRule rule,
RelOptRuleCall call) {
int nFieldsLeft = join.getLeft().getRowType().getFieldCount();
int nFieldsRight = join.getRight().getRowType().getFieldCount();
- if (inputSet.intersects(ImmutableBitSet.range(0, nFieldsLeft))
- && inputSet.intersects(ImmutableBitSet.range(nFieldsLeft, nFieldsLeft
+ nFieldsRight))) {
+
+ ImmutableBitSet inputIntersectsLeftSide = ImmutableBitSet.range(0,
nFieldsLeft);
+ ImmutableBitSet inputIntersectsRightSide =
+ ImmutableBitSet.range(nFieldsLeft, nFieldsLeft + nFieldsRight);
+ if (inputSet.intersects(inputIntersectsLeftSide)
+ && inputSet.intersects(inputIntersectsRightSide)) {
+ // The current existential rewrite needs to make join with one side of
the origin join and
+ // generate a new condition to replace the on clause. But for RexNode
whose operands are
+ // on either side of the join, we can't push them into join. So this
rewriting is not
+ // supported.
return;
}
- if (inputSet.intersects(ImmutableBitSet.range(0, nFieldsLeft))) {
+ final Set<CorrelationId> variablesSet =
RelOptUtil.getVariablesUsed(e.rel);
Review Comment:
```suggestion
final Set<CorrelationId> variablesSet =
RelOptUtil.getVariablesUsed(e.rel);
```
Nit: double space
##########
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java:
##########
@@ -915,43 +915,50 @@ private static void matchJoin(SubQueryRemoveRule rule,
RelOptRuleCall call) {
int nFieldsLeft = join.getLeft().getRowType().getFieldCount();
int nFieldsRight = join.getRight().getRowType().getFieldCount();
- if (inputSet.intersects(ImmutableBitSet.range(0, nFieldsLeft))
- && inputSet.intersects(ImmutableBitSet.range(nFieldsLeft, nFieldsLeft
+ nFieldsRight))) {
+
+ ImmutableBitSet inputIntersectsLeftSide = ImmutableBitSet.range(0,
nFieldsLeft);
+ ImmutableBitSet inputIntersectsRightSide =
+ ImmutableBitSet.range(nFieldsLeft, nFieldsLeft + nFieldsRight);
+ if (inputSet.intersects(inputIntersectsLeftSide)
+ && inputSet.intersects(inputIntersectsRightSide)) {
+ // The current existential rewrite needs to make join with one side of
the origin join and
+ // generate a new condition to replace the on clause. But for RexNode
whose operands are
+ // on either side of the join, we can't push them into join. So this
rewriting is not
+ // supported.
return;
}
- if (inputSet.intersects(ImmutableBitSet.range(0, nFieldsLeft))) {
+ final Set<CorrelationId> variablesSet =
RelOptUtil.getVariablesUsed(e.rel);
+ if (inputSet.intersects(inputIntersectsLeftSide)) {
builder.push(join.getLeft());
- final Set<CorrelationId> variablesSet =
- RelOptUtil.getVariablesUsed(e.rel);
final RexNode target =
rule.apply(e, variablesSet, logic, builder, 1, nFieldsLeft, 0);
final RexShuttle shuttle = new ReplaceSubQueryShuttle(e, target);
- RexNode newCond =
+ final RexNode newCond =
shuttle.apply(
RexUtil.shift(join.getCondition(), nFieldsLeft,
builder.fields().size() - nFieldsLeft));
- builder.push(join.getRight())
- .join(join.getJoinType(), newCond);
+ builder.push(join.getRight());
+ builder.join(join.getJoinType(), newCond);
final int nFields = builder.fields().size();
ImmutableList<RexNode> fields =
- builder.fields(ImmutableBitSet.range(0, nFieldsLeft)
Review Comment:
Now I see why you are saving the `ImmutableBitSet`, but I think it's better
to save the boolean value of the intersect and rebuild the range here (so
basically reverting this part of the change).
##########
core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml:
##########
@@ -6285,7 +6285,7 @@ LogicalProject(SAL=[$5])
]]>
</Resource>
</TestCase>
- <TestCase name="testJoinSubQueryRemoveRule">
+ <TestCase name="testJoinSubQueryRemoveRuleWithQuantifierNotIn">
Review Comment:
```suggestion
<TestCase name="testJoinSubQueryRemoveRuleWithNotIn">
```
--
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]
