pan3793 commented on code in PR #58661:
URL: https://github.com/apache/spark/pull/58661#discussion_r3987823802
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/PushdownPredicatesAndPruneColumnsForCTEDef.scala:
##########
@@ -77,8 +77,11 @@ object PushdownPredicatesAndPruneColumnsForCTEDef extends
Rule[LogicalPlan] with
val newPredicates = if (isTruePredicate(preds)) {
preds
} else {
- // Make sure we only push down predicates that do not contain
forward CTE references.
- val filteredPredicates = restoreCTEDefAttrs(predicates.filter(_.find
{
+ // Only push down deterministic predicates that do not contain
forward CTE references.
+ // The reference keeps its predicates, so a non-deterministic one
pushed into the
+ // definition as well would be evaluated twice.
+ val deterministicPredicates = predicates.filter(_.deterministic)
Review Comment:
Split out as SPARK-59434 (#58735), with your query as the test and no option
involved. Dropped the duplicated change here in 758d3a5cf71; the PR description
now states the dependency, and this one rebases once #58735 lands.
##########
docs/sql-ref-syntax-qry-select-cte.md:
##########
@@ -40,6 +40,19 @@ expression_name [ ( column_name [ , ... ] ) ] [ AS ] ( query
)
Specifies a name for the common table expression.
+* **MATERIALIZED**, **NOT MATERIALIZED**
+
+ Optionally specifies how the common table expression is evaluated.
`MATERIALIZED` forces it
+ to be evaluated once and shared by all references. `NOT MATERIALIZED`
forces it to be inlined,
+ so that each reference is planned and evaluated independently, and
non-deterministic
+ expressions such as `rand()` may yield different values per reference. A
`MATERIALIZED`
+ common table expression cannot reference columns of an outer query.
`MATERIALIZED` is not
+ supported in a statement whose common table expressions are always
inlined, such as a
+ multi-insert statement, nor in a subquery whose query, after the WITH
clause, references
Review Comment:
Taken your wording in 758d3a5cf71: "whose WITH clause or query references
columns of an outer query". The unreferenced-sibling shape stays rejected,
consistent with SPARK-45752.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/MaterializedCTECheck.scala:
##########
@@ -0,0 +1,110 @@
+/*
+ * 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.{OuterReference,
OuterScopeReference, SubExprUtils, SubqueryExpression}
+import org.apache.spark.sql.catalyst.plans.logical.{CTERelationDef,
CTERelationRef, LogicalPlan, SubqueryAlias, WithCTE}
+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 with it,
since they are
+ * inlined into it, unless they are MATERIALIZED themselves and form their own
boundary. Each
+ * definition is scanned at its own operator level and never inside its
subquery plans: a
+ * correlation a definition keeps inside its own subquery targets that
definition, not the
+ * enclosing query. A MATERIALIZED CTE is also rejected in a correlated
subquery when the query
+ * of its WITH clause references the outer query, as a `WithCTE` that is not
inlined cannot be
+ * decorrelated. 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 =>
+ (cteDef +: collectReferencedDefs(cteDef,
cteDefs)).foreach(checkDefinition)
+ }
+ // Decorrelation stops at a subtree without outer references, so only a
`WithCTE` whose
+ // own subtree is correlated is on its path. A correlation above the
WITH clause, e.g. on a
+ // derived table holding it, is fine.
+ plan.subqueriesAll.foreach(_.foreach {
Review Comment:
Gated in 758d3a5cf71 as you sketched: both passes hang off the materialized
definitions the first traversal collected, so a plan without a MATERIALIZED CTE
pays nothing beyond that traversal.
##########
sql/core/src/test/resources/sql-tests/inputs/cte-command.sql:
##########
@@ -29,5 +29,20 @@ INSERT INTO cte_tbl2 SELECT col;
SELECT * FROM cte_tbl;
SELECT * FROM cte_tbl2;
+-- MATERIALIZED CTE in a Multi-INSERT, should fail
+WITH s AS MATERIALIZED (SELECT 46 AS col)
+FROM s
+INSERT INTO cte_tbl SELECT col
+INSERT INTO cte_tbl2 SELECT col;
+
+-- NOT MATERIALIZED CTE in a Multi-INSERT
+WITH s AS NOT MATERIALIZED (SELECT 46 AS col)
+FROM s
+INSERT INTO cte_tbl SELECT col
+INSERT INTO cte_tbl2 SELECT col;
Review Comment:
Added in 758d3a5cf71: the single-INSERT case with two references and a
SELECT showing both rows, so the accepted command path is pinned next to the
two rejections.
--
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]
