[SPARK-46625][SQL] Place IDENTIFIER placeholder in command name slot

[cloud-fan]

What changes were proposed in this pull request?

Root-cause fix for SPARK-46625. Supersedes #55706.

Push PlanWithUnresolvedIdentifier into the command's identifier slot at parse time, for every parser-built command, instead of wrapping the whole command. With this change, CTESubstitution sees the real CTEInChildren command directly and places WithCTE on the command's children by construction -- the invalid WithCTE(InsertIntoStatement, ...) / WithCTE(CreateTableAsSelect, ...) shape never appears.

Parser placement (AstBuilder):

• AstBuilder.withInsertInto, visitCreateTable, visitReplaceTable build the command directly. A new helper buildWriteTableSlot returns NamedRelation and serves both InsertIntoStatement.table and OverwriteByExpression.table (whose slot is typed NamedRelation). CTAS/RTAS use single-arg withIdentClause to put the placeholder in the name slot.
• visitCacheTable's AS path uses a new helper buildCacheTableAsSelectName to build the temp-view name as an Expression. The non-AS path uses single-arg withIdentClause.

CacheTableAsSelect.tempViewName: String -> Expression:

The last identifier slot still expressed as a plain String is now an Expression. The parser produces a non-null string Literal for direct identifiers and IDENTIFIER('literal'), or an ExpressionWithUnresolvedIdentifier for IDENTIFIER(<non-literal>). The single-part invariant is validated at parse time for literal cases and at materialization for the non-literal case. CheckAnalysis enforces the post-analysis invariant that tempViewName is a non-null string Literal, and tempViewNameString extracts the string for execution. The expression slot is naturally visited by transformExpressions, so ResolveIdentifierClause's existing ExpressionWithUnresolvedIdentifier branch resolves it without any new code path.

Placeholder shape (unresolved.scala):

• PlanWithUnresolvedIdentifier mixes in NamedRelation so it can occupy OverwriteByExpression.table (typed NamedRelation) directly.
• It does not extend CTEInChildren. With the in-slot placement plus the CacheTableAsSelect refactor, no parser caller places the placeholder as the substitution root of a WITH ... <command> subtree, so a CTEInChildren safety net has no reachable path under the current grammar.

Materialization (ResolveIdentifierClause):

InsertIntoStatement.table and V2WriteCommand.table are non-child LogicalPlan slots (child = query), so the default resolveOperatorsUp traversal never visits placeholders inside them. Two special-cases recurse explicitly:

• case i: InsertIntoStatement if i.table.isInstanceOf[PlanWithUnresolvedIdentifier] => ...
• case w: V2WriteCommand if w.table.isInstanceOf[PlanWithUnresolvedIdentifier] => ...

Each case extracts the placeholder with a single asInstanceOf at the top of the body and inlines the identifierExpr.resolved && childrenResolved check, returning the unchanged command when not yet ready.

The V2WriteCommand match dispatches via the abstract withNewTable(NamedRelation) and pattern-matches the materialized result as NamedRelation, throwing an internal error otherwise. Only OverwriteByExpression is parser-built with a placeholder in table today; matching the trait keeps the rule consistent for any future analyzer-built node in the same shape.

Tree-pattern propagation (statements.scala, v2Commands.scala):

InsertIntoStatement and V2WriteCommand override getDefaultTreePatternBits to union table.treePatternBits, so containsPattern(...) pruning correctly reports patterns (PARAMETER, PLAN_WITH_UNRESOLVED_IDENTIFIER) living in table.

Parameter binding (parameters.scala):

BindParameters.bind pattern-matches both InsertIntoStatement and V2WriteCommand to recurse into table. Without this, INSERT ... IDENTIFIER(:p) and INSERT INTO REPLACE WHERE ... IDENTIFIER(:p) under spark.sql.legacy.parameterSubstitution.constantsOnly=true would fail to bind the parameter.

CreateTableAsSelect.name / ReplaceTableAsSelect.name:

Already children via V2CreateTableAsSelectPlan.childrenToAnalyze, so no extra traversal hook is needed for them.

