ulysses-you commented on code in PR #58661:
URL: https://github.com/apache/spark/pull/58661#discussion_r3985469584
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/InlineCTE.scala:
##########
@@ -77,15 +83,18 @@ case class InlineCTE(
private def shouldInline(cteDef: CTERelationDef, refCount: Int): Boolean = {
// A CTE definition that requests to skip inlining is never inlined, even
in `alwaysInline`
// mode, so that a producer can guarantee the CTE is materialized rather
than duplicated.
- !cteDef.forceSkipInline && (alwaysInline || {
- // We do not need to check enclosed `CTERelationRef`s for
`deterministic` or
- // `OuterReference`, because:
- // 1) It is fine to inline a CTE if it references another CTE that is
non-deterministic;
- // 2) Any `CTERelationRef` that contains `OuterReference` would have
been inlined first.
- refCount == 1 ||
- cteDef.deterministic ||
-
cteDef.child.exists(_.expressions.exists(_.isInstanceOf[OuterReference]))
- })
+ !cteDef.forceSkipInline && (alwaysInline || (cteDef.materialized match {
Review Comment:
Non-blocking (hardening): this match bypasses the old "correlated implies
inline" disjunct for Some(true) defs, and the boundary guard at L73-74
(validateNoOuterReferencesAcrossCTEBoundary, L100-126) only runs for
forceSkipInline defs. A materialized = Some(true) def is the same hazard class:
never inlined, so an outer reference across its boundary has nothing to resolve
against.
SQL-derived plans are fully covered: both producers set the flag only from
the parser, and MaterializedCTECheck runs on both analyzers
(CheckAnalysis.scala:344, ResolverRunner.scala:40-43) under executeAndCheck.
The residual path is programmatic CTERelationDef(child, materialized =
Some(true)) construction (catalyst is developer API), the same threat model the
existing forceSkipInline guard was written for (SPARK-58006). If one slips
through, the failure is an exception, not wrong results: TEMP_2168
(DecorrelateInnerQuery.scala:1121) inside a correlated subquery, or an
unbound-attribute planner error otherwise.
Fix shape: widen the guard's match at L73-74 to
```
cteDef.forceSkipInline || cteDef.materialized.contains(true)
```
Zero behavior change for SQL plans (analysis already rejects these), and it
makes the optimizer-side invariant self-contained like its peer.
--
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]
