cloud-fan commented on code in PR #58606:
URL: https://github.com/apache/spark/pull/58606#discussion_r3999372805
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveIdentifierClause.scala:
##########
@@ -54,13 +56,36 @@ class ResolveIdentifierClause(earlyBatches:
Seq[RuleExecutor[LogicalPlan]#Batch]
}
createView.copy(child = analyzedChild, query = analyzedQuery)
}
+ // Same as [[CreateView]]: only the query body's IDENTIFIER-clause
variables are dependencies
+ // of the view definition, so resolve the ALTER target without
recording. Recording a variable
+ // used only to compute the target would, for a persisted view, be
rejected by
+ // `verifyTemporaryObjectsNotExists`, and for a temporary view (which
skips that validator) be
+ // persisted as a spurious dependency that breaks later reads.
Review Comment:
**Nit (P3):** The extra target-only entry would make the temporary view
metadata inaccurate, but it does not itself break later reads because
allow-list entries are not proactively resolved. The nearby comments also
describe CACHE TABLE AS SELECT as unconditionally text-backed even when the
analyzed-plan setting applies, and list only temporary metadata writers as
consumers although persistent CREATE and ALTER use the set for validation.
Could these comments distinguish generic capture, configuration-dependent
metadata persistence, and persistent-view validation?
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2MetadataViewSuite.scala:
##########
@@ -287,6 +287,34 @@ class DataSourceV2MetadataViewSuite extends
SharedSparkSession {
}
}
+ test("v2 CREATE / ALTER VIEW rejects a temporary variable read via an
IDENTIFIER clause") {
Review Comment:
**Non-blocking (P2):** This test covers only the default rejection path,
although `CheckViewReferences` has separate legacy-enabled branches for both v2
`CREATE VIEW` and `ALTER VIEW`. An implementation that keeps rejecting when
`spark.sql.legacy.allowSessionVariableInPersistedView` is true would still
pass, and the v1 SQL golden case does not exercise this v2 catalog path. Please
add flag-enabled success cases for both commands alongside these assertions.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala:
##########
@@ -105,7 +106,10 @@ case class CreateViewCommand(
copy(
isAnalyzed = true,
// Collect the referred temporary functions from AnalysisContext
- referredTempFunctions = analysisContext.referredTempFunctionNames.toSeq)
+ referredTempFunctions = analysisContext.referredTempFunctionNames.toSeq,
+ // The analyzed plan no longer mentions the variables read by IDENTIFIER
clauses.
+ referredTempVariablesUnderIdentifier =
Review Comment:
**Non-blocking (P2):** This captured set is not passed to
`verifyTemporaryObjectsNotExists` below. When an `IDENTIFIER` appears only
inside a scalar subquery, recursive subquery analysis replaces the placeholder
and leaves the dependency only in this accumulator; the outer command-specific
scan cannot rediscover it. With the legacy flag disabled, persistent v1 `CREATE
VIEW` therefore accepts a session-variable dependency that should raise
`INVALID_TEMP_OBJ_REFERENCE`, then stores view text that cannot be resolved
later. Please forward this set to the validator under the existing legacy gate
and add a persistent scalar-subquery regression.
--
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]