The post-hoc WithCTE(c: CTEInChildren, _) => c.withCTEDefs(cteDefs) collapse in ResolveIdentifierClause from #55706 is removed entirely -- no command shape reaches the analyzer needing it.

Why are the changes needed?

WITH t AS (...)
INSERT [INTO|OVERWRITE] TABLE IDENTIFIER('t')
SELECT * FROM t

previously produced an analysed plan with WithCTE wrapping the InsertIntoStatement -- a structurally invalid shape. Plug-in datasources that re-analyse the InsertIntoStatement's query subtree throw NoSuchElementException: key not found. Zero-day issue since SPARK-46625.

#55706 fixed the symptom by collapsing WithCTE(c: CTEInChildren, defs) after the fact in ResolveIdentifierClause. That works but encodes an undo for a placement that should never have happened, and turned out to be unsafe: after a later analyzer rewrite (e.g. RewriteDeleteFromTable rewriting WithCTE(DeleteFromTable, defs) into WithCTE(ReplaceData, defs)), the collapse would push WithCTE into ReplaceData.query and orphan the CTE refs in ReplaceData.condition / groupFilterCondition, breaking 9 v2 DML rCTE tests on CI. The review on #55706 (#55706 (comment)) asked for the root-cause fix; this PR implements it.

Credit to @stevomitric for surfacing in the original PR thread that the parser-level placement also has to work for legacy parameter substitution -- that's why this PR adds the targeted BindParameters and tree-pattern-bits handling for InsertIntoStatement.table and V2WriteCommand.table.

Does this PR introduce any user-facing change?

No behavior change, but a minor timing change in visitCreateTable / visitReplaceTable: CTAS/RTAS validation errors (Schema may not be specified in a CTAS statement, Partition column types may not be specified..., Constraints may not be specified...) for non-literal identifiers now fire at parse time rather than at identifier resolution. Same error messages and same ctx; only the moment of throwing moves earlier. Fail-fast improvement, not a regression.

How was this patch tested?

New tests in ParametersSuite:

• WITH ... INSERT OVERWRITE TABLE IDENTIFIER(:p) SELECT ... FROM cte
• WITH ... INSERT INTO IDENTIFIER(:p) SELECT ... FROM cte
• CREATE TABLE IDENTIFIER(:p) AS WITH ... SELECT ... FROM cte
• INSERT IDENTIFIER(:p) under legacy parameter substitution (covers spark.sql.legacy.parameterSubstitution.constantsOnly=true, which exercises the BindParameters recursion into InsertIntoStatement.table)
• WITH ... INSERT INTO IDENTIFIER(:p) REPLACE WHERE ... -- parser (asserts the placeholder lives in OverwriteByExpression.table and no WithCTE(CTEInChildren, _) shape survives CTESubstitution)
• BindParameters recurses into OverwriteByExpression.table (rule-level test; full analysis would require a v2 catalog)
• CACHE TABLE IDENTIFIER(...) AS WITH ... SELECT ... -- parser (asserts tempViewName holds ExpressionWithUnresolvedIdentifier and no WithCTE(CTEInChildren, _) shape survives CTESubstitution)
• REPLACE TABLE IDENTIFIER(...) AS WITH ... SELECT ... -- parser (mirrors the CTAS test for RTAS)

Each CTE test asserts that no WithCTE(CTEInChildren, _) shape leaks through analysis.

DDLParserSuite.CACHE TABLE updated to construct CacheTableAsSelect with Literal("t") instead of "t" for the new Expression slot.

Existing suites run clean: ParametersSuite, DeltaBased/GroupBased Delete/Update/Merge suites (including the rCTE-with-DML tests that were the original CI failures), DataSourceV2DataFrameSuite, SQLViewSuite, IdentifierClauseParserSuite, AnalysisSuite / PlanParserSuite / AnalysisErrorSuite and siblings, DataSourceV2SQLSuiteV1Filter, InsertSuite, CTEInlineSuite / CTEHintSuite, CachedTableSuite, CreateTableAsSelectSuite / DDLParserSuite.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude (Claude Code, Opus 4.7)

