xumingming commented on code in PR #58269:
URL: https://github.com/apache/spark/pull/58269#discussion_r3935292649
##########
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:
@cloud-fan Thanks for the review.
I'd like to first pin down the threat model we're signing up for, because I
believe the two masking shapes it describes cannot occur in the plans InlineCTE
actually sees, and I want to make sure I'm not missing an
in-contract path.
1. Project self-masking. This requires a bare (un-aliased) OuterReference
in a node's projectList, because OuterReference.toAttribute passes through the
wrapped exprId. But the analyzer never leaves a bare OuterReference in a
projectList or
aggregate expression: aliasIfOuterReference (see the doc comment at
ColumnResolutionHelper.scala) wraps it in an Alias precisely because "an
attribute's ExprId leaks through the operator's output", and Alias.toAttribute
carries the
alias's own exprId, so nothing leaks. Ordering also helps: InlineCTE
runs in the "Inline CTE" batch, before "Pullup Correlated Expressions", so no
optimizer-created OuterReference-carrying Projects exist at validation time
either. So a
Project cannot "mirror" an escaping reference's exprId in any plan
produced by the analyzer.
2. Sibling duplicate-exprId masking. Within a single post-analysis plan,
duplicate exprIds can't occur: every resolution allocates a fresh exprId, and
whenever already-analyzed plans are embedded into a new query (temp views
created from
DataFrames, cached plans re-injected by CacheManager, LogicalRDD from
checkpoint(), DSv2 command rewrites), the embedding happens at the input of an
analysis pass and DeduplicateRelations rewrites duplicated relation instances
with fresh
exprIds before the optimizer runs. The PR's code comment documents
exactly this residual case: plans stitched together after analysis, bypassing
it.
Given that, both masking shapes require a plan that bypassed analysis —
outside the contract the optimizer operates under. Notably, real analyzed SQL
can't produce an escaping reference inside a CTE def either (a def body can't
see the main
query's relations — the negative tests in this PR are hand-built or
stitched for exactly that reason), so this validator is defense-in-depth
against out-of-contract plans either way. The question is how airtight it must
be against
plan-surgery shapes:
- If the standard is "fail-fast guard for well-formed (analyzed) plans",
I'd argue the current design is sufficient, with the documented residual gap.
- If the standard is "must catch even hand-stitched trees that bypass
analysis", I'm happy to implement the scope-aware validation (walk the def
carrying the enclosing scope downward, check each
OuterReference/OuterScopeReference against its
owning scope instead of a definition-wide union), which subsumes both
masking vectors, plus regressions for both shapes.
Could you let me know which contract you'd prefer? If you'd rather go with
the scope-aware version outright, I'll take that direction.
--
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]
