spark-test commented on code in PR #59959:
URL: https://github.com/apache/spark/pull/59959#discussion_r4426480069
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ViewCommandHelper.scala:
##########
@@ -61,6 +61,34 @@ object ViewCommandHelper {
dropView
}
+ /**
+ * Lowers a session-catalog [[CreateView]] to [[CreateViewCommand]], leaving
other targets intact.
Review Comment:
**Nit (P3):** `CreateViewInSessionCatalog` is a
`ResolvedIdentifierInSessionCatalog("CREATE", "VIEW")`. Its extractor throws
for a session-catalog target whose namespace does not have exactly one part:
`REQUIRES_SINGLE_PART_NAMESPACE`, or the builtin-namespace error when the last
namespace part is `builtin`. For a target such as `spark_catalog.a.b.v`, this
helper therefore neither lowers the command nor leaves it intact; it fails. The
behavior and the errors match the old inline case in `ResolveSessionCatalog`.
However, the new doc describes a function with only two outcomes, while the
sibling `rewrite(DropView)` doc in this object states the same failure ("Fails
if a session-catalog target's namespace does not have exactly one part."). The
helper is extracted so other callers can reuse it, so adding the same sentence
here would keep its contract complete and consistent with the DROP VIEW helper.
**Verification:**
- **Inspection:** The documented outcomes of the CREATE VIEW lowering helper
match what its session-catalog extractor does, including the failure for a
namespace that is not exactly one part, and are consistent with the documented
failure of the DROP VIEW helper.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisSuite.scala:
##########
@@ -1922,6 +1922,34 @@ class AnalysisSuite extends AnalysisTest with Matchers {
}
}
+ test("collect temporary variables from expression roots, subqueries and plan
children") {
+ def variable(name: String): VariableReference = {
+ val identifier = Identifier.of(Array("session"), name)
+ VariableReference(
+ originalNameParts = Seq("session", name),
+ catalog = FakeSystemCatalog,
+ identifier = identifier,
+ varDef = VariableDefinition(identifier, "1", Literal(1)))
+ }
+
+ val first = variable("first")
+ val second = variable("second")
+ val subqueryPlan = Project(Seq(Alias(Add(second, first), "value")()),
OneRowRelation())
+ val subquery = ScalarSubquery(subqueryPlan)
+ val expression = Add(first, subquery)
+ val plan = Project(Seq(Alias(expression, "value")()), subqueryPlan)
Review Comment:
**Nit (P3):** `subqueryPlan` is both this `Project`'s child and the plan
inside the root expression's `ScalarSubquery`, so every variable the child
operator references is already reachable from the root `Project`'s own
expressions. If `collectTemporaryVariablesInLogicalPlan` regressed to
inspecting only the root operator (for example
`plan.expressions.flatMap(collectTemporaryVariablesInExpressionTree).distinct`),
it would still return `expected` here, and the assertion on line 1948 would
still pass. The plan-children traversal named in the test title and in the PR
description is therefore not guarded. A regression there would drop variables
referenced only by child plans from the dependencies `ResolveIdentifierClause`
records, and it is not established that any existing SQL test would catch that.
Collector runtime behavior is unchanged; this is about what the test can detect.
Giving the child operator a variable that the root expressions cannot reach
makes the assertion discriminating, for example:
```scala
val third = variable("third")
val childPlan = Project(Seq(Alias(third, "c")()), OneRowRelation())
val plan = Project(Seq(Alias(expression, "value")()), childPlan)
assert(IdentifierResolution.collectTemporaryVariablesInLogicalPlan(plan) ==
Seq(first.originalNameParts, second.originalNameParts,
third.originalNameParts))
```
**Verification:**
- **Regression:** A plan whose child operator alone references a variable
yields that variable from the plan collector, so an implementation that
inspects only the root operator's expressions fails the test.
- **Behavior:** The existing expectations for both collectors still hold:
the root expression is included, subquery plans are searched, names are
distinct in first-occurrence order, and empty inputs return nothing.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/IdentifierResolution.scala:
##########
@@ -48,4 +49,20 @@ object IdentifierResolution extends AliasHelper with
EvalHelper {
}
}
}
+
+ /** Collects distinct session variable names from a plan, including its
subqueries. */
Review Comment:
**Nit (P3):** This doc, and the one on
`collectTemporaryVariablesInExpressionTree` at line 60, says the collectors
return session variable names. However, the matcher on line 64 returns
`originalNameParts` for every `VariableReference` without checking its catalog.
Inside a SQL script, `VariableResolution.lookupVariable` resolves a DECLAREd
local variable to a `VariableReference` backed by `FakeLocalCatalog` before it
falls back to session variables. A script-local name, for example the argument
of `IDENTIFIER(local_var)`, is therefore returned just like a session one.
Runtime behavior is unchanged, but these helpers are being made public so other
command-resolution and IDENTIFIER-preprocessing code can reuse them. A caller
that trusts this doc could treat every result as a session variable, for
example by qualifying it as `system.session`. Both docs would match the code if
they described the result as the distinct, originally written name parts of all
variable references, whether
session or SQL-script local.
**Verification:**
- **Inspection:** The documented result scope of both variable collectors
matches their matching logic, which accepts every variable reference regardless
of whether it is a session or a SQL-script local variable, and no runtime
behavior changes.
--
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]
