[SPARK-56681][SQL] PATH cleanup#55647
Closed
srielau wants to merge 12 commits into
Closed
Conversation
### What changes were proposed in this pull request? Address the open follow-ups from SPARK-56681 (umbrella for PATH / SPARK-56605 cleanup) in a single cleanup PR. Items #1 and #2 were already wired by SPARK-56639; this PR covers the remainder: - #3 (audit only): document the lifecycle of `AnalysisContext.resolutionPathEntries` and confirm `withNewAnalysisContext` / `reset()` correctly clear it. The full removal of the field requires plumbing the path through `RelationResolution` / `FunctionResolution` method calls and is flagged as a follow-up in the docstring. - apache#4: extract `Analyzer.runWithSessionConf` and add `SQLConf.getExistingConfIfSet`. `executeAndCheck` and `executeSameContext` now share one helper that yields to any outer `SQLConf.withExistingConf` scope (so persisted view / SQL-UDF configs are honored) instead of unconditionally clobbering it. - apache#5: short-circuit `VariableResolution.allowUnqualifiedSessionTempVariableLookup` when PATH is not explicitly set, so column resolution does not force-load the configured default catalog (which would raise `CATALOG_NOT_FOUND` if it is misconfigured). - apache#6: remove the `DROP VARIABLE` PATH gate in `ResolveCatalogs`. DDL on session variables (`DECLARE` / `CREATE` / `DROP`) always targets `system.session` directly, regardless of PATH; only DML (`SET VAR`, `SELECT @x`) goes through PATH. - apache#7: narrow the exception swallow in `FunctionResolution.lookupFunctionType` from `NonFatal` to the explicit not-found list (`NoSuchFunctionException`, `NoSuchNamespaceException`, `CatalogNotFoundException`, `FORBIDDEN_OPERATION`). `PERMISSION_DENIED`, transient catalog errors, and unrelated bugs now surface instead of being masked as `UNRESOLVED_ROUTINE`. - apache#8: filter `system.*` candidates out of the persistent fan-out in `lookupFunctionType`. Routines under `system.session`, `system.builtin`, and `system.ai` are already resolved by `lookupBuiltinOrTempFunction` / `lookupBuiltinOrTempTableFunction` earlier in the same method, so the extra `functionExists` calls were wasteful catalog round-trips. - apache#9: lift `resolutionPathEntriesForAnalysis(pinnedEntries, viewCatalogAndNamespace)` onto `CatalogManager` as the single source of truth used by relation, routine, and procedure resolution. The near-duplicate helpers in `RelationResolution` and `FunctionResolution` now route through it. - apache#11: clone `spark.sessionState.conf` for `ProtoToParsedPlanTestSuite.analyzerIsolationConf` instead of starting from a bare `new SQLConf()`. All `sparkConf` overrides (ANSI, alias config, ...) propagate automatically; only the genuine isolation knob (`PATH_ENABLED=false`) is overridden explicitly. - Bonus (umbrella comment): `UNRESOLVED_VARIABLE` now reports the actual SQL PATH searched. `unresolvedVariableError` is collapsed into a single method that takes `Seq[Seq[String]]` path entries with an optional `Origin` and renders `searchPath` as `[entry1, entry2, ...]` -- byte-for-byte consistent with `UNRESOLVED_ROUTINE` and `TABLE_OR_VIEW_NOT_FOUND`. ### Why are the changes needed? These are the cleanup items called out on SPARK-56681 from the post-merge source review of SPARK-56605. They eliminate dead code paths, plug user-visible bugs (force-loading a misconfigured default catalog on column resolution; clobbering pinned session configs; swallowing real catalog errors as `UNRESOLVED_ROUTINE`), remove the asymmetry between DDL and DML on session variables, and make `UNRESOLVED_VARIABLE` self-consistent with the other "not found" errors. ### Does this PR introduce _any_ user-facing change? Yes: - `UNRESOLVED_VARIABLE.searchPath` is now always rendered as a bracketed list (`` [`system`.`session`] ``), matching `UNRESOLVED_ROUTINE` and `TABLE_OR_VIEW_NOT_FOUND`. For unqualified `SET VAR` failures the list reflects the actual SQL PATH that was consulted instead of a hardcoded `SYSTEM.SESSION`. The error also carries a `queryContext` for `SET VAR` since `ResolveSetVariable` now passes the current origin. - `DROP TEMPORARY VARIABLE` no longer raises `UNRESOLVED_VARIABLE` when the SQL PATH does not contain `system.session`. DDL on session variables ignores PATH; this matches the existing behaviour of `DECLARE OR REPLACE VARIABLE`. - `lookupFunctionType` no longer swallows non-`NotFound` errors. A catalog reporting `PERMISSION_DENIED` (or similar) for a function lookup now propagates instead of silently producing `UNRESOLVED_ROUTINE`. ### How was this patch tested? - Updated `SetPathSuite` (DECLARE / SET VAR / DROP cycle under a non-default PATH; updated bonus test to assert the actual rendered search path). - Updated `SqlScriptingExecutionSuite` (local-variable SET error expectation) and the `sql-session-variables.sql` golden files (regenerated) for the new bracketed `searchPath` format. - Added `resolutionPathEntriesForAnalysis` stubs to mocked `CatalogManager` instances in `PlanResolutionSuite`, `AlignAssignmentsSuiteBase`, and `TableLookupCacheSuite`. - Ran focused suites locally: - `build/sbt 'sql/testOnly *SetPathSuite *SqlScriptingExecutionSuite *ExecuteImmediateEndToEndSuite'` - `build/sbt 'sql/testOnly *SimpleSQLViewSuite *SQLFunctionSuite'` - `build/sbt 'sql/testOnly *PlanResolutionSuite *UpdateTableAlignAssignmentsSuite *MergeIntoTableAlignAssignmentsSuite'` - `build/sbt 'catalyst/testOnly *TableLookupCacheSuite *AnalysisSuite *AnalysisErrorSuite *LookupFunctionsSuite'` - `build/sbt 'sql/testOnly *FunctionQualificationSuite *RelationQualificationSuite *DataSourceV2FunctionSuite'` - `build/sbt 'sql/testOnly *SQLQuerySuite'` - `build/sbt 'connect/testOnly *ProtoToParsedPlanTestSuite'` - `build/sbt 'sql/testOnly *SQLQueryTestSuite -- -z sql-session-variables.sql'` - All catalyst analysis suites and single-pass resolver suites. - `scalastyle` clean across catalyst, sql, and connect modules. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Cursor Claude Opus 4.7
Replace comments that describe how the code came to be (history / JIRA pointers) with comments that describe what the code does: - `ResolveSetVariable`: drop the rendering rationale at the throw site -- documented at `unresolvedVariableError` / `searchPathEntriesForError`. - `FunctionResolution.lookupFunctionType`: tighten the system-prefix filter and exception-list comments. - `ResolveCatalogs`: tighten the DROP VARIABLE policy comment. - `Analyzer.AnalysisContext.resolutionPathEntries`: condense lifecycle paragraph and replace prose follow-up with a single TODO marker. - `FunctionResolution.sqlResolutionPathEntriesForAnalysis`: state the invariant (relation/routine share path entries) instead of narrating the routing. - `SetPathSuite`, `SqlScriptingExecutionSuite`: drop test-side prose about why the rendering changed. No behavior change.
Variables only live in `system.session` -- there is no persistent variable catalog -- so the gate and the search-path-for-error are both simply "is `system.session` on the SQL path?". - Replace `CatalogManager.sessionScopeUnqualifiedAllowed(catalog, namespace)` with `CatalogManager.isSystemSessionOnPath`, which inspects the stored session path entries directly. No catalog load, no marker expansion: `CurrentSchemaEntry` resolves to the current schema and can never be `system.session`, so only literal entries matter. - `VariableResolution.allowUnqualifiedSessionTempVariableLookup` collapses to a one-liner: pass through qualified names, gate unqualified ones on `isSystemSessionOnPath`. - `VariableResolution.searchPathEntriesForError` always returns `[Seq(SYSTEM, SESSION)]` -- there is nowhere else variables can live, regardless of name-part count or PATH contents. Test expectation updates for the new always-`[system.session]` shape: `SetPathSuite` and `SqlScriptingExecutionSuite`. Golden files unchanged (already render `[system.session]` from the qualified path). No behavior change beyond the error message: variable lookup still only consults `system.session`.
…sion is off PATH
If the user removes `system.session` from the SQL path, an unqualified
SET VAR lookup never actually consults any namespace. Reporting
`[system.session]` as the search path in that case would be a lie:
nothing was searched. Return `[]` instead so the error reflects what
the resolver actually did.
`searchPathEntriesForError`:
- `system.session` on PATH -> `[Seq("system", "session")]`
- `system.session` off PATH -> `Seq.empty`
`SetPathSuite` updated to expect `[]` for the off-PATH case.
…IABLE Resolution and search-path reporting are separate concerns: - The resolver may *optimize* by skipping path entries where the looked-up object kind cannot live (e.g. `RelationResolution` skips `system.builtin`; the variable gate skips everything except `system.session`). - The reported `searchPath` is the full path the resolver *would* consult: `TABLE_OR_VIEW_NOT_FOUND` keeps `system.builtin` in its list even though no tables can be there. `UNRESOLVED_VARIABLE` should follow the same convention. For unqualified names, report the full resolved SQL path; for qualified names, report `[system.session]` (PATH does not apply). `isSystemSessionOnPath` is still used as the lookup gate, so the hot path never loads the configured default catalog. The full-path read in `searchPathEntriesForError` only happens at error time. Test expectations updated: `SetPathSuite` and `SqlScriptingExecutionSuite` now assert the actual full SQL PATH.
Reduce diff against `apache/spark master` by undoing changes that did not carry their weight: - Drop `unresolvedVariableError(name, pathEntries, origin)` overload and the corresponding `CurrentOrigin.get` pass in `ResolveSetVariable`. Master never had a caller for the origin form, and historical `UNRESOLVED_VARIABLE` errors don't carry a `queryContext`. Single 2-arg overload is enough. - Restore master's docstrings on `AnalysisContext.resolutionPathEntries`, `FunctionResolution.sqlResolutionPathEntriesForAnalysis`, `RelationResolution.relationResolutionEntries`, and `VariableResolution.allowUnqualifiedSessionTempVariableLookup`. The behaviour each describes is unchanged; the new prose was just narration. - Remove the now-unused doc on `unresolvedVariableError` and tighten `CatalogManager.isSystemSessionOnPath`. - Regenerated golden files have a single mechanical change per hunk: add brackets to `searchPath`. No more added `queryContext` blocks. Net effect: ~25% fewer lines added vs master while keeping all 11 items + bonus addressed and all tests passing.
…bleError Every variable-resolution failure now carries a `queryContext` pointing at the SQL fragment that triggered it, matching what `TABLE_OR_VIEW_NOT_FOUND` and `UNRESOLVED_ROUTINE` already do. - Collapse `unresolvedVariableError` to a single 3-arg signature with required `Origin`. No optional default, no two-overload pattern. - `VariableManager.set` now takes an `origin: Origin` parameter that it forwards to `unresolvedVariableError`. Both implementations (`TempVariableManager`, `SqlScriptingLocalVariableManager`) and all three callers (`SetVariableExec`, `FetchCursorExec`, `VariableAssignmentUtils`) updated to thread the calling `VariableReference`'s origin. - Analyzer-rule sites pass `CurrentOrigin.get`: `ResolveSetVariable`, `ResolveCatalogs.assertValidSessionVariableNameParts`, `ResolveFetchCursor`. Regenerated `sql-session-variables.sql.out` shows every UNRESOLVED_VARIABLE error now lists `searchPath` as a bracketed list and carries a `queryContext` with the offending SQL fragment.
Previously `UNRESOLVED_VARIABLE.queryContext` highlighted the entire SQL statement (e.g. `DECLARE OR REPLACE VARIABLE builtin.var1 INT`) because `withIdentClause` deliberately does not wrap its builder in `withOrigin(ctx)`. The identifier-specific origin per the `withIdentClause` doc comment is opt-in per call site. Opt in for the three variable parser paths so the error highlights just the variable identifier: - `visitCreateVariable`: wrap each `UnresolvedIdentifier` with `withOrigin(identifierReference)`. - `visitDropVariable`: wrap the `UnresolvedIdentifier` with `withOrigin(ctx.identifierReference())`. - `visitSetVariableImpl`: wrap each `UnresolvedAttribute` with `withOrigin(variableIdent)` / `withOrigin(assign.key)`. Plumb the inner identifier's origin through the analyzer rules (`u.origin` instead of `CurrentOrigin.get`): - `ResolveSetVariable` (`UnresolvedAttribute`). - `ResolveFetchCursor` (`UnresolvedAttribute`). - `ResolveCatalogs.assertValidSessionVariableNameParts` now takes an explicit `origin: Origin`; both call sites pass the matched `UnresolvedIdentifier`'s origin. Regenerated `sql-session-variables.sql.out` confirms the fragment is now the variable identifier (e.g. `"builtin.var1"`, `"ses.var1"`) instead of the whole statement. `SetPathSuite` / `SqlScriptingExecutionSuite` `ExpectedContext` updated to match.
…_VARIABLE DML lookup failures (`SET VAR`, `FETCH ... INTO`) now report the full SQL path regardless of how the name was qualified, matching the convention used by `TABLE_OR_VIEW_NOT_FOUND` and `UNRESOLVED_ROUTINE`. Qualification-independent rendering also future-proofs the error shape: if Spark ever grows struct-field assignment, 2-part forms like `SET VAR session.foo = ...` become genuinely ambiguous between qualified-variable and field-access interpretations -- and the resolver tries multiple interpretations through PATH-gated lookup. The error format will not have to change. DDL (`DECLARE` / `DROP` name validation) is unchanged: it does not consult the SQL path and continues to report `[system.session]` directly from `ResolveCatalogs.assertValidSessionVariableNameParts`. Added regression test `Dropped struct variable -- field access vs qualified name` to `sql-session-variables.sql` demonstrating the field-vs-qualified ambiguity and that `SELECT session.a` after dropping a struct variable falls through to `UNRESOLVED_COLUMN`. `searchPathEntriesForError` now takes no arguments and always returns the full path from `CatalogManager.resolutionPathEntriesForAnalysis`.
…uite
CI complained about an unformatted scaladoc line. Fix is purely a
line-wrap change emitted by
./build/mvn scalafmt:format -Dscalafmt.skip=false \
-Dscalafmt.validateOnly=false -Dscalafmt.changedOnly=false \
-pl sql/api -pl sql/connect/common -pl sql/connect/server \
-pl sql/connect/shims -pl sql/connect/client/jvm
cloud-fan
reviewed
May 2, 2026
Contributor
cloud-fan
left a comment
cloud-fan
left a comment
There was a problem hiding this comment.
Summary
Prior state and problem. SPARK-56605 introduced the SQL PATH machinery (search path for routines, relations, variables) and SPARK-56639 wired most of it. The SPARK-56681 umbrella enumerated 11 follow-ups from the post-merge review: dead code, an unfinished gate, three near-duplicate path-resolution helpers, an over-broad NonFatal swallow that hid permission errors as UNRESOLVED_ROUTINE, a hot-path currentCatalog load that could fault on misconfigured DEFAULT_CATALOG, an asymmetric DROP TEMPORARY VARIABLE PATH gate (DDL behaved differently than DECLARE/CREATE), executeAndCheck clobbering an outer SQLConf.withExistingConf (e.g. SQL UDF / view body's pinned configs), a bare SQLConf in ProtoToParsedPlanTestSuite that diverged from sparkConf, and UNRESOLVED_VARIABLE hardcoding SYSTEM.SESSION regardless of the actual SQL path.
Design approach. Centralize the three near-duplicate path helpers into one shared method CatalogManager.resolutionPathEntriesForAnalysis(pinnedEntries, viewCatalogAndNamespace) that all of routine, relation, and variable resolution route through. Replace the hot-path catalog read in unqualified variable lookup with CatalogManager.isSystemSessionOnPath which inspects stored session-path entries directly. Lift conf-yielding logic out of executeAndCheck / executeSameContext into a runWithSessionConf helper that yields to any outer withExistingConf scope (via the new SQLConf.getExistingConfIfSet peek). For variable errors, change UNRESOLVED_VARIABLE.searchPath from a single dotted path to a bracketed list of path entries (matching TABLE_OR_VIEW_NOT_FOUND / UNRESOLVED_ROUTINE); make Origin mandatory on unresolvedVariableError and plumb it through VariableManager.set so all error sites carry a queryContext pointing at the variable identifier.
Key design decisions.
resolutionPathEntriesForAnalysislives inCatalogManager(Coordination) and takesAnalysisContextdata as parameters rather than reading the threadlocal directly — preserves the package boundary soconnector.catalogdoesn't depend onanalysis.runWithSessionConfusesgetExistingConfIfSet(threadlocal peek) instead ofSQLConf.get. When there's no outerwithExistingConfscope butSQLConf.getdiffers from the analyzer's pinned conf, the new code installs the pinned conf where the oldexecuteSameContextwould yield. This is the deliberate fix for #4: analyzer isolation should win over the active session conf when no explicit outer scope was opened.DROP TEMPORARY VARIABLE's PATH gate is removed entirely (rather than mirroring it ontoDECLARE/CREATE): DDL on session variables targetssystem.sessiondirectly, symmetric withDECLARE/CREATE OR REPLACE.UNRESOLVED_VARIABLE.searchPathrendering switches toSeq[Seq[String]]+ bracketed mkString. Old single-arg overload removed (no overloads).
Implementation sketch.
- Coordination (
CatalogManager): addsisSystemSessionOnPathandresolutionPathEntriesForAnalysis; removessessionScopeUnqualifiedAllowed. - Strategy (
FunctionResolution,RelationResolution,VariableResolution): all three delegate to the new shared method.FunctionResolution.lookupFunctionTypeadditionally narrows the swallowed exception list (matches the pre-existing pattern at lines 146-153) and filterssystem.*candidates from the persistent loop. - Resolution rules (
ResolveCatalogs,ResolveSetVariable,ResolveFetchCursor): DROP path gate removed;Originplumbed throughassertValidSessionVariableNameParts. - Storage (
TempVariableManager,SqlScriptingLocalVariableManager,VariableManagertrait):setadds anorigin: Originparameter, mechanically threaded to error sites. - Analyzer:
runWithSessionConfhelper used by bothexecuteAndCheckandexecuteSameContext;SQLConf.getExistingConfIfSetexposes the threadlocal peek. - Parser (
AstBuilder):withOrigin(identifierReference)opt-ins forCreateVariable/DropVariable/SetVariableso error highlights pin to the variable identifier. - Tests: golden file regen for new bracketed
searchPathand identifier-pinnedqueryContext; new struct-variable-vs-column ambiguity test; DECLARE/SET VAR/DROP cycle test; mockedCatalogManagerstubs in three suites.
One real bug below (ResolveFetchCursor still hardcodes the path, contradicting the bonus item) plus a few clarity questions on the design.
- ResolveFetchCursor: route through `variableResolution.searchPathEntriesForError`
instead of hardcoding `Seq(Seq("SYSTEM", "SESSION"))`, so `FETCH ... INTO`
miss reports the full SQL path. Bonus item now consistent for SET VAR and
FETCH INTO.
- CatalogManager.isSystemSessionOnPath: spell out the load-bearing assumption
("`system` is not a registered catalog") so a future change that registers
a real catalog under `system` won't silently break the short-circuit.
- FunctionResolution.lookupFunctionType: tighten the `system.*` filter to
match the actually-resolved set (`system.session`, `system.builtin`)
rather than any prefix beginning with `system`. A future namespace under
`system.<x>` not covered by `identifierFromSystemNameParts` will now go
through persistent lookup as expected.
cloud-fan
approved these changes
May 3, 2026
Contributor
cloud-fan
left a comment
cloud-fan
left a comment
There was a problem hiding this comment.
Re-review at c2f8f64 — 4 addressed, 0 remaining, 1 new (late catch).
All four prior findings addressed:
- ResolveFetchCursor now routes through
variableResolution.searchPathEntriesForError. - FunctionResolution filter narrowed to
system.session/system.builtinonly — tracks exactly whatidentifierFromSystemNamePartsaccepts. - CatalogManager Scaladoc spells out the load-bearing assumption (
systemis not a registered catalog). - VariableResolution searchPath rendering confirmed deliberate (full-path matches
TABLE_OR_VIEW_NOT_FOUND/UNRESOLVED_ROUTINE; the existing doc onsearchPathEntriesForErroralready captures the rationale).
One late catch inline below: the FETCH … INTO fix is uncovered by tests — adding a symmetric SetPathSuite (or SqlScriptingCursorE2eSuite) case would lock in the bracketed-list rendering for the path the bonus item explicitly promises.
LGTM otherwise.
Symmetric coverage for the bonus item alongside the existing SET VAR test in SetPathSuite. Verifies that under a non-default PATH that excludes system.session, an unqualified FETCH ... INTO target fails with UNRESOLVED_VARIABLE and the searchPath is the bracketed list of the actual SQL path -- locking in the behavior wired up in the previous commit (FETCH ... INTO routes through `variableResolution.searchPathEntriesForError`).
Contributor
|
LGTM, the one CI failure looks flaky/unrelated. Merging to master |
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Address the open follow-ups from SPARK-56681 (umbrella for PATH / SPARK-56605 cleanup) in a single cleanup PR. Items #1 and #2 were already wired by SPARK-56639; this PR covers the remainder.
FunctionResolution.resolveProcedurewas dead codeAnalysisContext.resolutionPathEntriesthreadlocalwithNewAnalysisContext/reset()correctly clear it. Full removal needs a coordinated refactor to plumb the path throughRelationResolution/FunctionResolutionmethod calls; flagged as a follow-up.Analyzer.executeAndCheckclobbers outerSQLConf.withExistingConfrunWithSessionConfhelper, addedSQLConf.getExistingConfIfSet.executeAndCheckandexecuteSameContextnow share one path that yields to any outer scope.VariableResolution.allowUnqualifiedSessionTempVariableLookupforce-loads default catalogCatalogManager.isSystemSessionOnPath, which inspects stored session-path entries directly. No catalog load on column resolution.DROP VARIABLEPATH gate asymmetric withDECLARE/CREATEDECLARE/CREATE/DROP) always targetssystem.sessiondirectly; only DML (SET VAR,SELECT @x) goes through PATH.lookupFunctionTypeexception swallow too broadNonFatalto the explicit not-found list (NoSuchFunctionException,NoSuchNamespaceException,CatalogNotFoundException,FORBIDDEN_OPERATION). Other exceptions propagate.lookupFunctionTypefan-out had wastefulsystem.*candidatessystem.session,system.builtin,system.aiare already resolved earlier in the same method.CatalogManager.resolutionPathEntriesForAnalysis(pinnedEntries, viewCatalogAndNamespace). Relation, routine, and procedure resolution all route through it.sql-session-variables.sql.ProtoToParsedPlanTestSuite.analyzerIsolationConfwas a bareSQLConfspark.sessionState.confand only overridePATH_ENABLED=false, so allsparkConfoverrides (ANSI, alias config, ...) propagate automatically.ResolveSetVariablehardcodedSYSTEM.SESSIONregardless of actual PATHunresolvedVariableErrornow takesSeq[Seq[String]]path entries with requiredOrigin(no overloads). DML lookup failures (SET VAR,FETCH ... INTO) report the full SQL path as a bracketed list, byte-for-byte consistent withUNRESOLVED_ROUTINEandTABLE_OR_VIEW_NOT_FOUND. DDL name validation inResolveCatalogscontinues to report[system.session]since PATH does not apply there. Origin is plumbed throughVariableManager.setso all error sites carry aqueryContextpointing at the offending variable identifier (parser opt-ins viawithOrigin(identifierReference)so the highlight is the variable name, not the whole statement).Why are the changes needed?
These are the cleanup items called out on SPARK-56681 from the post-merge source review of SPARK-56605. They eliminate dead code paths, plug user-visible bugs (force-loading a misconfigured default catalog on column resolution; clobbering pinned session configs; swallowing real catalog errors as
UNRESOLVED_ROUTINE), remove the asymmetry between DDL and DML on session variables, and makeUNRESOLVED_VARIABLEself-consistent with the other "not found" errors.Does this PR introduce any user-facing change?
Yes.
UNRESOLVED_VARIABLE.searchPathis now rendered as a bracketed list. For DML lookups (SET VAR,FETCH ... INTO), the list reflects the actual SQL PATH that was consulted instead of a hardcodedSYSTEM.SESSION. For DDL name validation (DECLARE/DROPwith a non-session namespace), the list is[``system.session``]since PATH does not apply.UNRESOLVED_VARIABLEnow always carries aqueryContextthat highlights just the offending variable identifier (e.g."builtin.var1","ses.var1"), not the wholeDECLARE/SET VARstatement.DROP TEMPORARY VARIABLEno longer raisesUNRESOLVED_VARIABLEwhen the SQL PATH does not containsystem.session. DDL on session variables ignores PATH, matching the existing behaviour ofDECLARE OR REPLACE VARIABLE.lookupFunctionTypeno longer swallows non–NotFounderrors. A catalog reportingPERMISSION_DENIED(or similar) for a function lookup now propagates instead of silently producingUNRESOLVED_ROUTINE.How was this patch tested?
sql-session-variables.sqlregression test for the struct-variable field-vs-qualified ambiguity (DECLARE VARIABLE session STRUCT<a INT>→SELECT session.asucceeds →DROP→SELECT session.afalls through toUNRESOLVED_COLUMN).SetPathSuite: DECLARE / SET VAR / DROP cycle under a non-default PATH; bonus test asserts the actual rendered search path and the variable-identifierqueryContext.SqlScriptingExecutionSuitefor the new bracketedsearchPathand identifier-pinnedqueryContext.sql-session-variables.sql.outfor the new error shape.resolutionPathEntriesForAnalysisstubs to mockedCatalogManagerinstances inPlanResolutionSuite,AlignAssignmentsSuiteBase, andTableLookupCacheSuite.build/sbt 'sql/testOnly *SetPathSuite *SqlScriptingExecutionSuite *ExecuteImmediateEndToEndSuite'build/sbt 'sql/testOnly *SimpleSQLViewSuite *SQLFunctionSuite'build/sbt 'sql/testOnly *PlanResolutionSuite *UpdateTableAlignAssignmentsSuite *MergeIntoTableAlignAssignmentsSuite'build/sbt 'catalyst/testOnly *TableLookupCacheSuite *AnalysisSuite *AnalysisErrorSuite *LookupFunctionsSuite'build/sbt 'sql/testOnly *FunctionQualificationSuite *RelationQualificationSuite *DataSourceV2FunctionSuite'build/sbt 'sql/testOnly *SQLQuerySuite'build/sbt 'connect/testOnly *ProtoToParsedPlanTestSuite'build/sbt 'sql/testOnly *SQLQueryTestSuite -- -z sql-session-variables.sql'org.apache.spark.sql.catalyst.analysis.*,org.apache.spark.sql.catalyst.parser.*, andorg.apache.spark.sql.analysis.resolver.*suites.scalastyleandscalafmtclean across catalyst, sql, and connect modules.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Cursor Claude Opus 4.7