You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "suibianwanwank (via GitHub)" <gi...@apache.org> on 2025/02/13 15:50:35 UTC
[PR] [CALCITE-6824] JOIN_SUB_QUERY_TO_CORRELATE rule produces an incorrect… [calcite]
suibianwanwank opened a new pull request, #4195:
URL: https://github.com/apache/calcite/pull/4195
… plan when operands are not empty
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6824] JOIN_SUB_QUERY_TO_CORRELATE rule produces an incorrect… [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #4195:
URL: https://github.com/apache/calcite/pull/4195#discussion_r1954905949
##########
core/src/test/resources/sql/sub-query.iq:
##########
@@ -405,6 +405,78 @@ EnumerableCalc(expr#0..4=[{inputs}], EMPNO=[$t0])
EnumerableTableScan(table=[[scott, DEPT]])
!plan
+# [CALCITE-6824] JOIN_SUB_QUERY_TO_CORRELATE rule produces an incorrect plan when operands are not empty
Review Comment:
It's not only about the test per se, but also what the test harness sets up for it (what rules, in which order, etc.), there are many places where we run the test I keep forgetting what is done where tbh (even against the same dataset), but I have checked that quickly on my side and it's fine, thanks, it works OK with your patch and I can repro Julian's error without.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6824] JOIN_SUB_QUERY_TO_CORRELATE rule produces an incorrect… [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4195:
URL: https://github.com/apache/calcite/pull/4195#issuecomment-2661437890
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4195) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4195&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4195&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4195&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4195&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4195&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4195)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6824] JOIN_SUB_QUERY_TO_CORRELATE rule produces an incorrect… [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #4195:
URL: https://github.com/apache/calcite/pull/4195#discussion_r1955085885
##########
core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java:
##########
@@ -9116,6 +9116,32 @@ public interface Config extends RelRule.Config {
.check();
}
+ /**
+ * 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() {
+ final String sql = "SELECT empno FROM emp JOIN dept on "
+ + "emp.deptno not in (SELECT deptno FROM dept)";
+ sql(sql)
+ .withRule(CoreRules.JOIN_SUB_QUERY_TO_CORRELATE)
+ .check();
+ }
+
+ /**
+ * 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 testJoinSubQueryRemoveRule1() {
Review Comment:
Test names are documenting what you test is covering and should be self-descriptive, is this test specifically covering existential quantifiers as it seems? Lets call it "testJoinSubQueryRemoveRuleExistentialQuantifier" for instance. Let's do the same for the other test name.
##########
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java:
##########
@@ -910,16 +910,50 @@ private static void matchJoin(SubQueryRemoveRule rule, RelOptRuleCall call) {
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, 0);
- final RexShuttle shuttle = new ReplaceSubQueryShuttle(e, target);
- builder.join(join.getJoinType(), shuttle.apply(join.getCondition()));
- builder.project(fields(builder, join.getRowType().getFieldCount()));
+
+ ImmutableBitSet inputSet = RelOptUtil.InputFinder.bits(e.getOperands(), null);
+ 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))) {
+ return;
+ }
+
+ if (inputSet.intersects(ImmutableBitSet.range(0, nFieldsLeft))) {
+ builder.push(join.getLeft());
+
+ final Set<CorrelationId> variablesSet =
+ RelOptUtil.getVariablesUsed(e.rel);
Review Comment:
```suggestion
final Set<CorrelationId> variablesSet = RelOptUtil.getVariablesUsed(e.rel);
```
Nit: extra space removed and unnecessary newline removed, the line is short anyway IMO (I know it comes from the previous version of the code, but still)
Also, this should be taken out from the if-else as it's common to both branches.
##########
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java:
##########
@@ -910,16 +910,50 @@ private static void matchJoin(SubQueryRemoveRule rule, RelOptRuleCall call) {
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, 0);
- final RexShuttle shuttle = new ReplaceSubQueryShuttle(e, target);
- builder.join(join.getJoinType(), shuttle.apply(join.getCondition()));
- builder.project(fields(builder, join.getRowType().getFieldCount()));
+
+ ImmutableBitSet inputSet = RelOptUtil.InputFinder.bits(e.getOperands(), null);
+ 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))) {
+ return;
+ }
+
+ if (inputSet.intersects(ImmutableBitSet.range(0, nFieldsLeft))) {
Review Comment:
You could store these two intersect operations in variables named like "inputIntersectsLeftSide", it would improve readability
##########
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java:
##########
@@ -910,16 +910,50 @@ private static void matchJoin(SubQueryRemoveRule rule, RelOptRuleCall call) {
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, 0);
- final RexShuttle shuttle = new ReplaceSubQueryShuttle(e, target);
- builder.join(join.getJoinType(), shuttle.apply(join.getCondition()));
- builder.project(fields(builder, join.getRowType().getFieldCount()));
+
+ ImmutableBitSet inputSet = RelOptUtil.InputFinder.bits(e.getOperands(), null);
+ 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))) {
+ return;
+ }
+
+ if (inputSet.intersects(ImmutableBitSet.range(0, nFieldsLeft))) {
+ 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 =
+ shuttle.apply(
+ RexUtil.shift(join.getCondition(), nFieldsLeft,
+ builder.fields().size() - nFieldsLeft));
+ builder.push(join.getRight())
+ .join(join.getJoinType(), newCond);
+
+ final int nFields = builder.fields().size();
+ ImmutableList<RexNode> fields =
+ builder.fields(ImmutableBitSet.range(0, nFieldsLeft)
+ .union(ImmutableBitSet.range(nFields - nFieldsRight, nFields)));
+ builder.project(fields);
+ } else {
+ builder.push(join.getLeft()).push(join.getRight());
Review Comment:
```suggestion
builder.push(join.getLeft());
builder.push(join.getRight());
```
let's preserve the original form which is a bit more clear IMO
##########
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java:
##########
@@ -910,16 +910,50 @@ private static void matchJoin(SubQueryRemoveRule rule, RelOptRuleCall call) {
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, 0);
- final RexShuttle shuttle = new ReplaceSubQueryShuttle(e, target);
- builder.join(join.getJoinType(), shuttle.apply(join.getCondition()));
- builder.project(fields(builder, join.getRowType().getFieldCount()));
+
+ ImmutableBitSet inputSet = RelOptUtil.InputFinder.bits(e.getOperands(), null);
+ 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))) {
+ return;
+ }
+
+ if (inputSet.intersects(ImmutableBitSet.range(0, nFieldsLeft))) {
+ 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 =
+ shuttle.apply(
+ RexUtil.shift(join.getCondition(), nFieldsLeft,
+ builder.fields().size() - nFieldsLeft));
+ builder.push(join.getRight())
+ .join(join.getJoinType(), newCond);
Review Comment:
nit: the fluent notation hides some information here IMO, what about this similar to the original version?
```suggestion
builder.push(join.getRight());
builder.join(join.getJoinType(), newCond);
```
##########
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java:
##########
@@ -910,16 +910,50 @@ private static void matchJoin(SubQueryRemoveRule rule, RelOptRuleCall call) {
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, 0);
- final RexShuttle shuttle = new ReplaceSubQueryShuttle(e, target);
- builder.join(join.getJoinType(), shuttle.apply(join.getCondition()));
- builder.project(fields(builder, join.getRowType().getFieldCount()));
+
+ ImmutableBitSet inputSet = RelOptUtil.InputFinder.bits(e.getOperands(), null);
+ int nFieldsLeft = join.getLeft().getRowType().getFieldCount();
+ int nFieldsRight = join.getRight().getRowType().getFieldCount();
+
+ if (inputSet.intersects(ImmutableBitSet.range(0, nFieldsLeft))
Review Comment:
Can you produce a SQL example of this situation, ideally in a test?
I think this comment could be added to the code, if you thought it was needed for a reviewer, it will be probably needed for future readers too.
I'd like the comment to provide a bit more context into why we bail out in this case.
##########
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java:
##########
@@ -910,16 +910,50 @@ private static void matchJoin(SubQueryRemoveRule rule, RelOptRuleCall call) {
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, 0);
- final RexShuttle shuttle = new ReplaceSubQueryShuttle(e, target);
- builder.join(join.getJoinType(), shuttle.apply(join.getCondition()));
- builder.project(fields(builder, join.getRowType().getFieldCount()));
+
+ ImmutableBitSet inputSet = RelOptUtil.InputFinder.bits(e.getOperands(), null);
+ 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))) {
+ return;
+ }
+
+ if (inputSet.intersects(ImmutableBitSet.range(0, nFieldsLeft))) {
+ 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 =
+ shuttle.apply(
+ RexUtil.shift(join.getCondition(), nFieldsLeft,
+ builder.fields().size() - nFieldsLeft));
+ builder.push(join.getRight())
+ .join(join.getJoinType(), newCond);
+
+ final int nFields = builder.fields().size();
+ ImmutableList<RexNode> fields =
+ builder.fields(ImmutableBitSet.range(0, nFieldsLeft)
+ .union(ImmutableBitSet.range(nFields - nFieldsRight, nFields)));
+ builder.project(fields);
+ } else {
+ builder.push(join.getLeft()).push(join.getRight());
+
+ final Set<CorrelationId> variablesSet =
+ RelOptUtil.getVariablesUsed(e.rel);
+ final RexNode target =
+ rule.apply(e, variablesSet, logic, builder, 2, join.getRowType().getFieldCount(), 0);
+ final RexShuttle shuttle = new ReplaceSubQueryShuttle(e, target);
+
+ builder.join(join.getJoinType(), shuttle.apply(join.getCondition()));
+ builder.project(fields(builder, join.getRowType().getFieldCount()));
Review Comment:
Can you store the value of `join.getRowType().getFieldCount()` into a variable like in the original version?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6824] JOIN_SUB_QUERY_TO_CORRELATE rule produces an incorrect… [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #4195:
URL: https://github.com/apache/calcite/pull/4195#discussion_r1954905949
##########
core/src/test/resources/sql/sub-query.iq:
##########
@@ -405,6 +405,78 @@ EnumerableCalc(expr#0..4=[{inputs}], EMPNO=[$t0])
EnumerableTableScan(table=[[scott, DEPT]])
!plan
+# [CALCITE-6824] JOIN_SUB_QUERY_TO_CORRELATE rule produces an incorrect plan when operands are not empty
Review Comment:
It's not only about the test per se, but also what the test harness sets up for it (what rules, in which order, etc.), there are many places where we run the test I keep forgetting what is done where tbh (even against the same dataset), but I have checked that quickly on my side and it's fine, thanks!
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6824] JOIN_SUB_QUERY_TO_CORRELATE rule produces an incorrect… [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on PR #4195:
URL: https://github.com/apache/calcite/pull/4195#issuecomment-2657084497
@suibianwanwank just a general comment, could you see if the repro from Julian is also fixed by your proposal?
Note: I am not suggesting to add it to this PR, but it's important to make sure we didn't alter the reproducers by moving them elsewhere.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6824] JOIN_SUB_QUERY_TO_CORRELATE rule produces an incorrect… [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6824] JOIN_SUB_QUERY_TO_CORRELATE rule produces an incorrect… [calcite]
Posted by "suibianwanwank (via GitHub)" <gi...@apache.org>.
suibianwanwank commented on code in PR #4195:
URL: https://github.com/apache/calcite/pull/4195#discussion_r1954823813
##########
core/src/test/resources/sql/sub-query.iq:
##########
@@ -405,6 +405,78 @@ EnumerableCalc(expr#0..4=[{inputs}], EMPNO=[$t0])
EnumerableTableScan(table=[[scott, DEPT]])
!plan
+# [CALCITE-6824] JOIN_SUB_QUERY_TO_CORRELATE rule produces an incorrect plan when operands are not empty
Review Comment:
@asolimando I used the test given by Julian here. If required I can add it to JdbcTest as well.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6824] JOIN_SUB_QUERY_TO_CORRELATE rule produces an incorrect… [calcite]
Posted by "suibianwanwank (via GitHub)" <gi...@apache.org>.
suibianwanwank commented on code in PR #4195:
URL: https://github.com/apache/calcite/pull/4195#discussion_r1954776134
##########
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java:
##########
@@ -910,16 +910,50 @@ private static void matchJoin(SubQueryRemoveRule rule, RelOptRuleCall call) {
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, 0);
- final RexShuttle shuttle = new ReplaceSubQueryShuttle(e, target);
- builder.join(join.getJoinType(), shuttle.apply(join.getCondition()));
- builder.project(fields(builder, join.getRowType().getFieldCount()));
+
+ ImmutableBitSet inputSet = RelOptUtil.InputFinder.bits(e.getOperands(), null);
+ int nFieldsLeft = join.getLeft().getRowType().getFieldCount();
+ int nFieldsRight = join.getRight().getRowType().getFieldCount();
+
+ if (inputSet.intersects(ImmutableBitSet.range(0, nFieldsLeft))
Review Comment:
The original rewrite could not support inputRef on either side of a join.
##########
core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java:
##########
@@ -9116,6 +9116,32 @@ public interface Config extends RelRule.Config {
.check();
}
+ /**
+ * 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() {
+ final String sql = "SELECT empno FROM emp JOIN dept on "
+ + "emp.deptno not in (SELECT deptno FROM dept)";
+ sql(sql)
+ .withRule(CoreRules.JOIN_SUB_QUERY_TO_CORRELATE)
+ .check();
+ }
+
+ /**
+ * 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 testJoinSubQueryRemoveRule1() {
Review Comment:
These two tests throw assert exceptions before this PR.
##########
core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java:
##########
@@ -910,16 +910,50 @@ private static void matchJoin(SubQueryRemoveRule rule, RelOptRuleCall call) {
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, 0);
- final RexShuttle shuttle = new ReplaceSubQueryShuttle(e, target);
- builder.join(join.getJoinType(), shuttle.apply(join.getCondition()));
- builder.project(fields(builder, join.getRowType().getFieldCount()));
+
+ ImmutableBitSet inputSet = RelOptUtil.InputFinder.bits(e.getOperands(), null);
+ 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))) {
+ return;
+ }
+
+ if (inputSet.intersects(ImmutableBitSet.range(0, nFieldsLeft))) {
+ 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 =
+ shuttle.apply(
+ RexUtil.shift(join.getCondition(), nFieldsLeft,
+ builder.fields().size() - nFieldsLeft));
+ builder.push(join.getRight())
+ .join(join.getJoinType(), newCond);
+
+ final int nFields = builder.fields().size();
+ ImmutableList<RexNode> fields =
+ builder.fields(ImmutableBitSet.range(0, nFieldsLeft)
+ .union(ImmutableBitSet.range(nFields - nFieldsRight, nFields)));
+ builder.project(fields);
+ } else {
+ builder.push(join.getLeft()).push(join.getRight());
+
+ final Set<CorrelationId> variablesSet =
+ RelOptUtil.getVariablesUsed(e.rel);
+ final RexNode target =
+ rule.apply(e, variablesSet, logic, builder, 2, join.getRowType().getFieldCount(), 0);
+ final RexShuttle shuttle = new ReplaceSubQueryShuttle(e, target);
Review Comment:
If inputRef is on the right side of the join, or constants. Follow the original processing.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6824] JOIN_SUB_QUERY_TO_CORRELATE rule produces an incorrect… [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando merged PR #4195:
URL: https://github.com/apache/calcite/pull/4195
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6824] JOIN_SUB_QUERY_TO_CORRELATE rule produces an incorrect… [calcite]
Posted by "suibianwanwank (via GitHub)" <gi...@apache.org>.
suibianwanwank commented on PR #4195:
URL: https://github.com/apache/calcite/pull/4195#issuecomment-2661433141
@asolimando Done.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6824] JOIN_SUB_QUERY_TO_CORRELATE rule produces an incorrect… [calcite]
Posted by "suibianwanwank (via GitHub)" <gi...@apache.org>.
suibianwanwank commented on PR #4195:
URL: https://github.com/apache/calcite/pull/4195#issuecomment-2658413157
@asolimando Thank you for your detailed review! I have addressed and fixed the issues you mentioned. If I missed anything or made any mistakes, please let me know.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org