vlsi commented on code in PR #5291:
URL: https://github.com/apache/calcite/pull/5291#discussion_r4097427558
##########
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##########
@@ -1764,29 +1765,28 @@ private static int parseFieldIdx(RexNode e) {
private RexNode translateIn(RelOptUtil.Logic logic, @Nullable RelNode root,
final RexNode rex) {
+ // RexRangeRef contains the following fields:
+ // leftKeysForIn,
+ // rightKeysForIn (the original sub-query select list),
+ // nullIndicator
+ //
+ // The first two lists contain the same number of fields.
+ assert rex instanceof RexRangeRef;
+ final int fieldCount = rex.getType().getFieldCount();
+ final int keyCount = (fieldCount - 1) / 2;
Review Comment:
This layout (`leftKeysForIn`, `rightKeysForIn`, `nullIndicator`) holds only
when `register` gets `LEFT` and `leftKeys`. For `TRUE`, and for an `INNER`
register, `rex` covers the sub-query row type alone, and `keyCount` means
nothing there. Now that the comment and `keyCount` sit above the `switch`, they
claim the layout for every branch. Could they stay inside the two branches that
use them, or state the condition?
##########
core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java:
##########
@@ -2720,6 +2720,15 @@ void checkCorrelatedMapSubQuery(boolean expand) {
sql(sql).ok();
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7821";>[CALCITE-7821]
+ * Correlated query crashes SqlToRelConverter</a>. */
+ @Test void testNotInCorrelatedNullable() {
Review Comment:
The checked-in plan is the decorrelated one, so the shape that `translateIn`
now handles (two nested `LogicalCorrelate`) is not in any snapshot. Could you
add a `.withDecorrelate(false)` variant? A multi-column case is also worth a
test, such as `(t.mgr, t.deptno) not in (select s.mgr, s.deptno from emp as s
where s.empno = t.empno)`. That case exercises the replacement of
`joinInfo.leftKeys` with `rex` fields. I checked its plan and results and found
them correct, but nothing guards it.
##########
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##########
@@ -1514,9 +1514,10 @@ private void substituteSubQuery(Blackboard bb, SubQuery
subQuery) {
AggregateCall.create(call.getParserPosition(),
SqlStdOperatorTable.COUNT, false,
false, false, ImmutableList.of(), args,
-1, null, RelCollations.EMPTY, longType, null)));
- LogicalJoin join =
- LogicalJoin.create(bb.root(), aggregate, ImmutableList.of(),
- rexBuilder.makeLiteral(true), ImmutableSet.of(),
JoinRelType.INNER);
+ // createJoin will create a Correlate node if a sub-query is correlated
+ final RelNode join =
+ createJoin(bb, bb.root(), aggregate, rexBuilder.makeLiteral(true),
+ JoinRelType.INNER);
bb.setRoot(join, false);
Review Comment:
The same crash still happens when the outer query aggregates:
```sql
select deptno from emp as e group by deptno
having deptno not in (select s.mgr from emp as s where s.deptno = e.deptno)
```
It fails with `AssertionError: Required columns {3, 7} not subset of left
columns {0, 1, 2, 3}` from `Blackboard.register` → `createJoin`. The `IN`
variant and the uncorrelated `NOT IN` variant both work.
The new count join becomes `bb.root`, but it has no entry in
`mapRootRelToFieldProjection`. As a result, `getCorrelationUse` for the second
join takes the column positions from before the aggregate. With the following
lines before `bb.setRoot`, the query produces a valid plan both with and
without decorrelation:
```java
final Map<Integer, Integer> projection =
bb.mapRootRelToFieldProjection.get(bb.root());
if (projection != null) {
bb.mapRootRelToFieldProjection.put(join, projection);
}
```
Could you add this query as a test too?
##########
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##########
@@ -1834,10 +1835,12 @@ private RexNode translateIn(RelOptUtil.Logic logic,
@Nullable RelNode root,
falseLiteral,
rexBuilder.makeCall(SqlStdOperatorTable.IS_NOT_NULL, iRef),
trueLiteral);
- final JoinInfo joinInfo = join.analyzeCondition();
- for (int leftKey : joinInfo.leftKeys) {
- final RexNode kRef = rexBuilder.makeInputRef(root, leftKey);
- args.add(rexBuilder.makeCall(SqlStdOperatorTable.IS_NULL, kRef),
+ // Read the left keys from rex; if 'join' is a Correlate
+ // it has no join condition
Review Comment:
```suggestion
// Take the IN keys from rex rather than from the join condition,
// because a Correlate has no condition.
```
##########
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##########
@@ -1811,9 +1811,10 @@ private RexNode translateIn(RelOptUtil.Logic logic,
@Nullable RelNode root,
// cross join (select count(*) as c, count(deptno) as ck from v) as ct
// left join (select distinct deptno, true as i from v) as dt
// on e.deptno = dt.deptno
- final Join join = (Join) requireNonNull(root, "root");
+ // The joins may be Correlate nodes if the sub-query is correlated.
Review Comment:
Nit: "Either join is a Correlate when the sub-query is correlated." is more
precise. "The joins may be" leaves it unclear which joins.
##########
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##########
@@ -1514,9 +1514,10 @@ private void substituteSubQuery(Blackboard bb, SubQuery
subQuery) {
AggregateCall.create(call.getParserPosition(),
SqlStdOperatorTable.COUNT, false,
false, false, ImmutableList.of(), args,
-1, null, RelCollations.EMPTY, longType, null)));
- LogicalJoin join =
- LogicalJoin.create(bb.root(), aggregate, ImmutableList.of(),
- rexBuilder.makeLiteral(true), ImmutableSet.of(),
JoinRelType.INNER);
+ // createJoin will create a Correlate node if a sub-query is correlated
Review Comment:
```suggestion
// seek may reference the outer query; createJoin then returns
// a Correlate that binds the variable.
```
The comment uses "will" for current behavior, and it describes what
`createJoin` does rather than why it is called here instead of
`LogicalJoin.create`.
--
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]
