| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
…ditions Signed-off-by: joelrobin18 <joelrobin1818@gmail.com>
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.
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.
Sorry, something went wrong.
…sert conditions Signed-off-by: joelrobin18 <joelrobin1818@gmail.com>
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.
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.
Sorry, something went wrong.
| catalog.createTable(ident, tableInfo) | ||
| } | ||
|
|
||
| /** Declares a session variable for the duration of `f`, dropping it afterwards. */ |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| assignments = newAssignments, | ||
| condition = u.condition.map(resolveExpressionByPlanChildren(_, u))) | ||
| condition = u.condition.map( | ||
| resolveExpressionByPlanChildren(_, u, includeLastResort = true))) |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
…el tests Signed-off-by: joelrobin18 <joelrobin1818@gmail.com>
| Back | FazBrowse Home | New Git URL |
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:
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:
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:
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:
UPDATE condition.
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