[cloud-fan]

cc @peter-toth @gengliangwang

[gengliangwang]

Summary

SPARK-46625 is fixed at the root: instead of wrapping the entire write/CTAS/RTAS/CACHE-TABLE command in PlanWithUnresolvedIdentifier and post-hoc collapsing WithCTE(CTEInChildren, _) after materialization (the predecessor PR #55706's approach, which broke under later analyzer rewrites like RewriteDeleteFromTable), this PR pushes the placeholder directly into the command's identifier slot at parse time. With the command itself sitting at the top, CTESubstitution.withCTEDefs immediately sees a CTEInChildren and places WithCTE on the command's children by construction — the invalid WithCTE(InsertIntoStatement, ...) shape becomes structurally unreachable.

Prior state. The parser's withIdentClause(ctx, otherPlans, builder) unconditionally wrapped the produced command in a PlanWithUnresolvedIdentifier for non-literal IDENTIFIER. For write/CTAS/RTAS commands, this put the placeholder at the substitution root of a WITH ... <command> subtree. Because PlanWithUnresolvedIdentifier is not a CTEInChildren, CTESubstitution.withCTEDefs fell into the default WithCTE(p, cteDefs) branch, wrapping the placeholder. When ResolveIdentifierClause later materialized into a real CTEInChildren command, the surviving WithCTE was now wrapping a CTEInChildren — the invalid shape that breaks plug-in datasources re-analyzing InsertIntoStatement.query with NoSuchElementException. #55706's after-the-fact collapse worked for direct DML but broke after RewriteDeleteFromTable rewrote WithCTE(DeleteFromTable, defs) into WithCTE(ReplaceData, defs), where the collapse pushed WithCTE into ReplaceData.query and orphaned CTE refs in condition / groupFilterCondition.

Design approach. Mirror what DELETE/UPDATE/MERGE already do — place the placeholder inside the command's identifier slot so the command sits at the top. Concretely:

• InsertIntoStatement.table / OverwriteByExpression.table: new helper buildWriteTableSlot returns UnresolvedRelation for literal/direct identifiers and a PlanWithUnresolvedIdentifier for non-literal IDENTIFIER. PlanWithUnresolvedIdentifier now mixes in NamedRelation so it can occupy the NamedRelation-typed OverwriteByExpression.table slot directly.
• CreateTableAsSelect.name / ReplaceTableAsSelect.name: single-arg withIdentClause(ctx, parts => UnresolvedIdentifier(parts)). These are already children via V2CreateTableAsSelectPlan.childrenToAnalyze.
• CacheTableAsSelect.tempViewName: String → Expression: a string Literal for direct identifiers and IDENTIFIER('literal'), an ExpressionWithUnresolvedIdentifier for IDENTIFIER(<non-literal>). Avoids the wrap-whole-command form.

Key design decisions.

• PlanWithUnresolvedIdentifier extends NamedRelation — required for OverwriteByExpression.table: NamedRelation to typecheck. name returns the SQL text of the identifier expression as a placeholder string.
• PlanWithUnresolvedIdentifier does not extend CTEInChildren — with in-slot placement no parser caller places the placeholder as the substitution root, so the trait would have no reachable use.
• Two analyzer rules need explicit recursion for non-child table slots: ResolveIdentifierClause (materialization) and BindParameters (parameter binding). InsertIntoStatement and V2WriteCommand override getDefaultTreePatternBits to union table.treePatternBits so pruning correctly reaches these nodes.
• CheckAnalysis validates the post-analysis shape of CacheTableAsSelect.tempViewName (must be a non-null string Literal); tempViewNameString extracts the string at execution time.

Implementation sketch. Touches parser (AstBuilder.withInsertInto, visitCreateTable, visitReplaceTable, visitCacheTable; new helpers buildWriteTableSlot and buildCacheTableAsSelectName); plan nodes (PlanWithUnresolvedIdentifier, InsertIntoStatement, V2WriteCommand for tree-pattern bits; CacheTableAsSelect for the new Expression slot); analyzer rules (ResolveIdentifierClause, BindParameters for explicit recursion; CheckAnalysis for invariant validation); strategy (DataSourceV2Strategy calls tempViewNameString). The post-hoc WithCTE collapse from #55706 is removed.

General

The PR description references a helper buildWriteTableSlotNamedRelation ("for OverwriteByExpression.table"), but the code only adds one helper, buildWriteTableSlot, which returns NamedRelation and serves both InsertIntoStatement.table and OverwriteByExpression.table. The description also shows the ResolveIdentifierClause case as case i @ InsertIntoStatement(p: PlanWithUnresolvedIdentifier, ...) ..., but the actual code uses isInstanceOf/asInstanceOf (see inline below). Worth aligning the description with the code so future readers don't search for a non-existent helper or pattern.

[gengliangwang]

PlanWithUnresolvedIdentifier is now reachable as a NamedRelation via two error paths I traced: SparkStrategies.extractTableNameForError (sql/core/.../SparkStrategies.scala:1135) and the r: NamedRelation => toSQLId(r.name) fallback in QueryCompilationErrors.scala:3635. Both will render the SQL text of the unresolved expression (e.g. IDENTIFIER(:p) or concat('a', 'b')) as the "table name" in error messages. That's a reasonable fallback, but worth a one-line comment on this override noting that identifierExpr.sql is intentionally an unresolved-placeholder stand-in for those error paths.

[gengliangwang]

The PR description shows this as case i @ InsertIntoStatement(p: PlanWithUnresolvedIdentifier, ...) ..., but the code uses isInstanceOf in the guard and asInstanceOf in both the guard and the body (the cast appears twice). Either update the description, or extract the cast once — e.g. guard with i.table.isInstanceOf[PlanWithUnresolvedIdentifier], then do the identifierExpr.resolved && childrenResolved check and the materialization in the body, returning i unchanged when not yet resolved. The same applies to the V2WriteCommand case below.

[gengliangwang]

End-to-end coverage gap: the CACHE TABLE / REPLACE TABLE / INSERT INTO IDENTIFIER REPLACE WHERE tests are all parser-level. For CACHE TABLE specifically an e2e test doesn't need v2-catalog or temp-view machinery beyond what SharedSparkSession already provides — CACHE TABLE IDENTIFIER('temp_v') AS WITH ... SELECT ... should resolve and execute. That would exercise the tempViewNameString extraction in DataSourceV2Strategy:781 and the new CheckAnalysis invariant case for CacheTableAsSelect, neither of which any test currently reaches.

[peter-toth]

LGTM, only one nit and seems like there is a linter issue.

[peter-toth]

The if c.tempViewName.resolved guard was added in commit 90f09a9 specifically to keep this invariant check from pre-empting the proper UNRESOLVED_COLUMN user-facing error when IDENTIFIER(<unresolved-col>) itself fails to resolve. The comment documents the intent well, but there's no test that pins this behavior — a future change that drops the guard (e.g. case c: CacheTableAsSelect => without the guard) would silently swap the user-facing error for a SparkException internal error, with nothing in CI to catch it.

Worth adding a regression test alongside the SPARK-46625 cache-table tests in ParametersSuite. Rough shape:

test("SPARK-46625: CACHE TABLE IDENTIFIER(<unresolved-col>) reports UNRESOLVED_COLUMN, not an internal error") {
  val ex = intercept[AnalysisException] {
    spark.sql(
      """CACHE TABLE IDENTIFIER(unresolved_col) AS SELECT 1 AS a""".stripMargin)
  }
  assert(ex.getCondition.startsWith("UNRESOLVED_COLUMN"))
  // and explicitly NOT the internal-error condition the invariant case throws
  assert(!ex.getMessage.contains("CacheTableAsSelect.tempViewName must be"))
}

That pins both halves of the design — the guard fires for unresolved IDENTIFIERs (so the catch-all LogicalPlan case below produces UNRESOLVED_COLUMN), AND the case below it stays the catch-all, not pre-empted by this branch.