FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[SPARK-58966][SQL] Resolve SQL variables in UPDATE and MERGE INTO conditions by joelrobin18 · Pull Request #58244 · apache/spark · GitHub

/ spark Public

[SPARK-58966][SQL] Resolve SQL variables in UPDATE and MERGE INTO conditions - #58244

Open
joelrobin18 wants to merge 3 commits into
apache:masterfrom
joelrobin18:joel-robin/variable-resolution-in-update-merge
Open

[SPARK-58966][SQL] Resolve SQL variables in UPDATE and MERGE INTO conditions#58244
joelrobin18 wants to merge 3 commits into
apache:masterfrom
joelrobin18:joel-robin/variable-resolution-in-update-merge

Conversation

joelrobin18 commented Aug 24, 2026
edited
Loading

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

SQL variables declared with DECLARE cannot be referenced in the conditions of an
UPDATE or MERGE INTO statement. This passes includeLastResort = true at the nine
condition-resolution sites that make up those clauses, matching what SPARK-57260 did for
OverwriteByExpression.deleteExpr.

Variable resolution only runs from resolveColsLastResort, which is reached when
resolveExpressionByPlanOutput / resolveExpressionByPlanChildren are called with
includeLastResort = true. Both default the flag to false. Plans with no dedicated
resolution rule fall through to the generic operator case in ResolveReferences, which
does pass the flag -- which is why DELETE ... WHERE already works. UPDATE and
MERGE INTO each have a dedicated rule that omitted it:

  • UPDATE condition (ResolveReferencesInUpdate)
  • MERGE ON condition
  • MERGE WHEN MATCHED DELETE / UPDATE conditions
  • MERGE UPDATE * condition
  • MERGE WHEN NOT MATCHED INSERT / INSERT * conditions
  • MERGE WHEN NOT MATCHED BY SOURCE DELETE / UPDATE conditions

This covers every MergeAction condition form, so all MERGE conditions now resolve
variables consistently.

Assignment values (SET col = var, INSERT VALUES (var)) are affected by the same root
cause, but they resolve through resolveExprInAssignment, which sets
includeLastResort = false explicitly rather than by default. That is left unchanged
here pending a decision on whether the explicit false was deliberate, so this PR is
scoped to conditions only.

Why are the changes needed?

Variables resolve in SELECT, INSERT (including REPLACE WHERE) and DELETE, but in
no condition of UPDATE or MERGE INTO, which fails analysis with:

[UNRESOLVED_COLUMN.WITH_SUGGESTION] A column, variable, or function parameter with
name `target_dep` cannot be resolved. SQLSTATE: 42703

The message itself offers "A column, variable, or function parameter", so the analyzer
reports that variable resolution was attempted. Nothing in error-conditions.json, the
tests, or the docs records a restriction here. Where Spark does intentionally block
variables (SPARK-57360, generated columns) it uses an explicit validation and a dedicated
error class, so the inconsistency looks like an oversight rather than a deliberate
limitation.

This affects both session variables and SQL scripting local variables.

Does this PR introduce any user-facing change?

Yes, a bug fix.

Before, against a table using the built-in DSv2 row-level operation framework:

DECLARE OR REPLACE VARIABLE target_dep STRING DEFAULT 'hr';
UPDATE cat.ns1.test_table SET salary = 0 WHERE dep = target_dep;
-- [UNRESOLVED_COLUMN.WITH_SUGGESTION] ... `target_dep` cannot be resolved. SQLSTATE: 42703

After, the statement analyzes and executes, resolving target_dep to the declared
variable. Statements that previously succeeded are unaffected: the flag only enables a
last-resort resolution step that runs after normal column resolution fails, so column
references continue to take precedence over same-named variables.

How was this patch tested?

Six new tests, plus a shared withSessionVariable helper in
RowLevelOperationSuiteBase:

  • UpdateTableSuiteBase -- a session variable and a SQL scripting local variable in an
    UPDATE condition.
  • MergeIntoTableSuiteBase -- a variable in the MERGE ON condition, in the
    WHEN MATCHED / WHEN NOT MATCHED BY SOURCE conditions, and in the
    WHEN NOT MATCHED INSERT and INSERT * conditions.

