Uh oh!
There was an error while loading. Please reload this page.
[SPARK-58933][SQL] Resolve expressions in INSERT target IDENTIFIER clauses - #58204
[SPARK-58933][SQL] Resolve expressions in INSERT target IDENTIFIER clauses#58204cloud-fan wants to merge 25 commits into
Conversation
gengliangwang
left a comment
There was a problem hiding this comment.
2 blocking, 3 non-blocking, 0 nits.
The node-shape change is the right direction and most of the fallout is handled, but two child-descending consumers were missed and CI is red on both of them plus stale golden files.
Design / architecture (1)
- sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala:211: Making the target a child converts a structurally enforced invariant into a per-rule convention that each child-descending rule must re-establish, and the convention is not currently held. -- see inline
Correctness (3)
- sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveStrategies.scala:152: RelationConversions' read path now converts the INSERT target, so convertInsertingPartitionedTable=false no longer forces the Hive writer. -- see inline
- sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CTESubstitution.scala:417: The CTE guard fires only when the insert is the substitution root, so a multi-insert statement still has its target replaced by a same-named CTE. -- see inline
- General: Regenerate the four explain golden files: results/explain.sql.out, results/explain-aqe.sql.out, analyzer-results/explain.sql.out, and analyzer-results/explain-aqe.sql.out still hold the target as a constructor argument, and ThriftServerQueryTestSuite fails on explain.sql and explain-aqe.sql at query #22 (EXPLAIN EXTENDED INSERT INTO TABLE explain_temp5 SELECT * FROM explain_temp4). Regenerate with SPARK_GENERATE_GOLDEN_FILES=1 rather than hand-editing, and confirm the diff is limited to the target moving from an argument to a child.
Suggestions (1)
- sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:1158: An INSERT into a view now analyzes the whole view body before rejecting the statement, so a broken view reports its own error instead of the insert-into-view error. -- see inline
Verification
I traced the two red CI jobs to root causes rather than taking the PR's "all tests passed" at face value. HiveSQLInsertTestSuite "SPARK-54853" fails because RelationConversions runs top-down (resolveOperators → resolveOperatorsDownWithPruning): with convertInsertingPartitionedTable=false its config-guarded write case declines, the traversal descends into the new table child, and the read-path HiveTableRelation case converts the write target, so InsertIntoHiveTable is never built and the Hive dynamic-partition limit is not enforced. ThriftServerQueryTestSuite fails on explain.sql/explain-aqe.sql because the target now renders as a child rather than a constructor argument in the parsed plan.
For the CTE guard I confirmed reachability from the grammar down: ctes? dmlStatementNoWith admits fromClause multiInsertQueryBody+, visitMultiInsertQuery returns Union(inserts), and traverseAndSubstituteCTE hands that Union — not the insert — to substituteCTE, so the root-only guard does not fire and the target reaches resolveWithCTERelations.
I also checked two things that turned out sound. The write-privilege check SPARK-58370 added survives the resolution-order flip: the target still carries REQUIRED_WRITE_PRIVILEGES, so it bypasses the relation cache and loadTable(ident, writePrivileges) still runs on a cache miss regardless of whether the target is resolved first or last, and reads never carried privileges through that API. And the REPLACE WHERE condition is not resolved against the newly visible table.output — ResolveReferences.doApply's first case intercepts InsertIntoStatement before the expression catch-all, which is what prevents an ambiguous-reference regression when the query and target share a column name.
PR metadata suggestions
- Correct the testing claim: "All 704 active tests in the original test set passed" no longer holds for this head — CI fails
HiveSQLInsertTestSuite(SPARK-54853) andThriftServerQueryTestSuite(explain.sql, explain-aqe.sql). - Qualify the CTE claim: "CTE definitions and relation substitution also remain restricted to the input query" holds only when the INSERT is the root of the CTE substitution; a multi-insert statement parses to a
Unionand the target is still substituted. - Document: the target is deliberately
children(0), and both the CheckAnalysis missing-target error precedence and the new target-before-query resolution order depend on that position. - Add to the user-facing change section: INSERT target resolution now goes through the generic relation path, which also applies
resolveViewsto the target and changes when a view body is analyzed.
| override def apply(plan: LogicalPlan): LogicalPlan = plan resolveOperators { | ||
| case relation: HiveTableRelation | ||
| if DDLUtils.isHiveTable(relation.tableMeta) && relation.tableMeta.stats.isEmpty => | ||
| hiveTableWithStats(relation) |
There was a problem hiding this 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.
| 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( |
There was a problem hiding this 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.
| override def child: LogicalPlan = query | ||
| override protected def withNewChildInternal(newChild: LogicalPlan): InsertIntoStatement = | ||
| copy(query = newChild) | ||
| override def children: Seq[LogicalPlan] = Seq(table, query) |
There was a problem hiding this 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.
There was a problem hiding this comment.
Thanks, I updated the design so the INSERT target remains a child without exposing it as a read relation during general analysis.
The parser creates UnresolvedWriteTarget, which remains the intermediate target representation until ResolveRelations resolves it to a metadata-only ResolvedTable, ResolvedPersistentView, or ResolvedTempView.
InsertIntoStatement is also intermediate. When write-specific rules convert it into the actual V1/V2 insert command, they construct the required provider relation locally and consume it during that conversion. The relation is therefore never exposed as the target child for generic analyzer traversal.
| // 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 { |
There was a problem hiding this 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.
- Resolve persistent INSERT targets through a PATH-aware metadata lookup that preserves write privileges and options without consuming or publishing read-cache entries. - Classify persistent and temporary views without resolving non-writable view bodies, while retaining direct DataFrame temp-view writes. - Build each V1 write relation once and update regression coverage and comments for target-first resolution, cache isolation, PATH, and view errors. Verified with the focused SQL analyzer, catalog, PATH, temp-view, V1 data source, and parameter suites.
- Reuse one Hive table relation for write conversion checks and lowering.\n- Correct grammar in the partition-write comments.
dongjoon-hyun
left a comment
There was a problem hiding this comment.
Thank you for working on this. I read through the whole change and checked out the branch locally. The direction looks right to me, and I like that UnresolvedWriteTarget is a distinct type from UnresolvedRelation — that is what makes promoting the target to a child safe: CTESubstitution cannot substitute it, and the generic relation case in ResolveRelations cannot pick it up. The multi-insert CTE shadowing concern from the earlier review is now resolved structurally rather than by a guard, and there is a regression test for it. CI is green on 0e4f836.
A few things I would like to see addressed before this goes in, plus some non-blocking notes.
1. Undocumented behavior change: write target no longer shares its loaded Table with same-statement reads
DataSourceV2OptionSuite has a test that was renamed and inverted:
| before | after | |
|---|---|---|
| name | persistent write targets establish table pins for subsequent reads | persistent write targets stay isolated from source reads |
writeRelation.table eq readRelation.table | targetRelation.table ne sourceRelation.table | |
loadTableCalls.size == 1 | loadTableCalls.size == 2 |
This follows from the new if (writePrivileges == null) guard in RelationResolution. So INSERT INTO t SELECT * FROM t now issues two loadTable calls, and the target and the source may observe different table state. For snapshot/versioned catalogs that is a meaningful semantic change, and it reverses the pinning behavior that was added deliberately.
The inline comment argues the case ("Catalogs may authorize or select different table state for reads and writes"), and that is a reasonable position — but nothing in the PR description mentions it. Could you confirm this is intended and call it out under Does this PR introduce any user-facing change?
2. PR description understates the change
The description covers "make the target a child", but the actual change is considerably larger. At minimum I think these belong in it:
- the new
UnresolvedWriteTargetnode that the parser now places in the target slot - targets resolving to metadata-only
ResolvedTable/ResolvedTempView/ResolvedPersistentViewrather than a readable relation - the new
InsertIntoStatement.tableOptionsfield RelationResolution.resolveWriteTarget, i.e. a second resolution implementation alongside the read pathInsertWriteRelation, which re-derives the V1 provider relation on demand in three rules viaSparkSession.activeRelationConversionsinliningPreprocessTableInsertionandDataSourceAnalysis- the
ResolvedTempViewrestructuring, its newoutput, and the EXPLAIN / golden file output changes - item 1 above
Also, the How was this patch tested? section (including "All 704 active tests in the original test set passed") reflects an intermediate head rather than this one; worth refreshing now that CI is green.
3. New uses of SparkSession.active
DataSourceAnalysis, PreprocessTableInsertion, PreWriteCheck and RelationConversions all reach for SparkSession.active now. The established convention nearby is to take the session in the constructor (FindDataSourceTable(session), DetermineTableStats(session)), and turning the three objects into classes would keep that.
PreWriteCheck is the one I care about most: it is a check rule, but through InsertWriteRelation -> readDataSourceTable it now calls DataSource.resolveRelation and mutates catalog.cacheTable(...). A validation-only rule building relations and updating a session cache is a layering problem independent of where the session comes from.
Related: the provider relation used to be materialized once and carried in the plan; now three rules each re-derive it. The catalog cache absorbs most of the cost, but this is the central trade-off of the design and deserves a sentence somewhere.
4. RelationConversions calling two other rules inline
valpreprocessed=PreprocessTableInsertion(i.copy(table = converted))
DataSourceAnalysis(preprocessed)The Post-Hoc batch is Once and ordered RelationConversions -> ... -> PreprocessTableInsertion -> DataSourceAnalysis -> HiveAnalysis, and both of those rules already handle a converted target through InsertWriteRelation's case relation: LogicalRelation branch. So returning plain i.copy(table = converted) looks like it would be picked up by the batch anyway. Is there a case the inline invocation covers that the batch ordering does not? If not, dropping it would also let the LogicalRelation / HiveTableRelation branches in InsertWriteRelation go.
In the same block, new DetermineTableStats(SparkSession.active).withTableStats(...) instantiates a rule just to reach a helper. Could withTableStats move to DDLUtils or a companion object instead?
5. Two parallel resolution implementations
resolveInPath shares the search order, but resolvePersistentWriteTarget re-implements the payload handling from tryResolvePersistent: the V1Table + isViewLike -> view case, the DelegatingTable case, and the ViewCatalog fallback. That is the part most likely to drift when someone fixes only one side later. Would it be feasible to factor out the shared "identifier -> (catalog, ident, Table | View)" step as well?
6. ResolvedTempView.stringArgs drops information
- ResolvedTempView global_temp.showcolumn4, `global_temp`.`showColumn4`
+ ResolvedTempView global_temp.showcolumn4
The case-preserved view name is gone from plan output. Iterator(identifier, metadata.identifier) would keep OpaqueLogicalPlan hidden while preserving the existing rendering, and would shrink the golden file diff by five files.
7. InsertIntoStatement.stringArgs hides tableOptions
After resolution, INSERT INTO t WITH ('k' = 'v') no longer shows its options anywhere in EXPLAIN. If that is deliberate (redaction?), a short comment would help; otherwise it seems worth keeping them visible. The predicate can also just be filterNot(_.isInstanceOf[CaseInsensitiveStringMap]) since tableOptions is the only such argument.
8. OpaqueLogicalPlan
The private constructor + companion apply + viewRelationPlan.plan.asInstanceOf[TemporaryViewRelation] is a fair amount of machinery. Declaring ResolvedTempView(identifier: Identifier, viewRelation: TemporaryViewRelation) and overriding equals / hashCode achieves the same shallow-comparison goal while keeping the field statically typed and removing the runtime cast and the new type.
9. Unrelated import reformatting
CTESubstitution.scala: the only change in this file is reformatting an import whose contents are byte-for-byte identical before and after. Please revert the file.statements.scala:{FieldName, FieldPosition, UnresolvedException}split across three lines with no change in contents.CreateFlowCommandSuite.scala: same.
The multi-line style is also inconsistent between the new sites (trailing WithCTE} in one, closing paren on its own line in another). Single-line imports are the convention here.
10. Minor
FindDataSourceTable: theAppendDatacase moved fromExtractV2Table(which ignores catalog/identifier) toDataSourceV2Relation(V1Table(...), _, Some(catalog), Some(identifier), ...). A V1Table-backed relation without a catalog or identifier no longer falls back toInsertIntoStatement. Intentional?- Same case:
ifPartitionNotExists = append.isByNameis alwaysfalseunder the!append.isByNameguard. It came from the old positional call, but now that it is named the oddity is visible;falsewould read better. DataSourceV2SQLSuite: the new comment "The write target is resolved before its query" does not match the code —ResolveRelationsusesresolveOperatorsUp, so the query resolves first and the target last. The actual reason the privilege check still runs is that write loads neither read nor populate the caches.AstBuilder.buildWriteTableSlot: the dangling.asInstanceOf[NamedRelation]on its own line.PlanResolutionSuite: switching the sharedv1SessionCatalogfromEmptyFunctionRegistrytoFunctionRegistry.builtin.clone()changes what every test in the suite exercises, not just the new IDENTIFIER ones.DDLParserSuite/PlanParserSuite: mappingUnresolvedWriteTargetback toUnresolvedRelationinparseComparefollows the existing precedent there, but it does mean the parser-level node change is invisible to those suites. The two new focused tests cover it, so this is just an observation.
Things I checked that look fine
buildWriteTableSlotis only reachable fromInsertIntoStatementconstruction, so removing theV2WriteCommandbranches fromBindParametersandResolveIdentifierClauseis safe.- The generic
case p: PlanWithUnresolvedIdentifierinResolveIdentifierClausestill collectsreferredTempVars, so the deleted INSERT-specific branch loses nothing. withCTEDefskeepsWithCTEon the query child only, andParametersSuiteasserts it.ParsedStatement.resolvedisfinal ... = false, so wideningchildrendoes not change the meaning ofi.resolved.- No lines over 100 chars and no new non-ASCII characters in the changed files.
- Correct resolution-order comments and instance-member links to match analyzer behavior. - Load Hive table statistics only after confirming write conversion is enabled. Verification: git diff --check. The focused Hive test was deferred because Maven Central DNS resolution was unavailable.
- Lower direct Hive-backed temp-view INSERT targets while keeping non-direct view bodies opaque, with coverage for converted and Hive write paths. - Keep DML target lineage independent of CTE source shadowing and add a role-aware parser regression. - Evaluate V2 write-target extraction once and align nearby source comments with their actual scope. Tests: - build/sbt -Phive 'hive/testOnly org.apache.spark.sql.hive.HiveSQLInsertTestSuite' - build/sbt 'sql/testOnly org.apache.spark.sql.catalyst.parser.ParseSqlResultSuite'
cloud-fan
commented
Aug 26, 2026
Thanks for the thorough review. I addressed the actionable items in
I also caught and fixed a Hive regression during verification: an unconverted partitioned Hive Verification included |
- Reuse one alias-eliminated temporary-view target for view rejection and V2 lowering. - Gate Hive relation materialization on metadata-only write-conversion checks. - Clarify the session-catalog requirement in the V1 fallback comment. Verified with git diff --check.
- Materialize V1 INSERT targets directly from the write-loaded catalog table and regress self-referential inserts with distinct read and write metadata. - Clarify catalog collision and view fallback behavior, including which target load carries write privileges. - Limit Hive write-target classification to unwrapping the leading alias chain.
- Resolve function-valued INSERT identifiers before transaction selection while deferring table lookup to transaction-aware analysis, with regression coverage. - Avoid repeated V2 target construction until input resolution completes. - Strip only leading aliases for temp-view target checks to avoid full stored-plan traversal. Verified with Catalyst and SQL compilation and AppendDataTransactionSuite.
Resolve transaction discovery with SQL PATH semantics and preserve the V1 write fallback for persistent FileTable targets. Also render unresolved Scaladoc references as code.
Begin each transactional SQL PATH candidate before performing its write-context table lookup, abort missing candidates, and retain the winning transaction and resolved target for analysis. This prevents pre-transaction base-catalog loads while preserving dynamic IDENTIFIER and PATH fallback semantics. Add transaction lifecycle and load-routing assertions for an absent candidate and the winning catalog. Verify the change with AppendDataTransactionSuite and Catalyst/SQL compilation.
Forward the source analyzer's sessionConf when creating a transaction-aware clone, while retaining explicit outer SQLConf precedence. Add a focused regression test covering the replacement catalog manager and both configuration scopes. Verified with the focused AnalysisSuite test and Catalyst/SQL compilation.
- Scope target lookup and transaction selection to the owning analyzer SQLConf, with cross-session regression coverage. - Replace the known transactional write target without traversing its source query. - Correct the test comment to reflect target-first resolution.
- Prune temporary UnresolvedInsert lowering with a registered tree pattern and clarify placeholder placement. - Cover dynamic INSERT classification, source lineage, and select-list metadata. - Verify the Catalyst compile, ParametersSuite, and ParseSqlResultSuite.
Document legacy constant-only ParameterizedQuery wrappers as unsupported by pre-transaction target resolution and remove the positive test that implied support.
cloud-fan
commented
Aug 28, 2026
thanks for the review, I'm merging to master/4.x/4.3/4.2 (offending commit was merged to 4.2 #55949) |
…auses ### What changes were proposed in this pull request? This PR introduces a temporary binary `UnresolvedInsert` plan for INSERT statements whose target uses a dynamic `IDENTIFIER(...)` expression. Its target and input query are ordinary children, so analyzer rules can resolve parameters, functions, and SQL variables in the target expression through normal tree traversal. After the expression is evaluated, its builder creates an `UnresolvedInsertTarget` containing the raw multipart identifier, target options, and write privileges. The analyzer then converts this marker to an `UnresolvedRelation` and lowers `UnresolvedInsert` to the existing unary `InsertIntoStatement`. The target is subsequently qualified and resolved by the existing INSERT analysis path. `UnresolvedInsertTarget` is deliberately not a `NamedRelation`, so CTE substitution cannot treat an INSERT target as a readable relation. Static table names and literal `IDENTIFIER` clauses still parse directly to `InsertIntoStatement` and retain the existing behavior. `QueryExecution` resolves only the dynamic target identifier before transaction detection. This allows transactional INSERTs to be recognized without adding a separate target-resolution path or changing the long-lived child contract of `InsertIntoStatement`. `ResolveUnresolvedInsert` uses a dedicated tree pattern and rule ID, so the fixed-point Resolution batch skips plans that cannot contain the temporary carrier. ### Why are the changes needed? `InsertIntoStatement.table` is not a logical-plan child, so normal analyzer rules do not visit a dynamic `IDENTIFIER` expression stored in that slot. Nested functions, parameters, or SQL variables can therefore remain unresolved. Making `InsertIntoStatement` permanently binary would affect analyzer, planner, lineage, and command consumers that rely on its input query being its only child. The temporary `UnresolvedInsert` node limits the binary shape to identifier-expression resolution, while `UnresolvedInsertTarget` preserves the raw name until normal relation resolution. This also avoids qualifying an already-resolved identifier a second time. ### Does this PR introduce _any_ user-facing change? Yes. INSERT statements can now resolve expressions inside a target `IDENTIFIER` clause. For example, the following statement now resolves and executes correctly: ```sql INSERT INTO IDENTIFIER( lower(regexp_replace(:table_name, 'PLACEHOLDER', 'TBL'))) REPLACE WHERE id = 1 VALUES (1, 'updated') ``` ### How was this patch tested? Added parser, analyzer, end-to-end, parse-metadata lineage and classification, and transactional-catalog coverage for dynamic INSERT targets, including parameters, nested functions, SQL variables, qualified multipart names, query-source and select-list extraction, and a target sharing its name with a CTE. The following compile and test checks passed: ```bash build/sbt catalyst/Test/compile build/sbt 'sql/testOnly org.apache.spark.sql.ParametersSuite' build/sbt 'sql/testOnly org.apache.spark.sql.catalyst.parser.ParseSqlResultSuite' build/sbt 'sql/testOnly org.apache.spark.sql.connector.AppendDataTransactionSuite' build/sbt pipelines/compile build/sbt connect/compile SPARK_GENERATE_GOLDEN_FILES=1 build/sbt \ 'sql/testOnly org.apache.spark.sql.SQLQueryTestSuite -- -z explain.sql' SPARK_GENERATE_GOLDEN_FILES=1 build/sbt \ 'sql/testOnly org.apache.spark.sql.SQLQueryTestSuite -- -z explain-aqe.sql' ``` ### Was this patch authored or co-authored using generative AI tooling? Generated-by: OpenAI Codex (GPT-5) Closes#58204 from cloud-fan/fix-identifier-dml-table-resolution. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 3bc29d9) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
cloud-fan
commented
Aug 28, 2026
…ER clauses ### What changes were proposed in this pull request? This backports #58204 (`3bc29d9d0ae031dfe0e793886ba652ca98ae7f29`) to branch-4.3. It introduces a temporary binary `UnresolvedInsert` plan for INSERT statements whose target uses a dynamic `IDENTIFIER(...)` expression. This lets normal analyzer traversal resolve parameters, functions, and SQL variables in the target before lowering the plan to `InsertIntoStatement`. The parse-metadata changes from the original commit are omitted because the corresponding `ParseSqlResult` and `SqlStatementCodes` infrastructure is not present on branch-4.3. ### Why are the changes needed? `InsertIntoStatement.table` is not a logical-plan child, so analyzer rules do not normally visit a dynamic target expression stored there. Nested functions, parameters, or SQL variables can therefore remain unresolved. ### Does this PR introduce _any_ user-facing change? Yes. INSERT statements on branch-4.3 can now resolve expressions inside a target `IDENTIFIER` clause, matching the behavior fixed on master by #58204. ### How was this patch tested? The backported parser, analyzer, end-to-end, and transactional-catalog tests were included. The following checks passed locally with Java 17: ```bash build/sbt -java-home /usr/lib/jvm/java-17-openjdk-amd64 \ catalyst/Test/compile \ 'sql/testOnly org.apache.spark.sql.ParametersSuite' \ 'sql/testOnly org.apache.spark.sql.connector.AppendDataTransactionSuite' \ pipelines/compile connect/compile ``` `ParametersSuite`: 162 tests passed. `AppendDataTransactionSuite`: 19 tests passed. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: OpenAI Codex (GPT-5) Closes#58372 from cloud-fan/SPARK-58933-branch-4.3. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
…ER clauses ### What changes were proposed in this pull request? This backports #58204 (`3bc29d9d0ae031dfe0e793886ba652ca98ae7f29`) to branch-4.2. It introduces a temporary binary `UnresolvedInsert` plan for INSERT statements whose target uses a dynamic `IDENTIFIER(...)` expression. This lets normal analyzer traversal resolve parameters, functions, and SQL variables in the target before lowering the plan to the existing write plan. For branch-4.2, `INSERT ... REPLACE WHERE` is lowered to that branch's `OverwriteByExpression` representation. The transactional regression test is also expressed using the older transaction test harness. The parse-metadata changes from the original commit are omitted because the corresponding `ParseSqlResult` and `SqlStatementCodes` infrastructure is not present on branch-4.2. ### Why are the changes needed? `InsertIntoStatement.table` is not a logical-plan child, so analyzer rules do not normally visit a dynamic target expression stored there. Nested functions, parameters, or SQL variables can therefore remain unresolved. ### Does this PR introduce _any_ user-facing change? Yes. INSERT statements on branch-4.2 can now resolve expressions inside a target `IDENTIFIER` clause, matching the behavior fixed on master by #58204. ### How was this patch tested? The backported parser, analyzer, end-to-end, and transactional-catalog tests were included and adapted to branch-4.2. The following checks passed locally with Java 17: ```bash build/sbt -java-home /usr/lib/jvm/java-17-openjdk-amd64 \ catalyst/Test/compile \ 'sql/testOnly org.apache.spark.sql.ParametersSuite' \ 'sql/testOnly org.apache.spark.sql.connector.AppendDataTransactionSuite' \ pipelines/compile connect/compile ``` `ParametersSuite`: 161 tests passed. `AppendDataTransactionSuite`: 18 tests passed. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: OpenAI Codex (GPT-5) Closes#58373 from cloud-fan/SPARK-58933-branch-4.2. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
### What changes were proposed in this pull request? This follow-up to #58204 simplifies and corrects parsed INSERT target handling: * Make the SQL parser always produce `UnresolvedInsert` for table-target INSERT statements. A static target is represented by `UnresolvedInsertTarget`, while a dynamic `IDENTIFIER` expression remains in `PlanWithUnresolvedIdentifier`. The analyzer lowers either form to `InsertIntoStatement` once the target identifier is ready for relation resolution. * Update parser-oriented consumers, including SQL statement classification, `parse_sql` lineage collection, and pipeline flow registration, to handle the single parsed INSERT shape. `InsertIntoStatement` handling remains where plans can be created programmatically or have already been lowered by analysis. * Scope the `parse_sql` CTE-shadow exemption specifically to `UnresolvedInsertTarget`. DELETE, UPDATE, and MERGE targets continue to follow CTE substitution semantics. * Run early dynamic INSERT target resolution inside the analysis planning tracker and report its failures through `QueryPlanningTracker.setAnalysisFailed`. ### Why are the changes needed? The parser previously produced `InsertIntoStatement` for static targets and `UnresolvedInsert` for dynamic targets. Parser consumers therefore had to understand both shapes even though `UnresolvedInsert` is only an intermediate node and is lowered immediately after its target is ready. Using one parsed representation makes that boundary explicit and removes duplicated matching logic. The target-role exemption added by #58204 was also broader than required. A CTE-shadowed DELETE, UPDATE, or MERGE target could be reported as a catalog target even though CTE substitution replaces that relation. In addition, early dynamic target resolution was absent from analysis timing and failure reporting. ### Does this PR introduce _any_ user-facing change? Yes, within unreleased master only. `parse_sql` no longer reports CTE-shadowed DELETE, UPDATE, or MERGE targets as catalog target-table references. There is no change relative to a released Spark version. ### How was this patch tested? Added and updated regression coverage, then ran: ```bash build/sbt \ "catalyst/testOnly org.apache.spark.sql.catalyst.parser.PlanParserSuite org.apache.spark.sql.catalyst.parser.DDLParserSuite org.apache.spark.sql.catalyst.parser.IdentifierClauseParserSuite" \ "sql/testOnly org.apache.spark.sql.ParametersSuite org.apache.spark.sql.catalyst.parser.ParseSqlResultSuite org.apache.spark.sql.execution.QueryExecutionSuite org.apache.spark.sql.execution.command.v2.CreateFlowCommandSuite" \ pipelines/compile ``` All 284 Catalyst tests and 204 SQL tests passed, and the pipelines module compiled successfully. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: OpenAI Codex (GPT-5) Closes#58378 from cloud-fan/fix-identifier-dml-followup. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
### What changes were proposed in this pull request? This follow-up to #58204 simplifies and corrects parsed INSERT target handling: * Make the SQL parser always produce `UnresolvedInsert` for table-target INSERT statements. A static target is represented by `UnresolvedInsertTarget`, while a dynamic `IDENTIFIER` expression remains in `PlanWithUnresolvedIdentifier`. The analyzer lowers either form to `InsertIntoStatement` once the target identifier is ready for relation resolution. * Update parser-oriented consumers, including SQL statement classification, `parse_sql` lineage collection, and pipeline flow registration, to handle the single parsed INSERT shape. `InsertIntoStatement` handling remains where plans can be created programmatically or have already been lowered by analysis. * Scope the `parse_sql` CTE-shadow exemption specifically to `UnresolvedInsertTarget`. DELETE, UPDATE, and MERGE targets continue to follow CTE substitution semantics. * Run early dynamic INSERT target resolution inside the analysis planning tracker and report its failures through `QueryPlanningTracker.setAnalysisFailed`. ### Why are the changes needed? The parser previously produced `InsertIntoStatement` for static targets and `UnresolvedInsert` for dynamic targets. Parser consumers therefore had to understand both shapes even though `UnresolvedInsert` is only an intermediate node and is lowered immediately after its target is ready. Using one parsed representation makes that boundary explicit and removes duplicated matching logic. The target-role exemption added by #58204 was also broader than required. A CTE-shadowed DELETE, UPDATE, or MERGE target could be reported as a catalog target even though CTE substitution replaces that relation. In addition, early dynamic target resolution was absent from analysis timing and failure reporting. ### Does this PR introduce _any_ user-facing change? Yes, within unreleased master only. `parse_sql` no longer reports CTE-shadowed DELETE, UPDATE, or MERGE targets as catalog target-table references. There is no change relative to a released Spark version. ### How was this patch tested? Added and updated regression coverage, then ran: ```bash build/sbt \ "catalyst/testOnly org.apache.spark.sql.catalyst.parser.PlanParserSuite org.apache.spark.sql.catalyst.parser.DDLParserSuite org.apache.spark.sql.catalyst.parser.IdentifierClauseParserSuite" \ "sql/testOnly org.apache.spark.sql.ParametersSuite org.apache.spark.sql.catalyst.parser.ParseSqlResultSuite org.apache.spark.sql.execution.QueryExecutionSuite org.apache.spark.sql.execution.command.v2.CreateFlowCommandSuite" \ pipelines/compile ``` All 284 Catalyst tests and 204 SQL tests passed, and the pipelines module compiled successfully. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: OpenAI Codex (GPT-5) Closes#58378 from cloud-fan/fix-identifier-dml-followup. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 900b44e) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
What changes were proposed in this pull request?
This PR introduces a temporary binary
UnresolvedInsertplan for INSERT statements whose targetuses a dynamic
IDENTIFIER(...)expression. Its target and input query are ordinary children, soanalyzer rules can resolve parameters, functions, and SQL variables in the target expression
through normal tree traversal.
After the expression is evaluated, its builder creates an
UnresolvedInsertTargetcontaining theraw multipart identifier, target options, and write privileges. The analyzer then converts this
marker to an
UnresolvedRelationand lowersUnresolvedInsertto the existing unaryInsertIntoStatement. The target is subsequently qualified and resolved by the existing INSERTanalysis path.
UnresolvedInsertTargetis deliberately not aNamedRelation, so CTE substitution cannot treatan INSERT target as a readable relation. Static table names and literal
IDENTIFIERclauses stillparse directly to
InsertIntoStatementand retain the existing behavior.QueryExecutionresolves only the dynamic target identifier before transaction detection. Thisallows transactional INSERTs to be recognized without adding a separate target-resolution path or
changing the long-lived child contract of
InsertIntoStatement.ResolveUnresolvedInsertuses a dedicated tree pattern and rule ID, so the fixed-point Resolutionbatch skips plans that cannot contain the temporary carrier.
Why are the changes needed?
InsertIntoStatement.tableis not a logical-plan child, so normal analyzer rules do not visit adynamic
IDENTIFIERexpression stored in that slot. Nested functions, parameters, or SQL variablescan therefore remain unresolved.
Making
InsertIntoStatementpermanently binary would affect analyzer, planner, lineage, andcommand consumers that rely on its input query being its only child. The temporary
UnresolvedInsertnode limits the binary shape to identifier-expression resolution, whileUnresolvedInsertTargetpreserves the raw name until normal relation resolution. This also avoidsqualifying an already-resolved identifier a second time.
Does this PR introduce any user-facing change?
Yes. INSERT statements can now resolve expressions inside a target
IDENTIFIERclause. Forexample, the following statement now resolves and executes correctly:
How was this patch tested?
Added parser, analyzer, end-to-end, parse-metadata lineage and classification, and
transactional-catalog coverage for dynamic INSERT targets, including parameters, nested functions,
SQL variables, qualified multipart names, query-source and select-list extraction, and a target
sharing its name with a CTE.
The following compile and test checks passed:
Was this patch authored or co-authored using generative AI tooling?
Generated-by: OpenAI Codex (GPT-5)