cloud-fan commented on code in PR #58269:
URL: https://github.com/apache/spark/pull/58269#discussion_r3933711073
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/InlineCTE.scala:
##########
@@ -89,29 +89,55 @@ case class InlineCTE(
}
private def validateNoOuterReferencesAcrossCTEBoundary(cteDef:
CTERelationDef): Unit = {
- val outerRefs = cteDef.child.flatMap(
- _.expressions.flatMap(_.collect { case o: OuterReference => o }))
- if (outerRefs.nonEmpty) {
+ // Only an outer reference that actually escapes the CTE definition is
invalid. An outer
+ // reference that points to an attribute produced by an operator inside
the definition
+ // body (e.g. a correlated subquery whose correlated column lives in the
body) does not
+ // escape, so the definition is self-contained and safe to materialize.
Walk the whole
+ // definition (main tree plus nested subquery plans) once, collect every
attribute bound
+ // anywhere in the definition, and reject only references that resolve to
none of them.
+ //
+ // Matching by exprId is exact for plans produced by a single analysis
pass: every
+ // `OuterReference` wraps the very attribute it binds to, exprIds are
allocated fresh
+ // per resolution, and `DeduplicateRelations` rewrites duplicated relation
instances
+ // (self-join, union, etc.) with fresh exprIds before the optimizer runs.
The known
+ // residual gap is a plan stitched together from already-analyzed plans
(e.g. a producer
+ // embedding the same analyzed dataset twice): such a tree can carry the
same exprId in
+ // multiple places, so an escaping reference could match a duplicated
exprId and slip
+ // through. Such a query is ill-formed and fails later during planning or
execution
+ // anyway; this check is defense-in-depth that fails fast with a clear
error otherwise.
+ val allNodes = cteDef.child.collectWithSubqueries { case n: LogicalPlan =>
n }
+ val boundExprIds = allNodes.iterator
+ .flatMap(_.output.filter(_.resolved).map(_.exprId))
Review Comment:
**Blocking (P1):** `boundExprIds` includes every plan node's output, but
`Project.output` is derived from `projectList.toAttribute`, and
`OuterReference.toAttribute` preserves the wrapped attribute's `exprId`. A
nested project can therefore insert an escaping reference's own ID into this
set and make the predicate accept it; the same definition-wide union also lets
an unrelated sibling with a duplicate `exprId` mask the escape. The CTE is then
materialized despite depending on an operator outside its boundary, so the
intended `INTERNAL_ERROR` is deferred to later optimization or planning.
**Recommended change:** Make this validation scope-aware: only genuine
producers in an outer reference's enclosing scope inside the CTE may bind it,
and add regressions for both a Project that projects the escaping
OuterReference and an unrelated sibling that reuses the same exprId.
**Why this works:** Walk the CTE definition while carrying lexical binder
scopes from genuine operator inputs and children, then validate each
OuterReference or OuterScopeReference against the appropriate enclosing scope
instead of a definition-wide union of plan outputs.
**Scope:** InlineCTE.validateNoOuterReferencesAcrossCTEBoundary and focused
Catalyst and SQL CTE-inline regressions covering the two masking shapes.
**Compatibility:** Preserve acceptance of genuinely self-contained
correlations and the existing INTERNAL_ERROR contract for references whose
producer is outside the CTE, without changing unrelated InlineCTE behavior.
**Risks:** An incorrect scope walk could reject a valid internal correlation
or mis-handle nested subquery scope. Continuing to use exprId membership
without lexical ownership would remain vulnerable to reused analyzed fragments.
**Constraints:** Keep unresolved outputs excluded as the current
implementation does. Validate both OuterReference and OuterScopeReference
across nested subqueries. Cover one direct Project self-masking case and one
real-analyzer sibling duplicate-exprId case.
**Success:** Both escaping-reference shapes fail at the CTE boundary with
INTERNAL_ERROR, while the existing self-contained OuterReference and
OuterScopeReference cases still materialize successfully.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
