Dwrite commented on code in PR #5036:
URL: https://github.com/apache/calcite/pull/5036#discussion_r3566443105
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##########
@@ -547,9 +552,97 @@ public Result visit(Correlate e) {
return result(join, leftResult, rightResult);
}
+ /**
+ * Returns whether any expression (including nested sub-queries) references
a correlated variable
+ * defined by the given input and having the same row type.
+ */
+ private static boolean inputRowTypeMatchesCorrelVariable(
+ RelNode input,
+ Set<CorrelationId> correlIds,
+ Iterable<? extends RexNode> exprs) {
+
+ if (correlIds.isEmpty()) {
+ return false;
+ }
+
+ final RelDataType inputRowType = input.getRowType();
+
+ /** Visitor that detects matching correlated variables. */
+ class Finder extends RexVisitorImpl<Void> {
+ boolean found;
+
+ Finder() {
+ super(true);
+ }
+
+ @Override public Void visitCorrelVariable(RexCorrelVariable v) {
+ if (correlIds.contains(v.id)
Review Comment:
Hi @mihaibudiu
I add new code `!(definedHere.isEmpty()) && e.getInput().getInputs().size()
<= 1`. and here's explaination.
The purpose of calling `resetAliasForCorrelation` in `visit(Filter)` /
`visit(Project)`
is narrow: give `e.getInput()` a canonical alias so that a correlated
sub-query embedded in this node's condition/projects can resolve
`$corX.field` against that alias.
This is only safe/necessary when `e.getInput()`'s rendered SQL already
represents a single addressable relation:
- `e.getInput().getInputs().size() <= 1` (TableScan, Values, Project,
Filter, Aggregate, ...): these nodes always render as a single
identity -- either a bare table reference or a freshly wrapped SELECT
derived table. Assigning an alias here is safe, and is exactly the
step the original bug was missing.
- `e.getInput().getInputs().size() > 1` (Join, Correlate): `visit(Join)`
already builds a multi-alias context via
`joinContext(leftContext, rightContext)` (e.g. `{EMP: ..., DEPT: ...}`),
keeping each side independently addressable -- which is exactly what a
correlated sub-query needs to resolve `$corX.field`. Calling
`resetAliasForCorrelation` here would collapse that multi-alias context
into a single alias, destroying field resolution that was already
correct (this is the root cause of the `DEPTNO0` leakage observed in
the FilterIntoJoinRule / FilterCorrelateRule regression cases).
Scope: this check only governs whether `visit(Filter)` / `visit(Project)`
should invoke `resetAliasForCorrelation` for the purpose of correlation
variable binding. It says nothing about the general contract of
`resetAliasForCorrelation` itself, and does not affect other call sites
(e.g. `visitAntiOrSemiJoin`, which legitimately calls it on a
multi-alias Result for an unrelated purpose -- wrapping a complex
anti-join input). That is why the check belongs at this call site rather
than as an assertion inside `resetAliasForCorrelation`: an assertion
inside the shared method would incorrectly impose this call site's
precondition on every caller.
Verified against three concrete scenarios:
- EXISTS in WHERE: e.getInput() = TableScan (0 inputs) -> pushed=true,
correct
- FilterIntoJoinRule / FilterCorrelateRule residual filter: e.getInput() =
Join
(2 inputs) -> pushed=false, correct (avoids DEPTNO0 leakage)
- foodmart correlated IN, e.getInput() = Project over Join: Project itself
has 1 input -> pushed=true, correct
--
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]
