cloud-fan commented on code in PR #58269:
URL: https://github.com/apache/spark/pull/58269#discussion_r3853304493
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/InlineCTE.scala:
##########
@@ -89,30 +89,48 @@ case class InlineCTE(
}
private def validateNoOuterReferencesAcrossCTEBoundary(cteDef:
CTERelationDef): Unit = {
- val outerRefs = cteDef.child.flatMap(
- _.expressions.flatMap(_.collect { case o: OuterReference => o }))
- if (outerRefs.nonEmpty) {
- throw SparkException.internalError(
- "A force-materialized CTE cannot carry an outer reference across its
boundary, but " +
- s"found outer reference '${outerRefs.head.name}' in the CTE
definition " +
- s"(cteId=${cteDef.id}).")
- }
+ // Only an outer reference that actually escapes the CTE definition is
invalid. An outer
+ // reference that resolves to an operator inside the definition body (e.g.
a correlated
+ // subquery whose correlated column lives in the body) is self-contained
and safe to
+ // materialize. So collect every attribute bound anywhere in the
definition (including in
+ // nested subquery plans) and reject only references that resolve to none
of them.
+ val boundExprIds = cteDef.child
+ .collectWithSubqueries { case n: LogicalPlan => n }
+ .flatMap(_.output.filter(_.resolved).map(_.exprId))
+ .toSet
- val outerScopeSubqueries = cteDef.child.flatMap(
- _.expressions.flatMap(_.collect {
- case s: SubqueryExpression if s.outerScopeAttrs.nonEmpty => s
+ // Walk the whole definition (main tree plus nested subquery plans).
Locate either a direct
+ // `OuterReference` or a correlated subquery whose outer-scope attributes
escape the def --
+ // i.e. resolve to none of `boundExprIds`.
+ val allNodes = cteDef.child.collectWithSubqueries { case n: LogicalPlan =>
n }
Review Comment:
**Non-blocking:**
Reuse the first `collectWithSubqueries` result here and derive
`boundExprIds` from it. This helper materializes the full CTE and
nested-subquery node sequence, so the second call repeats a plan-size traversal
for every force-materialized definition.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/InlineCTE.scala:
##########
@@ -89,30 +89,48 @@ case class InlineCTE(
}
private def validateNoOuterReferencesAcrossCTEBoundary(cteDef:
CTERelationDef): Unit = {
- val outerRefs = cteDef.child.flatMap(
- _.expressions.flatMap(_.collect { case o: OuterReference => o }))
- if (outerRefs.nonEmpty) {
- throw SparkException.internalError(
- "A force-materialized CTE cannot carry an outer reference across its
boundary, but " +
- s"found outer reference '${outerRefs.head.name}' in the CTE
definition " +
- s"(cteId=${cteDef.id}).")
- }
+ // Only an outer reference that actually escapes the CTE definition is
invalid. An outer
+ // reference that resolves to an operator inside the definition body (e.g.
a correlated
Review Comment:
**Nit:**
Please say that the outer reference points to an attribute produced by an
operator inside the definition. Catalyst outer references resolve to
attributes, and this validator compares their ExprIds.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/InlineCTESuite.scala:
##########
@@ -109,6 +109,74 @@ class InlineCTESuite extends PlanTest {
"found a subquery with outer-scope reference"))
}
+ test("SPARK-58006: forceSkipInline CTE with an internal outer reference is
materialized") {
+ // Corresponds to: WITH t AS (SELECT a FROM r WHERE EXISTS (SELECT 1 FROM
s WHERE s.k = t.a))
Review Comment:
**Nit:**
Please change `t.a` to `r.a` here and at lines 134 and 136. `t` is not in
scope inside its own CTE definition; both test plans correlate to
`relation.output.head` from `r`.
--
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]
