You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "AveryQi115 (via GitHub)" <gi...@apache.org> on 2026/07/07 18:28:16 UTC
[PR] [SPARK-58006][SQL] Add forceSkipInline flag to CTERelationDef to force CTE materialization in logical planning [spark]
AveryQi115 opened a new pull request, #57091:
URL: https://github.com/apache/spark/pull/57091
### What changes were proposed in this pull request?
This PR adds a `forceSkipInline` field to `CTERelationDef`.
When set to true, `InlineCTE` never inlines the CTE relation, even in `alwaysInline` mode or when the CTE is deterministic and referenced only once. This lets a producer guarantee that a CTE is materialized rather than duplicated.
The flag is self-gating: it defaults to false, so there is no behavior change unless a caller explicitly sets it.
Because a force-materialized CTE is evaluated on its own, it cannot carry an outer reference across its boundary (there is no surrounding operator to resolve it against after materialization). `InlineCTE` now validates this invariant and throws an `INTERNAL_ERROR` with a descriptive message when it is violated, for both direct `OuterReference`s and subqueries with outer-scope references.
`PushdownPredicatesAndPruneColumnsForCTEDef` is updated to preserve the new field via `copy` instead of reconstructing `CTERelationDef`, and `CTERelationDef.stringArgs` keeps the default string representation stable when the flag is not set.
### Why are the changes needed?
Previously the default operation for CTE in spark is to inline it.
However, there are many internal features requiring to generate a materialized CTE which is guaranteed to be executed only once. For example, DSv2 command rewrites (e.g. MERGE INTO) can wrap a source in a CTE to avoid duplicating it. For correctness, if such CTE contains non-deterministic operation, such a CTE must be materialized and never inlined. There is currently no way to express that requirement explicitly on `CTERelationDef`.
Besides, we plan to reuse CTE results by introducing a shuffle and reuse the shuffle files. For usecases like DSv2 commands, the best CTE partitioning is already defined by the consumers. (In DSv2 case, the partitioning is the join keys for the join between source and target.) Providing a field for the suggested partitioning for the materialized CTE is necessary.
### Does this PR introduce any user-facing change?
No. The new field defaults to false and is internal only.
### How was this patch tested?
New unit tests in `InlineCTESuite` covering: the flag keeps a single-reference deterministic CTE materialized, the same CTE is inlined when the flag is unset, and validation fails with `INTERNAL_ERROR` when a force-materialized CTE carries an outer reference across its boundary.
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
Generated-by: claude-code-Opus 4.8 (1M context)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58006][SQL] Add forceSkipInline flag to CTERelationDef to force CTE materialization in logical planning [spark]
Posted by "AveryQi115 (via GitHub)" <gi...@apache.org>.
AveryQi115 commented on PR #57091:
URL: https://github.com/apache/spark/pull/57091#issuecomment-4907204409
cc: @juliuszsompolski
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58006][SQL] Add forceSkipInline flag to CTERelationDef to force CTE materialization in logical planning [spark]
Posted by "AveryQi115 (via GitHub)" <gi...@apache.org>.
AveryQi115 commented on PR #57091:
URL: https://github.com/apache/spark/pull/57091#issuecomment-4907201385
request for review from: @cloud-fan TY
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58006][SQL] Add forceSkipInline flag to CTERelationDef to force CTE materialization in logical planning [spark]
Posted by "juliuszsompolski (via GitHub)" <gi...@apache.org>.
juliuszsompolski commented on PR #57091:
URL: https://github.com/apache/spark/pull/57091#issuecomment-4908077971
@gengliangwang could you take a look?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58006][SQL] Add forceSkipInline flag to CTERelationDef to force CTE materialization in logical planning [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #57091:
URL: https://github.com/apache/spark/pull/57091#discussion_r3540143319
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/InlineCTE.scala:
##########
@@ -49,20 +50,56 @@ case class InlineCTE(
val cteMap = mutable.SortedMap.empty[Long, CTEReferenceInfo]
buildCTEMap(plan, cteMap)
cleanCTEMap(cteMap)
- inlineCTE(plan, cteMap)
+ val inlined = inlineCTE(plan, cteMap)
+ // A CTE that opts out of inlining must be self-contained: it cannot carry an outer
+ // reference across its boundary, because after the CTE is materialized there is no
+ // surrounding operator to resolve that reference against.
+ inlined.foreachWithSubqueries {
+ case cteDef: CTERelationDef if cteDef.forceSkipInline =>
+ validateNoOuterReferencesAcrossCTEBoundary(cteDef)
+ case _ =>
+ }
+ inlined
} else {
plan
}
}
- private def shouldInline(cteDef: CTERelationDef, refCount: Int): Boolean = 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]))
+ 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]))
+ })
+ }
+
+ 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}).")
+ }
+
+ val outerScopeSubqueries = cteDef.child.flatMap(
+ _.expressions.flatMap(_.collect {
+ case s: SubqueryExpression if s.outerScopeAttrs.nonEmpty => s
+ }))
+ if (outerScopeSubqueries.nonEmpty) {
+ throw SparkException.internalError(
+ "A force-materialized CTE cannot carry an outer reference across its boundary, but " +
+ "found a subquery with outer-scope reference " +
+ s"'${outerScopeSubqueries.head.outerScopeAttrs.head.name}' in the CTE definition " +
Review Comment:
This doesn't compile: `outerScopeAttrs` is typed `Seq[Expression]`, and `Expression` has no `.name` (only `NamedExpression` does). CI's "Precompile Spark" fails right here — `value name is not a member of ...Expression` at `InlineCTE.scala:100:63` — which is what cascades the other module builds red.
The elements of `outerScopeAttrs` are expressions that *contain* an `OuterScopeReference` (the partition predicate is `_.exists(_.isInstanceOf[OuterScopeReference])`), and may be compound (e.g. `Add(OuterScopeReference(a), Literal(1))`), so a plain cast to `NamedExpression` isn't safe either — collect the reference node:
```suggestion
if (outerScopeSubqueries.nonEmpty) {
val outerScopeRef = outerScopeSubqueries.head.outerScopeAttrs
.flatMap(_.collect { case r: OuterScopeReference => r }).head
throw SparkException.internalError(
"A force-materialized CTE cannot carry an outer reference across its boundary, but " +
"found a subquery with outer-scope reference " +
s"'${outerScopeRef.name}' in the CTE definition " +
```
This needs `OuterScopeReference` added to the imports on line 24 (`import org.apache.spark.sql.catalyst.expressions.{Alias, OuterReference, OuterScopeReference, SubqueryExpression}`). Check 1 above is fine as-is — `outerRefs` is `Seq[OuterReference]` and `OuterReference` is a `NamedExpression`.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/InlineCTESuite.scala:
##########
@@ -50,4 +51,41 @@ class InlineCTESuite extends PlanTest {
}
assert(e.getMessage.contains("No CTERelationDef found"))
}
+
+ test("SPARK-58006: forceSkipInline keeps a single-reference deterministic CTE") {
+ // A single-reference deterministic CTE is normally inlined, but `forceSkipInline` should
+ // keep it materialized in the `WithCTE` node.
+ val cteDef = CTERelationDef(OneRowRelation().select(1.as("a")), forceSkipInline = true)
+ val cteRef = CTERelationRef(cteDef.id, cteDef.resolved, cteDef.output, cteDef.isStreaming)
+ val plan = WithCTE(cteRef.select($"a"), Seq(cteDef)).analyze
+ val optimized = Optimize.execute(plan)
+ assert(optimized.collectFirst { case _: WithCTE => true }.isDefined,
+ "CTE with forceSkipInline should not be inlined")
+ }
+
+ test("SPARK-58006: forceSkipInline is inlined normally when not set") {
+ // The same single-reference deterministic CTE is inlined when `forceSkipInline` is false.
+ val cteDef = CTERelationDef(OneRowRelation().select(1.as("a")))
+ val cteRef = CTERelationRef(cteDef.id, cteDef.resolved, cteDef.output, cteDef.isStreaming)
+ val plan = WithCTE(cteRef.select($"a"), Seq(cteDef)).analyze
+ val optimized = Optimize.execute(plan)
+ assert(optimized.collectFirst { case _: WithCTE => true }.isEmpty,
+ "CTE without forceSkipInline should be inlined")
+ }
+
+ test("SPARK-58006: forceSkipInline CTE with an outer reference across its boundary fails") {
Review Comment:
The three tests look good, but the only cross-boundary test constructs a direct `OuterReference` on the CTE child, which exercises check 1. Check 2 (the `outerScopeAttrs`-based subquery branch — the one that currently doesn't compile) has no coverage. Worth adding a test with a nested-correlation subquery inside a `forceSkipInline` CTE so `outerScopeAttrs` is non-empty; that exercises the failing branch and guards the fix.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58006][SQL] Add forceSkipInline flag to CTERelationDef to force CTE materialization in logical planning [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #57091:
URL: https://github.com/apache/spark/pull/57091#issuecomment-4916430475
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/176249b739567f320a4ae14725754f81599045b7
- merged into branch-4.x https://github.com/apache/spark/commit/172ebf16ce1c4debe8158aee0001188b5ff572bb
*Posted by `merge_spark_pr.py`*
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58006][SQL] Add forceSkipInline flag to CTERelationDef to force CTE materialization in logical planning [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #57091: [SPARK-58006][SQL] Add forceSkipInline flag to CTERelationDef to force CTE materialization in logical planning
URL: https://github.com/apache/spark/pull/57091
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org