cloud-fan commented on code in PR #58012:
URL: https://github.com/apache/spark/pull/58012#discussion_r3813572943
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/InlineCTE.scala:
##########
@@ -40,10 +40,17 @@ import
org.apache.spark.sql.catalyst.trees.TreePattern.{CTE, PLAN_EXPRESSION}
* @param alwaysInline if true, inline all CTEs in the query plan.
* @param keepDanglingRelations if true, dangling CTE relations will be kept
in the original
* `WithCTE` node.
+ * @param isAnalysis if true, this rule runs during analysis (e.g. from
`CheckAnalysis`), where
+ * the plan may be a subplan that references
`CTERelationDef`s owned by a
+ * surrounding scope; such out-of-scope references are
tolerated and left for
+ * the owning scope to resolve. If false (the optimizer),
the plan is complete,
+ * so a `CTERelationRef` with no definition in the plan
indicates corruption and
+ * raises an error rather than being silently dropped.
*/
case class InlineCTE(
alwaysInline: Boolean = false,
- keepDanglingRelations: Boolean = false) extends Rule[LogicalPlan] {
+ keepDanglingRelations: Boolean = false,
+ isAnalysis: Boolean = true) extends Rule[LogicalPlan] {
Review Comment:
**Non-blocking:**
Make the missing-definition tolerance opt-in at the scoped analysis call
site and keep strict behavior as the default. This default also reaches
`ProgressReporter.scala:496` on a completed streaming plan and the
`InlineCTESuite` optimizer executor, so both silently inherit analysis
semantics even though their plans are expected to be complete.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/InlineCTE.scala:
##########
@@ -148,7 +155,7 @@ case class InlineCTE(
buildCTEMap(child, cteMap, outerCTEId)
}
- case ref: CTERelationRef =>
+ case ref: CTERelationRef if cteMap.contains(ref.cteId) =>
Review Comment:
**Non-blocking:**
Pattern-match on `cteMap.get(ref.cteId)` once so the normal path does not
probe the `SortedMap` with `contains` and then look up the same key again.
Please apply the same single-lookup shape to the matching guard in `inlineCTE`
at line 254.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/InlineCTESuite.scala:
##########
@@ -108,4 +108,21 @@ class InlineCTESuite extends PlanTest {
assert(e.getMessage.contains(
"found a subquery with outer-scope reference"))
}
+
+ test("SPARK-58779: optimizer InlineCTE (isAnalysis = false) fails on a ref
with no definition") {
+ // During analysis a CTERelationRef whose definition is not in the plan is
tolerated -- it is
+ // owned by a surrounding scope (e.g. ResolveSQLTableFunctions
checkAnalyzes a table-function
Review Comment:
**Nit:**
```suggestion
// owned by a surrounding scope (e.g. when `ResolveSQLTableFunctions`
runs `checkAnalysis` on a
```
--
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]