Each test was verified to fail without the fix (raising UNRESOLVED_COLUMN) and to pass
with it. The not-matched tests keep a source row that the variable predicate excludes, so
they fail if the variable is resolved but evaluated incorrectly, not just if resolution
fails.

Suites run: GroupBasedUpdateTableSuite, GroupBasedMergeIntoTableSuite,
DeltaBasedUpdateTableSuite and DeltaBasedMergeIntoTableSuite -- 289 tests, all
passing. Reverting only the Analyzer change makes the two not-matched tests fail with
UNRESOLVED_COLUMN on the variable in the insertaction condition.

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

Generated-by: Claude Code

…ditions

Signed-off-by: joelrobin18 <joelrobin1818@gmail.com>

cloud-fan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

0 blocking, 1 non-blocking, 0 nits.
The shared fallback approach is sound, but the MERGE condition enumeration is incomplete.

Design / architecture (1)

  • Non-blocking: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:1899: Apply last-resort variable resolution to both WHEN NOT MATCHED insert-condition branches and cover the form with a focused test. -- see inline

Verification

I traced every MergeAction condition through ResolveReferences and the downstream merge rewrite. Matched and not-matched-by-source actions now opt into last-resort resolution, while InsertAction and InsertStarAction conditions still use the column-only resolver. Those insert conditions are consumed as normal MERGE predicates, and an unresolved variable reaches CheckAnalysis as UNRESOLVED_COLUMN.

PR metadata suggestions

  • Correct the condition-coverage claim, preferably by including and testing WHEN NOT MATCHED AND <variable>; both insert-condition branches still omit last-resort variable resolution.

…sert conditions

Signed-off-by: joelrobin18 <joelrobin1818@gmail.com>
joelrobin18 requested a review from cloud-fan August 25, 2026 05:52

cloud-fan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

1 addressed, 0 remaining, 2 new to this AI review. (0 newly introduced, 2 late catches, 0 previously raised, 0 unattributed findings.)
0 blocking, 2 non-blocking, 0 nits.
The analyzer fix and condition coverage are sound; two non-production cleanup and documentation issues remain.

Correctness (2)

  • Non-blocking: sql/core/src/test/scala/org/apache/spark/sql/connector/RowLevelOperationSuiteBase.scala:130: Make the new session-variable test helper preserve or reject an existing same-named variable and reuse QueryTest's safe cleanup. -- see inline
  • Non-blocking: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveReferencesInUpdate.scala:71: Update ResolveReferencesInUpdate's documented resolution order to include the condition-only last-resort steps. -- see inline

Verification

I traced UPDATE, the MERGE ON clause, and each matched, not-matched, and not-matched-by-source action to ColumnResolutionHelper. Normal plan-output resolution still runs first, and includeLastResort converges on the existing outer-reference-then-variable fallback. The two branches missing in the prior revision now enable that path, and their focused tests assert the filtered row set. I did not run the Spark test suites locally.

catalog.createTable(ident, tableInfo)
}

/** Declares a session variable for the duration of `f`, dropping it afterwards. */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Non-blocking:

Make this helper reject or preserve an existing same-named variable. DECLARE OR REPLACE overwrites the outer entry, and the inner finally then drops it, so a nested call returns to an outer body where the variable is gone. The inherited QueryTest.withSessionVariable(name) already provides exception-safe cleanup; wrapping a plain DECLARE VARIABLE with it avoids both clobbering and cleanup masking.

assignments = newAssignments,
condition = u.condition.map(resolveExpressionByPlanChildren(_, u)))
condition = u.condition.map(
resolveExpressionByPlanChildren(_, u, includeLastResort = true)))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Non-blocking:

Update the class Scaladoc with this condition path. It currently presents a complete three-step UpdateTable resolution order, but includeLastResort = true adds outer-reference and SQL-variable resolution for conditions. Separating the assignment and condition orders would keep the contract accurate.

…el tests

Signed-off-by: joelrobin18 <joelrobin1818@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants


Back | FazBrowse Home | New Git URL