pan3793 commented on code in PR #58661: URL: https://github.com/apache/spark/pull/58661#discussion_r3978573830
########## sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/MaterializedCTECheck.scala: ########## @@ -0,0 +1,100 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.sql.catalyst.analysis + +import scala.collection.mutable + +import org.apache.spark.sql.catalyst.expressions.{AttributeSet, OuterReference, OuterScopeReference, SubqueryExpression} +import org.apache.spark.sql.catalyst.plans.logical.{CTERelationDef, CTERelationRef, LogicalPlan} +import org.apache.spark.sql.catalyst.trees.TreePattern.CTE +import org.apache.spark.sql.errors.QueryCompilationErrors + +/** + * Checks that a MATERIALIZED CTE does not reference the query enclosing it, as it is evaluated + * once on its own. The CTEs it references, transitively, are checked against the same boundary, + * since they are inlined into it. An outer reference crosses the boundary only if it targets an + * attribute that is not produced within it: a CTE nested in a subquery of the definition may + * legitimately be correlated to the definition's own relations. The check covers the given plan + * and all its subqueries. + */ +object MaterializedCTECheck extends (LogicalPlan => Unit) { + override def apply(plan: LogicalPlan): Unit = { + if (plan.containsPattern(CTE)) { + // All CTE definitions, including those of nested subqueries, so that references from a + // MATERIALIZED CTE can be followed across subquery boundaries. + val cteDefs = mutable.LinkedHashMap.empty[Long, CTERelationDef] + plan.foreachWithSubqueries { + case cteDef: CTERelationDef => cteDefs(cteDef.id) = cteDef + case _ => + } + cteDefs.values.filter(_.materialized.contains(true)).foreach { cteDef => + checkMaterializedCTE(cteDef, cteDefs) + } + } + } + + private def checkMaterializedCTE( + cteDef: CTERelationDef, + cteDefs: collection.Map[Long, CTERelationDef]): Unit = { + val inlinedDefs = collectInlinedDefs(cteDef, cteDefs) + // The attributes produced within the materialized boundary, including those of nested + // subqueries, as a correlation to any of them does not cross the boundary. Unresolved + // operators are skipped: they may not have an output, and are reported by the analysis + // checks that follow. + val internalAttrs = AttributeSet(inlinedDefs.flatMap( Review Comment: Fixed in 0364b66b093 the way you describe: the check inlines the referenced CTEs with `InlineCTE(alwaysInline = true, isAnalysis = true)` first, then rejects any `OuterReference` or outer-scope subquery reference left at the operator level. No attribute set. Two refinements: definitions nested inside a definition are left in place (they are inlined there anyway), and a referenced `MATERIALIZED` definition is not followed, as it is its own boundary. The shared-CTE shape is a negative in `CTEInlineSuite` and `cte.sql`, and the `v2 AS MATERIALIZED (SELECT * FROM v1)` shape is a positive in both. ########## sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala: ########## @@ -316,6 +316,9 @@ trait CheckAnalysis extends LookupCatalog with QueryErrorsBase with PlanToString } def checkAnalysis(plan: LogicalPlan): Unit = { + // Check MATERIALIZED CTE relations before inlining, as `InlineCTE` below inlines them to + // restore the original plan shape. + MaterializedCTECheck(plan) Review Comment: Fixed in 0364b66b093: the check runs only for a resolved definition. `checkAnalysis0` has a catch-all for unresolved operators, so an unresolved definition is always reported by the resolution checks, and a reference to an unresolved definition is itself unresolved, which covers a typo in a referenced CTE. Analysis-only commands analyze their body before `checkAnalysis`, so their definitions are still checked; `CTEInlineSuite` pins that with `CREATE TEMPORARY VIEW`, and pins the typo query reporting `UNRESOLVED_COLUMN` on both analyzers. -- 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]
