srielau commented on code in PR #55647:
URL: https://github.com/apache/spark/pull/55647#discussion_r3177635417
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveFetchCursor.scala:
##########
@@ -64,7 +64,8 @@ class ResolveFetchCursor(val catalogManager: CatalogManager)
extends Rule[Logica
nameParts = u.nameParts
) match {
case Some(variable) => variable.copy(canFold = false)
- case _ => throw unresolvedVariableError(u.nameParts, Seq("SYSTEM",
"SESSION"))
+ case _ => throw unresolvedVariableError(
+ u.nameParts, Seq(Seq("SYSTEM", "SESSION")), u.origin)
Review Comment:
Good catch -- this was a real miss. Fixed in c2f8f642c45: `FETCH ... INTO`
now routes through `variableResolution.searchPathEntriesForError` and reports
the full SQL path consistently with `SET VAR`.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/VariableResolution.scala:
##########
@@ -43,10 +43,25 @@ class VariableResolution(
* (PATH enabled and explicitly set).
*/
private def allowUnqualifiedSessionTempVariableLookup(nameParts:
Seq[String]): Boolean = {
- if (nameParts.length != 1) return true
- catalogManager.sessionScopeUnqualifiedAllowed(
- catalogManager.currentCatalog.name(),
- catalogManager.currentNamespace.toSeq)
+ nameParts.length != 1 || catalogManager.isSystemSessionOnPath
+ }
+
+ /**
+ * Search-path entries to report in `UNRESOLVED_VARIABLE` for DML lookups
(`SET VAR`,
+ * `FETCH ... INTO`). The full SQL path is reported regardless of how the
name was
+ * qualified, matching the convention used by `TABLE_OR_VIEW_NOT_FOUND` and
+ * `UNRESOLVED_ROUTINE`. Keeping the rendering qualification-independent
also avoids
+ * re-shaping the error if Spark ever grows struct-field assignment, where
2-part forms
+ * become genuinely ambiguous.
+ *
+ * DDL paths (`DECLARE` / `DROP` name validation in
+ * [[org.apache.spark.sql.catalyst.analysis.ResolveCatalogs]]) do not
consult the SQL path
+ * and report `[system.session]` directly at their throw site.
+ */
+ def searchPathEntriesForError: Seq[Seq[String]] = {
Review Comment:
Yes, deliberate. Two reasons:
1. **Consistency with the other `*_NOT_FOUND` errors.**
`TABLE_OR_VIEW_NOT_FOUND` keeps `system.builtin` in its rendered `searchPath`
even though no tables can ever live there, and `UNRESOLVED_ROUTINE` likewise
reports the full path regardless of qualification. The reported `searchPath` is
"the path the resolver would consult", not "the path that could possibly hold
this object kind". Filtering for variables would make `UNRESOLVED_VARIABLE` the
third inconsistent variant in this family.
2. **Future-proofing against struct-field assignment.** If `SET VAR
session.foo = ...` ever gains a struct-field-assignment interpretation, 2-part
forms become genuinely ambiguous between qualified-variable and field-access.
The resolver would then try multiple interpretations through PATH-gated lookup,
and the error format wouldn't have to change.
`isSystemSessionOnPath` keeps the *resolution-time* check cheap and accurate
(no spurious lookups in non-session namespaces), so resolution and reporting
are decoupled: optimization in the gate, full faithful path in the error. The
added doc on `searchPathEntriesForError` calls this out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
