Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 29.4k
[SPARK-46625][SQL] Place IDENTIFIER placeholder in command name slot#55949
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
59a0637d17669d9b263aa15d9e8444eb6c61cf623a9a1c9a95ba07b9c58e977b61d7613bb3e676a006d590f09a99390269bfd6789File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -59,19 +59,35 @@ trait UnresolvedUnaryNode extends UnaryNode with UnresolvedNode | ||
| /** | ||
| * A logical plan placeholder that holds the identifier clause string expression. It will be | ||
| * replaced by the actual logical plan with the evaluated identifier string. | ||
| * | ||
| * Extends `NamedRelation` so it can occupy a `NamedRelation`-typed slot (e.g. | ||
| * `OverwriteByExpression.table`) directly at parse time, instead of wrapping the whole command. | ||
| * | ||
| * The parser always places this node inside the command's identifier slot (a child slot for | ||
| * DELETE/UPDATE/MERGE/CTAS/RTAS, or a non-child slot for `InsertIntoStatement.table` and | ||
| * `OverwriteByExpression.table` -- handled via explicit cases in `ResolveIdentifierClause` and | ||
| * `BindParameters`). It is never the substitution root of a `WITH ... <command>` subtree, so | ||
| * `CTEInChildren` semantics are not needed: any surrounding `WithCTE` produced by | ||
| * `CTESubstitution` targets the inner command directly. | ||
| */ | ||
| case class PlanWithUnresolvedIdentifier( | ||
| identifierExpr: Expression, | ||
| children: Seq[LogicalPlan], | ||
| planBuilder: (Seq[String], Seq[LogicalPlan]) => LogicalPlan) | ||
| extends UnresolvedNode { | ||
| extends UnresolvedNode with NamedRelation { | ||
| def this(identifierExpr: Expression, planBuilder: Seq[String] => LogicalPlan) = { | ||
| this(identifierExpr, Nil, (ident, _) => planBuilder(ident)) | ||
| } | ||
| final override val nodePatterns: Seq[TreePattern] = Seq(PLAN_WITH_UNRESOLVED_IDENTIFIER) | ||
| // Placeholder name used by error paths that render `NamedRelation.name` for an unresolved | ||
| // table reference -- e.g. `SparkStrategies.extractTableNameForError` and the `r: NamedRelation` | ||
| // fallback in `QueryCompilationErrors`. Renders as the SQL text of the identifier expression | ||
| // (e.g. `IDENTIFIER(:p)` or `concat('a', 'b')`) so error messages remain informative. | ||
| override def name: String = identifierExpr.sql | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
| ||
| override protected def withNewChildrenInternal( | ||
| newChildren: IndexedSeq[LogicalPlan]): LogicalPlan = | ||
| copy(identifierExpr, newChildren, planBuilder) | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
if c.tempViewName.resolvedguard was added in commit 90f09a9 specifically to keep this invariant check from pre-empting the properUNRESOLVED_COLUMNuser-facing error whenIDENTIFIER(<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 aSparkException 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:That pins both halves of the design — the guard fires for unresolved IDENTIFIERs (so the catch-all
LogicalPlancase below producesUNRESOLVED_COLUMN), AND the case below it stays the catch-all, not pre-empted by this branch.