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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
