srielau commented on code in PR #55523:
URL: https://github.com/apache/spark/pull/55523#discussion_r3140883299
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/RelationResolution.scala:
##########
@@ -195,15 +229,15 @@ class RelationResolution(
).orElse(tryResolvePersistent(u, identifier, finalTimeTravelSpec))
}
- // 1-part name: try each scope in relationResolutionSearchPath order (from
- // [[SQLConf.resolutionSearchPath]]).
- val candidates = relationResolutionSearchPath
- for (scope <- candidates) {
- val result = scope match {
- case SessionScope =>
+ // 1-part name: try each step in [[relationResolutionSteps]] order (from
+ // [[SQLConf.sqlResolutionPathEntries]]).
Review Comment:
Same broken `[[SQLConf.sqlResolutionPathEntries]]` ref.
```suggestion
// 1-part name: try each step in [[relationResolutionSteps]] order (from
// [[CatalogManager.sqlResolutionPathEntries]]).
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/v2ResolutionPlans.scala:
##########
@@ -68,15 +68,39 @@ case class UnresolvedView(
allowTemp: Boolean,
suggestAlternative: Boolean = false) extends UnresolvedLeafNode
+/**
+ * Controls which search path is shown in `TABLE_OR_VIEW_NOT_FOUND` for
+ * [[UnresolvedTableOrView]] (see
[[org.apache.spark.sql.catalyst.analysis.CheckAnalysis]]).
+ */
+sealed trait UnresolvedTableOrViewSearchPathMode
+
+object UnresolvedTableOrViewSearchPathMode {
+ /** DDL on catalog objects: `system.session` and current catalog namespace
only. */
+ case object Ddl extends UnresolvedTableOrViewSearchPathMode
+ /**
+ * Like `SELECT` / DML: full `sqlResolutionPathEntries` order; fully
qualified
+ * `system.session.*` names still use the temp-view-only path in errors.
+ */
+ case object QueryLike extends UnresolvedTableOrViewSearchPathMode
+ /** Commands that only target temp views (e.g. some `DROP TEMPORARY VIEW`
paths). */
+ case object TempViewOnly extends UnresolvedTableOrViewSearchPathMode
+}
Review Comment:
`QueryLike` and `TempViewOnly` are dead — `rg
'UnresolvedTableOrViewSearchPathMode\.(QueryLike|TempViewOnly)'` only finds the
case-object definitions and the matches in `CheckAnalysis`. Every
`UnresolvedTableOrView(...)` call in `AstBuilder` and `SparkSqlParser` uses the
3-arg form and gets the default `Ddl`, including
`createUnresolvedTableOrView(ctx.identifierReference, "DESCRIBE TABLE")`. The
old code in `CheckAnalysis` inferred the mode from `commandName` (`"DESCRIBE
…"` → `fullSearchPathForError`, `"… TEMPORARY VIEW …"` →
`tempViewOnlySearchPathForError`); removing that without wiring the enum at the
construction sites is a visible regression for `TABLE_OR_VIEW_NOT_FOUND` on
DESCRIBE.
Either pass `QueryLike` from `SparkSqlParser.visitDescribeRelation` (and
audit AstBuilder for other DESCRIBE/SHOW callers that should match) or restore
the commandName inference in `CheckAnalysis` and drop these two case objects.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/RelationResolution.scala:
##########
@@ -110,34 +114,64 @@ class RelationResolution(
/**
* Scope in the relation resolution search path. Used to interpret
- * [[SQLConf.resolutionSearchPath]] when resolving unqualified table/view
names.
+ * [[SQLConf.sqlResolutionPathEntries]] when resolving unqualified
table/view names.
*/
- private sealed trait RelationResolutionScope
- private case object SessionScope extends RelationResolutionScope
- private case object PersistentScope extends RelationResolutionScope
+ private sealed trait RelationResolutionStep
+ private case object SessionScopeStep extends RelationResolutionStep
+ private case class PersistentCatalogStep(catalogAndNamespace: Seq[String])
+ extends RelationResolutionStep
+
+ /**
+ * Path entries for unqualified relation resolution.
+ *
+ * Inside a view, [[AnalysisContext.resolutionPathEntries]] is usually a
snapshot from
+ * [[AnalysisContext.snapshotViewResolutionPath]]: either
[[CatalogTable.VIEW_RESOLUTION_PATH]]
+ * (frozen at creation, no virtual markers or `system.session` for persisted
views) or, if absent,
+ * [[SQLConf.sqlResolutionPathEntries]] with view default catalog/namespace
and live marker
+ * expansion. The snapshot is honored only when [[SQLConf.PATH_ENABLED]] is
true; when PATH is
+ * disabled, legacy [[SQLConf.sqlResolutionPathEntries]] rules apply.
Review Comment:
Two issues in this Scaladoc:
- `[[AnalysisContext.snapshotViewResolutionPath]]` doesn't exist anywhere in
the repo (`rg snapshotViewResolutionPath` returns only this comment).
- `[[SQLConf.sqlResolutionPathEntries]]` (3x) — the method is on
`CatalogManager`, not `SQLConf`.
```suggestion
/**
* Path entries for unqualified relation resolution.
*
* Inside a view, [[AnalysisContext.resolutionPathEntries]] is usually a
snapshot populated
* when the view is entered: either [[CatalogTable.VIEW_RESOLUTION_PATH]]
(frozen at creation,
* no virtual markers or `system.session` for persisted views) or, if
absent,
* [[CatalogManager.sqlResolutionPathEntries]] with view default
catalog/namespace and live
* marker expansion. The snapshot is honored only when
[[SQLConf.PATH_ENABLED]] is true; when
* PATH is disabled, legacy [[CatalogManager.sqlResolutionPathEntries]]
rules apply.
*/
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/RelationResolution.scala:
##########
@@ -110,34 +114,64 @@ class RelationResolution(
/**
* Scope in the relation resolution search path. Used to interpret
- * [[SQLConf.resolutionSearchPath]] when resolving unqualified table/view
names.
+ * [[SQLConf.sqlResolutionPathEntries]] when resolving unqualified
table/view names.
Review Comment:
`sqlResolutionPathEntries` is on `CatalogManager`, not `SQLConf`.
```suggestion
* [[CatalogManager.sqlResolutionPathEntries]] when resolving unqualified
table/view names.
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/ResolverGuard.scala:
##########
@@ -474,10 +474,10 @@ class ResolverGuard(
private def checkUnresolvedFunction(unresolvedFunction: UnresolvedFunction)
= {
val nameParts = unresolvedFunction.nameParts
val funcName = nameParts.last.toLowerCase(Locale.ROOT)
-
- if (nameParts.length == 1) {
- // Unqualified: same as master (unsupported, non-builtin, or check
children)
- if (isUnsupportedFunction(funcName)) {
+ if (nameParts.size == 1) {
+ // Unqualified: reject if unsupported, else non-builtin or check
children (same as master)
+ if (ResolverGuard.UNSUPPORTED_FUNCTION_NAMES.contains(funcName) ||
+ isUnsupportedFunction(funcName)) {
Review Comment:
Redundant `||`: `isUnsupportedFunction(name)` is defined as
`UNSUPPORTED_FUNCTION_NAMES.contains(name)`, so the two operands are identical.
Either the first operand was meant to be a different check, or it should just
be removed.
```suggestion
if (isUnsupportedFunction(funcName)) {
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -139,17 +139,22 @@ object FakeV2SessionCatalog extends TableCatalog with
FunctionCatalog with Suppo
* even if a temp view `t` has been created.
* @param outerPlan The query plan from the outer query that can be used to
resolve star
* expressions in a subquery.
+ * @param resolutionPathEntries When resolving a view body, the ordered path
for unqualified
+ * relation names (see
[[AnalysisContext.withAnalysisContext]]).
+ * [[None]] outside views: compute from session
+ * [[SQLConf.sqlResolutionPathEntries]].
Review Comment:
Broken ref (`SQLConf` → `CatalogManager`) and the sentence fragment reads
awkwardly.
```suggestion
* When [[None]] (outside views), the path is
computed from the
* session via
[[CatalogManager.sqlResolutionPathEntries]].
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -2078,9 +2093,19 @@ class Analyzer(
throw
QueryCompilationErrors.notAScalarFunctionError(nameParts.mkString("."), f)
case FunctionType.NotFound =>
- val catalogPath =
- catalogManager.currentCatalog.name +:
catalogManager.currentNamespace
- val searchPath =
SQLConf.get.resolutionSearchPath(catalogPath.toSeq)
+ val ctx = AnalysisContext.get.catalogAndNamespace
+ val pathDefault =
+ if (ctx.nonEmpty) ctx
+ else {
+ (catalogManager.currentCatalog.name +:
+ catalogManager.currentNamespace).toSeq
+ }
+ val searchPath = catalogManager
+ .sqlResolutionPathEntries(
+ pathDefault.head,
+ pathDefault.tail.toSeq,
+ catalogManager.currentCatalog.name,
+ catalogManager.currentNamespace.toSeq)
Review Comment:
This `ctx ?: currentCatalog +:
currentNamespace`-then-call-`sqlResolutionPathEntries(4-arg)` pattern is
duplicated identically in `CheckAnalysis.scala:409` and (with
`currentCatalogPath` factored out) in `FunctionResolution.scala:76`. Worth
pulling into one helper. The PR description says `catalogPathForError` now
consults `AnalysisContext.catalogAndNamespace`, but `CheckAnalysis.scala:93`
still returns `currentCatalog.name +: currentNamespace` — making
`catalogPathForError` do that consultation would let `UnresolvedTableOrView`
errors also report the view's path inside view bodies, and would let all three
sites collapse to a single call.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -77,11 +80,13 @@ trait CheckAnalysis extends LookupCatalog with
QueryErrorsBase with PlanToString
}
/**
- * `SQLConf.resolutionSearchPath` entries formatted with [[toSQLId]] for
TABLE_OR_VIEW_NOT_FOUND.
- * Same ordering as relation resolution and routine resolution search paths.
+ * [[SQLConf.sqlResolutionPathEntries]] formatted with [[toSQLId]] for
TABLE_OR_VIEW_NOT_FOUND.
+ * Same ordering as relation and routine resolution search paths.
Review Comment:
Broken ref (`SQLConf` → `CatalogManager`) and the first line is a sentence
fragment.
```suggestion
* Formats [[CatalogManager.sqlResolutionPathEntries]] with [[toSQLId]] for
* TABLE_OR_VIEW_NOT_FOUND. Same ordering as relation and routine
resolution search paths.
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -592,6 +634,60 @@ class FunctionResolution(
errorClass = errorClass,
messageParameters = messageParameters)
}
+
+ /**
+ * Resolves [[UnresolvedProcedure]] for `CALL` / `DESCRIBE PROCEDURE` using
the same multipart
+ * candidates as SQL functions and relations ([[resolutionCandidates]] /
+ * [[sqlResolutionPathEntriesForAnalysis]]). Catalogs that do not implement
+ * [[ProcedureCatalog]] are skipped for unqualified names; an explicitly
catalog-qualified name
+ * that targets a non-[[ProcedureCatalog]] still raises
+ * [[QueryCompilationErrors.missingCatalogProceduresAbilityError]].
+ */
+ def resolveProcedure(unresolved: UnresolvedProcedure): LogicalPlan = {
+ val candidates = resolutionCandidates(unresolved.nameParts)
+ for (multipart <- candidates) {
+ val expandedOpt =
+ try {
+ Some(relationResolution.expandIdentifier(multipart))
+ } catch {
+ case NonFatal(_) => None
+ }
+ expandedOpt.foreach { expanded =>
+ CatalogAndIdentifier.unapply(expanded).foreach { case (catalog, ident)
=>
+ catalog match {
+ case pc: ProcedureCatalog =>
+ try {
+ val procedure = pc.loadProcedure(ident)
+ return ResolvedProcedure(pc, ident, procedure)
+ } catch {
+ case e: AnalysisException => throw e
+ case e: SparkThrowable => throw e
+ case NonFatal(e) =>
+ if (unresolved.nameParts.length > 1) {
+ val cause = e match {
+ case ex: Exception => ex
+ case th => new RuntimeException(th)
+ }
+ throw QueryCompilationErrors.failedToLoadRoutineError(
+ catalog.name +: ident.asMultipartIdentifier,
+ cause)
+ }
Review Comment:
When `unresolved.nameParts.length == 1` and `pc.loadProcedure(ident)` throws
`NonFatal`, the exception is silently swallowed and the loop falls through to
the eventual `unresolvedRoutineError`, which only carries the search path — the
actual catalog failure is lost. For multi-part names the error is wrapped in
`failedToLoadRoutineError` with `cause`. Since `loadProcedure` is the same kind
of call in both branches, consider attaching the last suppressed cause (or
letting it propagate as `failedToLoadRoutineError` once we've at least found a
`ProcedureCatalog`) so users debugging a misbehaving catalog get a real signal.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionResolution.scala:
##########
@@ -83,23 +87,41 @@ class FunctionResolution(
/**
* Produces the ordered list of candidate names for resolution. Expansion
happens in two cases:
*
- * 1. Single-part names: expanded via the search path, where each search
path entry is
- * fully qualified so appending the name produces fully qualified
candidates.
+ * 1. Single-part names: expanded via [[SQLConf.sqlResolutionPathEntries]]
(same list as
Review Comment:
`sqlResolutionPathEntries` is on `CatalogManager`.
```suggestion
* 1. Single-part names: expanded via
[[CatalogManager.sqlResolutionPathEntries]] (same list
```
--
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]
