gengliangwang commented on code in PR #58204:
URL: https://github.com/apache/spark/pull/58204#discussion_r3832876212
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveStrategies.scala:
##########
@@ -150,12 +150,6 @@ class DetermineTableStats(session: SparkSession) extends
Rule[LogicalPlan] {
case relation: HiveTableRelation
if DDLUtils.isHiveTable(relation.tableMeta) &&
relation.tableMeta.stats.isEmpty =>
hiveTableWithStats(relation)
Review Comment:
`spark.sql.hive.convertInsertingPartitionedTable=false` no longer forces the
Hive writer, now that the INSERT target is a child.
`RelationConversions` runs top-down (`resolveOperators` delegates to
`resolveOperatorsDownWithPruning`), and its write-path case is guarded by
`convertInsertingPartitionedTable` / `convertInsertingUnpartitionedTable`. When
that guard is false the `InsertIntoStatement` case no longer matches, so the
traversal descends into the node's children — and `table` is now one of them,
so the read-path case at line 258 matches the write target and replaces it via
`metastoreCatalog.convert(r, isWrite = false)`. `HiveAnalysis` then never
builds an `InsertIntoHiveTable`, so the built-in writer runs against a relation
configured for reading and `hive.exec.max.dynamic.partitions` is never
enforced. The rule's own doc comment at line 196 states the opposite contract.
`HiveSQLInsertTestSuite` "SPARK-54853: SET hive.exec.max.dynamic.partitions
takes effect in session conf" already fails on this head for exactly this
reason: "Expected exception org.apache.spark.SparkException to be thrown, but
no exception was thrown".
Add an `InsertIntoStatement` case to this rule that recurses into `query`
only, so the read path cannot reach a write target whatever the config says.
Deleting `DetermineTableStats`' insert case here is fine by contrast — its
generic case performs the same rewrite the deleted one did.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CTESubstitution.scala:
##########
@@ -411,6 +411,14 @@ object CTESubstitution extends Rule[LogicalPlan] {
alwaysInline: Boolean,
cteRelations: Seq[(String, CTERelationDef)],
recursiveCTERelation: Option[(String, CTERelationDef)]): LogicalPlan = {
+ plan match {
+ case i: InsertIntoStatement =>
+ // CTE names are visible in the input query, but they do not shadow
the INSERT target.
+ return i.copy(query = substituteCTE(
Review Comment:
This guard only fires when the `InsertIntoStatement` is the root of the
`substituteCTE` call, so a multi-insert statement still gets its target
replaced by a same-named CTE.
`substituteCTE` is called on the `UnresolvedWith`'s child (line 242). For a
single insert that child *is* the `InsertIntoStatement` and this case matches.
For a multi-insert, `visitMultiInsertQuery` builds `Union(inserts.toSeq)`, so
the child is a `Union`, this case does not match, and
`resolveOperatorsUpWithPruning` descends into each insert's `table` child —
where `case u @ UnresolvedRelation(Seq(table), _, _)` at line 436 hands the
target to `resolveWithCTERelations`.
`WITH t1 AS (SELECT ...) FROM src INSERT INTO t1 SELECT ... INSERT INTO t2
SELECT ...` is valid syntax (`ctes? dmlStatementNoWith`, with `fromClause
multiInsertQueryBody+` as one of its forms). On master the target could not be
substituted in either shape because it was not a child. Now it becomes a
`SubqueryAlias` over the CTE plan, and `PreWriteCheck` rejects the statement
with `UNSUPPORTED_INSERT.RDD_BASED` instead of writing to the persistent table.
A root check can't be made to work here, because the traversal is bottom-up:
by the time a nested `InsertIntoStatement` is visited, its target has already
been rewritten. Handle the node inside the traversal and skip only its `table`
subtree. That also restores this node's own expression pass, which the early
`return` currently bypasses — `case other` is what substitutes CTEs inside
subquery expressions, and `replaceCriteriaOpt` is an expression on this node.
Worth adding a multi-insert test; nothing covers that shape today.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala:
##########
@@ -209,21 +208,15 @@ case class InsertIntoStatement(
require(replaceCriteriaOpt.isEmpty || overwrite,
"REPLACE USING/ON/WHERE requires overwrite to be true")
- override def child: LogicalPlan = query
- override protected def withNewChildInternal(newChild: LogicalPlan):
InsertIntoStatement =
- copy(query = newChild)
+ override def children: Seq[LogicalPlan] = Seq(table, query)
Review Comment:
This trades a structural guarantee for a convention that every
child-descending rule now has to know about.
While `table` was not a child, "only the target-specific resolver rewrites
the INSERT target" was enforced by the node shape: nothing could reach the
slot, and the few rules that needed to look inside it did so explicitly. Now
each rule that descends into children has to decide whether it is looking at a
write target, and they disagree. `ResolveRelations` and `FindDataSourceTable`
get it right because their `InsertIntoStatement` cases are unguarded and match
before their generic relation cases. `RelationConversions` gets it wrong,
because its write case is config-guarded and the generic read case picks up the
target when the guard is false. `CTESubstitution` gets it half right, guarding
only the case where the insert is the substitution root. Both are flagged
separately.
The peer commands this shape imitates aren't evidence that it's safe for
INSERT: MERGE/UPDATE/DELETE reject Hive-serde and V1 file-source targets, so no
read-conversion rule ever has to protect their target child. INSERT accepts
them.
My suggestion is to give the invariant one owner rather than a case per
rule. Patching the two known gaps leaves the next rule that descends into
children with the same unstated obligation — which is how both of these arose.
Marking the target slot itself, so the generic read-side cases skip it by
construction, makes the protection inheritable instead of remembered.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -1151,17 +1151,11 @@ class Analyzer(
def apply(plan: LogicalPlan)
: LogicalPlan = plan.resolveOperatorsUpWithPruning(AlwaysProcess.fn,
ruleId) {
case i @ InsertIntoStatement(table, _, _, _, _, _, _, _, _) =>
- val relation = table match {
- case u: UnresolvedRelation if !u.isStreaming =>
- resolveRelation(u).getOrElse(u)
- case other => other
- }
-
// Inserting into a file-based temporary view is allowed.
// (e.g., spark.read.parquet("path").createOrReplaceTempView("t").
- // Thus, we need to look at the raw plan if `relation` is a temporary
view.
+ // Thus, we need to look at the raw plan if `table` is a temporary
view.
// unwrapRelationPlan also resolves V2TableReference nodes in temp
view plans.
- unwrapRelationPlan(relation) match {
+ unwrapRelationPlan(table) match {
Review Comment:
Now that the target resolves through the generic relation case, an INSERT
into a view analyzes the entire view body before this line rejects it.
The generic case resolves the target with
`resolveRelation(u).map(resolveViews(_, u.options))`, and `resolveViews` runs
`ViewResolution.resolve` on a `View` whose child is unresolved. Only afterwards
does `unwrapRelationPlan` reach the `View` and raise
`insertIntoViewNotAllowedError`. Previously the insert case resolved the target
itself and never called `resolveViews`, so the body was never analyzed.
For `CREATE TEMP VIEW v AS SELECT * FROM t; DROP TABLE t; INSERT INTO v
VALUES (1);` the user now gets `TABLE_OR_VIEW_NOT_FOUND` for `t` instead of
being told they cannot insert into a view, and a valid view target pays for a
full body analysis — recursively, for nested views — that is then thrown away.
Keeping the target off the generic view-resolution path avoids both, and the
target-slot intercept that fixes the Hive case would cover it.
--
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]
