[CALCITE-7821] Correlated query crashes SqlToRelConverter

[mihaibudiu]

Jira Link

CALCITE-7821

Changes Proposed

SqlToRelConverter.translateIn was assuming that the Join nodes are never Correlates. The changes should handle both Join and Correlate nodes uniformly.

[vlsi]

The fix works for the query in the test: it fails on main with AssertionError: contains $cor0 and passes here. I ran seven correlated NOT IN queries with expand=true and compared their results with expand=false, and all seven matched. They covered multi-column keys, the SELECT list, two NOT IN in one WHERE, nesting inside EXISTS, NOT (x IN …), and an expression key. Three other points are inline: the same crash in HAVING, a comment that became wrong once it moved above the switch, and missing test shapes.

The ON clause is broken separately, and this PR only moves the crash. On this branch, select * from emp as e join dept as d on e.mgr not in (select s.mgr from emp as s where s.deptno = d.deptno) now fails in the new createJoin with Required columns {9} not subset of left columns {0, 1, 2, 3, 4, 5, 6, 7, 8}. The uncorrelated on e.mgr not in (select s.mgr from emp as s) already fails on main with ArrayIndexOutOfBoundsException: Index 6 out of bounds for length 4. The cause is that the count join is not added to bb.registered, so convertOnCondition drops it when it calls reRegister(rightRel). That is probably a separate JIRA, but then the description should not say that both node kinds are handled "uniformly".

About the description: on main the crash is not in translateIn. It happens in Blackboard.register → LogicalCorrelate.isValid, because the count join was built with LogicalJoin.create and left $cor0 unbound in its right input. Suggested wording: "The count sub-query that NOT IN joins to the outer query was built with LogicalJoin.create, which leaves its correlation variable unbound, so SqlToRelConverter failed with "contains $cor0" when it correlated the second join. The count join is now built by createJoin, which returns a Correlate for a correlated sub-query, and translateIn accepts either join kind."

[vlsi]

The same crash still happens when the outer query aggregates:

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:

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?

[vlsi]

Suggested change

	// createJoin will create a Correlate node if a sub-query is correlated
	// 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.

[vlsi]

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?

[vlsi]

Nit: "Either join is a Correlate when the sub-query is correlated." is more precise. "The joins may be" leaves it unclear which joins.

[vlsi]

Suggested change

	// Read the left keys from rex; if 'join' is a Correlate
	// it has no join condition
	// Take the IN keys from rex rather than from the join condition,
	// because a Correlate has no condition.

[vlsi]

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.

[vlsi]

The tests cover one variant of each thing this change touched. These cases are missing:

1. Composite key. The change reads the IN keys from rex instead of joinInfo.leftKeys, and the two differ once there is more than one key. Both new tests use a single column. For example: select * from emp as t where (t.mgr, t.deptno) not in (select s.mgr, s.deptno from emp as s where s.empno = t.empno).
2. The plan before decorrelation. testNotInCorrelatedNullable only snapshots the decorrelated plan, so no reference file has the two nested LogicalCorrelate nodes that translateIn now has to handle. A .withDecorrelate(false) variant of the same query would add one.
3. Other clauses than WHERE. The plan test covers only WHERE, and the SELECT list is covered only in sub-query.iq. HAVING fails (see the inline comment on bb.setRoot(join, false)), and it needs a test once it is fixed: 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).
4. Two correlated NOT IN in one query. translateIn casts root to BiRel and its left input to Project, so it depends on the shape of the tree. Two sub-queries chained one after the other put the most weight on that shape. For example: select * from emp as t where t.mgr not in (select s.mgr from emp as s where s.empno = t.empno) and t.comm not in (select s.comm from emp as s where s.ename = t.ename). I checked that the plan is valid and that expand=true returns the same rows as expand=false, but no test guards it.

[mihaibudiu]

Some of these extra tests would be nice, but they are not really in scope of the issue being solved.
Once the issue is solved I plan to file at least one more issue about another failure which was uncovered by testing, but which needs the current fix.

[vlsi]

Thanks for the HAVING fix and for taking the comment suggestions. 8f2ab7d adds the mapRootRelToFieldProjection entry and testNotInCorrelatedNullableHaving, which covers that case.

You wrote: "Some of these extra tests would be nice, but they are not really in scope of the issue being solved." Which of the four do you consider out of scope, and why? I agree for some of them and not for others, so I'd like to understand where you draw the line. Here is my view of each:

1. Composite key. I think this one is in scope. The PR replaces joinInfo.leftKeys with the first keyCount fields of rex, and with a single key the test cannot tell whether that count or the order of the keys is right. One query with (t.mgr, t.deptno) not in (...) would cover it.
2. Plan before decorrelation. I think this one is in scope too. The new code in translateIn walks two nested LogicalCorrelate nodes, but the only snapshot is the decorrelated plan, which has no LogicalCorrelate at all. A second test method with the same query and .withDecorrelate(false) would record the shape the fix handles.
3. HAVING. Done, thanks.
4. Two correlated NOT IN in one query. I agree this one can go to a later change. translateIn depended on the shape of the tree before this PR as well, so the test belongs with a change that touches that dependency.

On the ON clause: I agree it is a separate issue. Is it the follow-up you plan to file? If not, I can file it.

The PR description still says that translateIn "was assuming that the Join nodes are never Correlates" and that the change handles "both Join and Correlate nodes uniformly". On main the crash happens earlier, in Blackboard.register → Correlate.isValid, and the ON clause still fails, so neither sentence matches the change. My review has a suggested wording, if it helps.

[mihaibudiu]

The composite key program returns wrong results, so it's a separate issue I need to file.
I will add a plan before decorrelation.

[mihaibudiu]

@vlsi I have pushed another commit with an answer to your request 2.
Request 1 unveils a different bug, so it cannot be resolved in this PR.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud