[SPARK-58966][SQL] Resolve SQL variables in UPDATE and MERGE INTO conditions - #58244
joelrobin18 wants to merge 4 commits into
Conversation
…ditions Signed-off-by: joelrobin18 <joelrobin1818@gmail.com>
cloud-fan
left a comment
There was a problem hiding this comment.
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 MATCHEDinsert-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>
cloud-fan
left a comment
There was a problem hiding this comment.
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.
…el tests Signed-off-by: joelrobin18 <joelrobin1818@gmail.com>
cloud-fan
left a comment
There was a problem hiding this comment.
Review summary
One blocking correctness issue remains. The earlier inline concerns are addressed in the pinned head, and the existing tests plus green CI cover ordinary UPDATE/MERGE variable resolution well. However, no test exercises a condition name that becomes a target column during MERGE schema evolution while a same-named variable exists; that fixed-point path can permanently bind the variable before the evolved target is reloaded. I did not run tests locally.
Findings
1 total: 0 P0, 1 P1, 0 P2, 0 P3.
Blocking (P1)
- Delay variable fallback until the evolved target schema is final —
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:1885— see inline.
| case DeleteAction(deleteCondition) => | ||
| val resolvedDeleteCondition = deleteCondition.map( | ||
| resolveExpressionByPlanOutput(_, targetTable)) | ||
| resolveExpressionByPlanOutput(_, targetTable, includeLastResort = true)) |
There was a problem hiding this comment.
Blocking (P1): Please defer last-resort resolution for MERGE conditions until schema evolution has reached its final target schema. On the first analyzer pass, assignments have not yet made pending schema changes visible, so a condition reference such as new_col can bind to a same-named SQL variable here. On the next pass, ResolveSchemaEvolution reloads the target with new_col, but the resolved VariableReference is not reconsidered; the variable then wins over the target column and an update/delete predicate can affect the wrong rows. Please gate the last-resort step on schema evolution being disabled or being ready with no pending changes (across all MERGE condition forms), and add a collision test that distinguishes per-row evolved-column values from the variable value.
Recommended change: Delay last-resort resolution for MERGE conditions until schema evolution is disabled or the command is ready and has no pending schema changes, then add focused MERGE schema-evolution coverage where a newly added target column collides with a SQL variable.
Why this works: Use the command's existing final-schema readiness facts to keep condition attributes unresolved while assignments establish and apply pending schema changes. On the fixed-point pass after the target relation is reloaded, resolve ordinary target/source columns first and only then run outer-reference and SQL-variable fallback.
Scope: Coordinate MERGE condition fallback with final target-schema readiness and verify variable/column precedence across the schema-evolution iteration boundary.
Compatibility: Ordinary target/source columns continue to win over outer references and SQL variables, and missing names still fall back to variables only after normal column resolution fails.
Risks: The readiness gate must still allow assignment resolution to make schema evolution ready and must not prevent fixed-point convergence. Deferring fallback must not suppress source-only condition variables once the final target schema is established.
Constraints: Preserve table/source column precedence over outer references and SQL variables. Keep assignment-value resolution outside this PR's condition-only scope. Retain existing behavior for MERGE statements without schema evolution and for schema-evolution commands with no pending changes.
Success: A name that becomes a target column through the current MERGE schema evolution resolves to that column before a same-named SQL variable in every target-visible MERGE condition. SQL variables still resolve in all MERGE condition families after the command reaches its final target schema. MERGE behavior without schema evolution and assignment-value resolution remain unchanged.
What changes were proposed in this pull request?
SQL variables declared with
DECLAREcannot be referenced in the conditions of anUPDATEorMERGE INTOstatement. This passesincludeLastResort = trueat the ninecondition-resolution sites that make up those clauses, matching what SPARK-57260 did for
OverwriteByExpression.deleteExpr.Variable resolution only runs from
resolveColsLastResort, which is reached whenresolveExpressionByPlanOutput/resolveExpressionByPlanChildrenare called withincludeLastResort = true. Both default the flag tofalse. Plans with no dedicatedresolution rule fall through to the generic operator case in
ResolveReferences, whichdoes pass the flag -- which is why
DELETE ... WHEREalready works.UPDATEandMERGE INTOeach have a dedicated rule that omitted it:UPDATEcondition (ResolveReferencesInUpdate)MERGEONconditionMERGEWHEN MATCHEDDELETE/UPDATEconditionsMERGEUPDATE *conditionMERGEWHEN NOT MATCHEDINSERT/INSERT *conditionsMERGEWHEN NOT MATCHED BY SOURCEDELETE/UPDATEconditionsThis covers every
MergeActioncondition form, so allMERGEconditions now resolvevariables consistently.
Assignment values (
SET col = var,INSERT VALUES (var)) are affected by the same rootcause, but they resolve through
resolveExprInAssignment, which setsincludeLastResort = falseexplicitly rather than by default. That is left unchangedhere pending a decision on whether the explicit
falsewas deliberate, so this PR isscoped to conditions only.
Why are the changes needed?
Variables resolve in
SELECT,INSERT(includingREPLACE WHERE) andDELETE, but inno condition of
UPDATEorMERGE INTO, which fails analysis with:The message itself offers "A column, variable, or function parameter", so the analyzer
reports that variable resolution was attempted. Nothing in
error-conditions.json, thetests, 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:
After, the statement analyzes and executes, resolving
target_depto the declaredvariable. 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
withSessionVariablehelper inRowLevelOperationSuiteBase:UpdateTableSuiteBase-- a session variable and a SQL scripting local variable in anUPDATEcondition.MergeIntoTableSuiteBase-- a variable in theMERGEONcondition, in theWHEN MATCHED/WHEN NOT MATCHED BY SOURCEconditions, and in theWHEN NOT MATCHEDINSERTandINSERT *conditions.Each test was verified to fail without the fix (raising
UNRESOLVED_COLUMN) and to passwith 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,DeltaBasedUpdateTableSuiteandDeltaBasedMergeIntoTableSuite-- 289 tests, allpassing. Reverting only the
Analyzerchange makes the two not-matched tests fail withUNRESOLVED_COLUMNon the variable in theinsertactioncondition.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code