cloud-fan commented on code in PR #58606:
URL: https://github.com/apache/spark/pull/58606#discussion_r3995050250
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveIdentifierClause.scala:
##########
@@ -69,14 +79,26 @@ class ResolveIdentifierClause(earlyBatches:
Seq[RuleExecutor[LogicalPlan]#Batch]
referredTempVars.get ++= collectTemporaryVariablesInLogicalPlan(p)
}
- executor.execute(p.planBuilder.apply(
+ val resolvedPlan = executor.execute(p.planBuilder.apply(
IdentifierResolution.evalIdentifierExpr(p.identifierExpr),
p.children))
+ // Only record the variables when the identifier names something
inside a view body. When it
Review Comment:
**Nit (P3):** This explanation is narrower than the implementation: the
general `apply0` path records any non-`Command` IDENTIFIER result, including a
standalone SELECT, not only a view body. Related new comments also say an ALTER
target would always be rejected, although a temporary ALTER skips that
validator and would instead persist the wrong dependency, and `AnalysisContext`
says the set is exported to view metadata without the documented legacy
persisted-view exception. Could these comments distinguish generic recording
from temporary-view consumption, default persisted-view rejection, and
temporary-target metadata ownership?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -270,6 +281,8 @@ object AnalysisContext {
resolutionPathEntries = function.functionStoredResolutionPath
.map(CatalogManager.deserializePathEntriesOrFail(
_, "SQL function", function.name.unquotedString)),
+ // See the same reset in `withAnalysisContext(viewDesc)`.
+ referredTempVariableNamesUnderIdentifier = mutable.LinkedHashSet.empty,
Review Comment:
**Non-blocking (P2):** `withAnalysisContext(function)` replaces the caller's
`referredTempVariableNamesUnderIdentifier` set. If a temporary view selects
from a temporary SQL function whose body reads a relation through
`IDENTIFIER(variable)`, the inner analysis records the variable here, then
restoring `originContext` discards it. View creation succeeds, but the stored
view has no referred-variable entry, so every later read fails when
view-context resolution rejects the missing name. Please preserve or merge the
caller's accumulator across function expansion and add the composed
function-in-view regression; this does not require persisting the variable in
`SQLFunction` metadata itself.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala:
##########
@@ -84,7 +84,8 @@ case class CreateViewCommand(
viewType: ViewType,
viewSchemaMode: ViewSchemaMode = SchemaUnsupported,
isAnalyzed: Boolean = false,
- referredTempFunctions: Seq[String] = Seq.empty)
+ referredTempFunctions: Seq[String] = Seq.empty,
+ referredTempVariablesUnderIdentifier: Seq[Seq[String]] = Seq.empty)
Review Comment:
**Non-blocking (P2):** This new payload is not propagated by
`CacheTableAsSelect`: its `markAsAnalyzed` snapshots only temporary functions,
and `CacheTableAsSelectExec` constructs an already-analyzed `CreateViewCommand`
without this argument. For `CACHE TABLE cv AS SELECT IDENTIFIER(v) ...`,
analysis records `v` and then drops it, so resolving the text-backed temp view
for eager caching (or a later lazy read) cannot resolve the variable. Please
thread the dependency through the cache command/strategy/exec path, while
excluding a variable used only to compute the cache view name.
**Recommended change:** Give CacheTableAsSelect and CacheTableAsSelectExec
an explicit referredTempVariablesUnderIdentifier payload, capture only
variables from the SELECT plan, forward it through DataSourceV2Strategy, and
pass it to CreateViewCommand. Add eager and/or lazy cache coverage that
resolves the created view while ensuring an IDENTIFIER used only for the cache
target name is excluded.
**Why this works:** Handle CacheTableAsSelect like CREATE/ALTER VIEW in
ResolveIdentifierClause: analyze the target expression without recording a view
dependency and analyze the SELECT body with recording enabled. Snapshot the
resulting set in markAsAnalyzed and thread the immutable sequence through
planning into the already-analyzed CreateViewCommand.
**Scope:**
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis,
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical,
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2,
sql/core/src/test
**Compatibility:** Existing CACHE TABLE AS SELECT naming, eager/lazy
semantics, temporary-function propagation, storage-level options, and failure
cleanup remain unchanged.
**Risks:** Recording the variable used to compute the cache view name would
pollute its dependency metadata. Changing case-class arity requires updating
all pattern matches and constructors.
**Constraints:** Preserve current eager versus lazy caching behavior and
cleanup on cache failure. Do not treat a target-name IDENTIFIER variable as
part of the cached view definition.
**Success:** CACHE TABLE AS SELECT with an IDENTIFIER-only query dependency
creates a resolvable text-backed temporary view. Both eager caching and a later
lazy read can resolve the stored variable dependency. A variable used only to
compute the cache view name is not stored as a referred variable.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala:
##########
@@ -283,7 +304,8 @@ case class AlterViewAsCommand(
Some(originalText),
analyzedPlan,
aliasedPlan = analyzedPlan,
- referredTempFunctions)
+ referredTempFunctions,
+ referredTempVariablesUnderIdentifier =
referredTempVariablesUnderIdentifier)
Review Comment:
**Non-blocking (P2):** Please add an end-to-end case that creates a local
temporary view, alters its body to read a relation or expression name through
an `IDENTIFIER` session variable, and then selects from the altered view. The
current permanent-ALTER and target-name cases do not consume this forwarding
argument; removing it leaves those tests green while stored-text re-analysis of
the temporary view fails. The SQL golden outputs should be regenerated through
`SQLQueryTestSuite`.
--
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]
