You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "stevomitric (via GitHub)" <gi...@apache.org> on 2026/09/08 07:45:06 UTC
[PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
stevomitric opened a new pull request, #58606:
URL: https://github.com/apache/spark/pull/58606
### What changes were proposed in this pull request?
A temporary variable read only inside a view's `IDENTIFIER` clause is now recorded as a variable the view refers to, so it stays resolvable when the stored view text is analyzed again.
`ResolveIdentifierClause` replaces the placeholder with the plan built from the *evaluated* name, which drops the identifier expression. The `VariableReference` is therefore absent from the analyzed plan, and `ViewHelper.collectTemporaryVariables`
- the only source for the view's `view.referredTempVariablesNames` property - cannot see it. On re-analysis `ColumnResolutionHelper.resolveVariables` restricts variable lookup to that property whenever `AnalysisContext.catalogAndNamespace` is non-empty, which is always true inside a view body, so the variable resolves to nothing and the identifier expression is reported as not constant.
The names are now recorded while they are still visible, in a new `AnalysisContext.referredTempVariableNamesUnderIdentifier` accumulator, and persisted by `CreateViewCommand` and `AlterViewAsCommand` via `markAsAnalyzed`. The accumulator is reset when entering a nested view or SQL function body so an inner object's variables are not attributed to the object being created.
This also fixes `collectTemporaryVariablesInExpressionTree`, which had an extra `children` hop and so skipped the expression's own root node.
### Why are the changes needed?
The view is unusable after creation:
```sql
CREATE OR REPLACE TEMPORARY VIEW t AS SELECT 1 AS c1;
DECLARE OR REPLACE VARIABLE relation_name STRING DEFAULT 't';
CREATE OR REPLACE TEMPORARY VIEW v AS SELECT * FROM IDENTIFIER(relation_name);
SELECT * FROM v;
```
fails with
```
[NOT_A_CONSTANT_STRING.NOT_CONSTANT] The expression relation_name used for the
routine or clause IDENTIFIER must be a constant STRING which is NOT NULL.
```
`CREATE VIEW` succeeds, so the failure only shows up on every later reference.
### Does this PR introduce _any_ user-facing change?
Yes. The query above now returns the rows of `t` instead of failing. A view's stored properties may now list a referred temporary variable that was previously omitted.
### How was this patch tested?
New case in `identifier-clause.sql`. It fails on master with `NOT_A_CONSTANT_STRING.NOT_CONSTANT` and passes with this change.
### Was this patch authored or co-authored using generative AI tooling?
Co-authored-by: Claude Opus 4.8
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #58606:
URL: https://github.com/apache/spark/pull/58606#discussion_r4028290371
##########
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:
Confirmed in the current head: v1 CREATE now forwards the captured IDENTIFIER dependencies to persisted-view validation, and the scalar-subquery rejection is covered.
<!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"inline:3999372799","thread_id":"inline:3999372799","verdict_sha256":"f8b9e9e8019e17927c80e73ee028149c331391e2c415bc53aa6ac479a6ce91bb"} -->
##########
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:
Confirmed: the current v2 suite now covers both CREATE and ALTER with the legacy flag enabled, alongside the default rejection path.
<!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"inline:3999372801","thread_id":"inline:3999372801","verdict_sha256":"f8b9e9e8019e17927c80e73ee028149c331391e2c415bc53aa6ac479a6ce91bb"} -->
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala:
##########
@@ -629,7 +663,8 @@ object ViewHelper extends SQLConfHelper with Logging with CapturesConfig {
isTemporary: Boolean,
viewNameParts: Seq[String],
child: LogicalPlan,
- referredTempFunctions: Seq[String]): Unit = {
+ referredTempFunctions: Seq[String],
+ referredTempVariablesUnderIdentifier: Seq[Seq[String]] = Seq.empty): Unit = {
Review Comment:
The current head now snapshots metric-view IDENTIFIER dependencies inside the analysis context and validates them under the legacy gate, with default-rejection and legacy-success coverage. I found a separate session-configuration scoping issue in the new helper and will leave that as a new review item.
<!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"inline:4000506575","thread_id":"inline:4000506575","verdict_sha256":"f8b9e9e8019e17927c80e73ee028149c331391e2c415bc53aa6ac479a6ce91bb"} -->
##########
sql/core/src/test/resources/sql-tests/results/identifier-clause.sql.out:
##########
@@ -3197,3 +3197,826 @@ DROP SCHEMA identifier_clause_test_schema
struct<>
-- !query output
+
+
+-- !query
+CREATE OR REPLACE TEMPORARY VIEW identifier_var_target AS SELECT 1 AS c1
+-- !query schema
+struct<>
+-- !query output
+
+
+
+-- !query
+DECLARE OR REPLACE VARIABLE identifier_var_name STRING DEFAULT 'identifier_var_target'
+-- !query schema
+struct<>
+-- !query output
+
+
+
+-- !query
+CREATE OR REPLACE TEMPORARY VIEW identifier_var_view AS
+SELECT * FROM IDENTIFIER(identifier_var_name)
+-- !query schema
+struct<>
+-- !query output
+
+
+
+-- !query
+SELECT count(*) AS row_count FROM identifier_var_view
+-- !query schema
+struct<row_count:bigint>
+-- !query output
+1
+
+
+-- !query
+DROP VIEW identifier_var_view
+-- !query schema
+struct<>
+-- !query output
+
+
+
+-- !query
+DROP VIEW identifier_var_target
+-- !query schema
+struct<>
+-- !query output
+
+
+
+-- !query
+DROP TEMPORARY VARIABLE identifier_var_name
+-- !query schema
+struct<>
+-- !query output
+
+
+
+-- !query
+CREATE TEMPORARY VIEW identifier_perm_target AS SELECT 1 AS c1
+-- !query schema
+struct<>
+-- !query output
+
+
+
+-- !query
+DECLARE OR REPLACE VARIABLE identifier_perm_name STRING DEFAULT 'identifier_perm_target'
+-- !query schema
+struct<>
+-- !query output
+
+
+
+-- !query
+CREATE VIEW identifier_perm_view AS SELECT * FROM IDENTIFIER(identifier_perm_name)
+-- !query schema
+struct<>
+-- !query output
+org.apache.spark.sql.AnalysisException
+{
+ "errorClass" : "INVALID_TEMP_OBJ_REFERENCE",
+ "sqlState" : "42K0F",
+ "messageParameters" : {
+ "obj" : "VIEW",
+ "objName" : "`unknown`",
Review Comment:
Confirmed in the current head: the early rejection now derives the resolved CREATE VIEW target, so the diagnostic reports the actual multipart name.
<!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"inline:4000506578","thread_id":"inline:4000506578","verdict_sha256":"f8b9e9e8019e17927c80e73ee028149c331391e2c415bc53aa6ac479a6ce91bb"} -->
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -499,6 +523,48 @@ class Analyzer(
private def executeSameContext(plan: LogicalPlan): LogicalPlan =
runWithSessionConf(super.execute(plan))
+ /**
+ * Like [[executeAndCheck]], but also returns the temporary variables recorded via IDENTIFIER
+ * clauses during this analysis (`AnalysisContext.referredTempVariableNamesUnderIdentifier`).
+ *
+ * Those variables are absent from the analyzed plan (the placeholder is replaced by the plan
+ * built from the evaluated name), and the accumulator that holds them is discarded when the
+ * analysis scope exits. A caller that separately validates a freshly analyzed body against
+ * persisted-view rules (metric-view creation) therefore cannot recover them afterwards, so this
+ * entry point reads them inside the owning scope and freezes them into the returned result.
+ *
+ * It runs the fixed-point analyzer directly (in a context it owns, so the accumulator stays
+ * readable). The only caller analyzes a metric-view placeholder, which is explicitly unsupported
+ * by the single-pass resolver (see the resolver's unsupported-feature list), so
+ * [[executeAndCheck]] would fall back to this same fixed-point path anyway.
+ */
+ def executeAndCheckReferredTempVariablesUnderIdentifier(
+ plan: LogicalPlan,
+ tracker: QueryPlanningTracker): (LogicalPlan, Seq[Seq[String]]) = {
+ if (plan.analyzed) {
+ (plan, Seq.empty)
+ } else {
+ def analyze(): (LogicalPlan, Seq[Seq[String]]) = AnalysisHelper.markInAnalyzer {
+ val analyzed = QueryPlanningTracker.withTracker(tracker) {
+ executeSameContext(plan)
Review Comment:
The current head now preserves the forced-single-pass dispatch through `executeAndCheck` and adds a regression case. I found a separate cross-session configuration-scope issue in the helper and will report it independently.
<!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"inline:4015151240","thread_id":"inline:4015151240","verdict_sha256":"f8b9e9e8019e17927c80e73ee028149c331391e2c415bc53aa6ac479a6ce91bb"} -->
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #58606:
URL: https://github.com/apache/spark/pull/58606#discussion_r3999377364
##########
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:
Addressed in the current head: the SQL suite now alters a local temporary view with an IDENTIFIER dependency and reads the altered view back.
<!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"inline:3995050252","thread_id":"inline:3995050252","verdict_sha256":"12b7f981564f986cf8d4f4a9b51e54d32eca69438988a6670d47d8c183663030"} -->
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "stevomitric (via GitHub)" <gi...@apache.org>.
stevomitric commented on code in PR #58606:
URL: https://github.com/apache/spark/pull/58606#discussion_r3987959061
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveIdentifierClause.scala:
##########
@@ -95,12 +117,27 @@ class ResolveIdentifierClause(earlyBatches: Seq[RuleExecutor[LogicalPlan]#Batch]
if (referredTempVars.isDefined) {
referredTempVars.get ++= collectTemporaryVariablesInExpressionTree(e)
}
+ if (recordUnderIdentifier) {
+ recordTemporaryVariablesUnderIdentifier(e.identifierExpr)
+ }
e.exprBuilder.apply(
IdentifierResolution.evalIdentifierExpr(e.identifierExpr), e.otherExprs)
}
}
+ /**
+ * Records the temporary variables read by an identifier expression in the [[AnalysisContext]].
+ * Evaluating the identifier expression is the last time these references are visible: the
+ * placeholder is replaced by the plan or expression built from the evaluated name, which no
+ * longer mentions them. Temporary view creation persists the recorded names so that the
+ * variables are still resolvable when the stored view text is analyzed again.
+ */
+ private def recordTemporaryVariablesUnderIdentifier(identifierExpr: Expression): Unit = {
Review Comment:
Reworded to views only.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #58606:
URL: https://github.com/apache/spark/pull/58606#discussion_r4000507758
##########
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:
Thanks for rewording this. A few narrower overstatements remain: the legacy-enabled persisted paths discard rather than validate or persist the captured set, and ordinary CREATE can reject in ResolveIdentifierClause before verifyTemporaryObjectsNotExists. Could you qualify those comments as well?
<!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"inline:3999372805","thread_id":"inline:3999372805","verdict_sha256":"621d081f741b0dcc4900b398892280681caa77ccb57db06fb0b7d2a6dc80e430"} -->
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #58606:
URL: https://github.com/apache/spark/pull/58606#issuecomment-5896766185
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/ef61d20c10f52a882a60fc646a71ae211017bffa
- merged into branch-4.x https://github.com/apache/spark/commit/b4bb4fa6989389c1df0ad9bee7247a4909945392
*Posted by `merge_spark_pr.py`*
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "stevomitric (via GitHub)" <gi...@apache.org>.
stevomitric commented on code in PR #58606:
URL: https://github.com/apache/spark/pull/58606#discussion_r4000133228
##########
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:
Fixed. Forwarding IDENTIFIER variables to verifyTemporaryObjectsNotExists.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "stevomitric (via GitHub)" <gi...@apache.org>.
stevomitric commented on PR #58606:
URL: https://github.com/apache/spark/pull/58606#issuecomment-5605862789
cc @uros-b could you PTAL when you get a chance?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b closed pull request #58606: [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view
URL: https://github.com/apache/spark/pull/58606
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #58606:
URL: https://github.com/apache/spark/pull/58606#discussion_r3999376498
##########
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:
The current head now distinguishes generic recording, target-name exclusion, temporary-view metadata, and persisted-view validation. I called out the remaining narrower wording overstatements separately.
<!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"inline:3995050250","thread_id":"inline:3995050250","verdict_sha256":"12b7f981564f986cf8d4f4a9b51e54d32eca69438988a6670d47d8c183663030"} -->
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #58606:
URL: https://github.com/apache/spark/pull/58606#issuecomment-5655284837
I reviewed the current head and did not find any new blocking issue.
<!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"conversation:5632892769","thread_id":"conversation:5632892769","verdict_sha256":"621d081f741b0dcc4900b398892280681caa77ccb57db06fb0b7d2a6dc80e430"} -->
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "anshulbaliga7 (via GitHub)" <gi...@apache.org>.
anshulbaliga7 commented on PR #58606:
URL: https://github.com/apache/spark/pull/58606#issuecomment-5967626635
Filed SPARK-59970 and opened [#59226](https://github.com/anshulbaliga7/spark/issues/59226) for the pre-existing SQL-function IDENTIFIER gap @cloud-fan noted in review here. It reuses the accumulator this PR introduced, so no changes to the view paths.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on code in PR #58606:
URL: https://github.com/apache/spark/pull/58606#discussion_r3978186145
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveIdentifierClause.scala:
##########
@@ -95,12 +117,27 @@ class ResolveIdentifierClause(earlyBatches: Seq[RuleExecutor[LogicalPlan]#Batch]
if (referredTempVars.isDefined) {
referredTempVars.get ++= collectTemporaryVariablesInExpressionTree(e)
}
+ if (recordUnderIdentifier) {
+ recordTemporaryVariablesUnderIdentifier(e.identifierExpr)
+ }
e.exprBuilder.apply(
IdentifierResolution.evalIdentifierExpr(e.identifierExpr), e.otherExprs)
}
}
+ /**
+ * Records the temporary variables read by an identifier expression in the [[AnalysisContext]].
+ * Evaluating the identifier expression is the last time these references are visible: the
+ * placeholder is replaced by the plan or expression built from the evaluated name, which no
+ * longer mentions them. Temporary view creation persists the recorded names so that the
+ * variables are still resolvable when the stored view text is analyzed again.
+ */
+ private def recordTemporaryVariablesUnderIdentifier(identifierExpr: Expression): Unit = {
Review Comment:
The comment states "Temporary view and SQL function creation persist the recorded names," but only view creation consumes referredTempVariableNamesUnderIdentifier: it is read solely by CreateViewCommand.markAsAnalyzed and AlterViewAsCommand.markAsAnalyzed.
No SQL-function command reads it (the symbol is absent from master and the diff touches no function file; CreateSQLFunctionCommand is unchanged). The withAnalysisContext(function) reset only prevents a function body's identifier vars from being mis-attributed to the enclosing view, it does not persist them for the function.
Reword the comment to views only, or, if temporary SQL functions share the same latent bug, split that off as a follow-up.
##########
sql/core/src/test/resources/sql-tests/inputs/identifier-clause.sql:
##########
@@ -468,3 +468,91 @@ SELECT EXTRACT(IDENTIFIER('YEAR') FROM DATE'2024-01-15');
SELECT TIMESTAMPADD(IDENTIFIER('YEAR'), 1, DATE'2024-01-15');
DROP SCHEMA identifier_clause_test_schema;
+
Review Comment:
Coverage is limited to a LocalTempView table reference (positive) and a permanent-view rejection (negative). The subtlest new logic, the accumulator reset on entering a nested view / SQL-function body, which guards against attributing an inner object's variables to the object being created - has no test.
Also untested: ALTER VIEW AS ... IDENTIFIER(var), GLOBAL TEMPORARY VIEW, and the expression-position ExpressionWithUnresolvedIdentifier case. The ALTER/global paths reuse the same createTemporaryViewRelation call already exercised, but a nested-view guard test would lock in the reset behavior.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "miland-db (via GitHub)" <gi...@apache.org>.
miland-db commented on code in PR #58606:
URL: https://github.com/apache/spark/pull/58606#discussion_r3956145126
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveIdentifierClause.scala:
##########
@@ -110,16 +125,13 @@ class ResolveIdentifierClause(earlyBatches: Seq[RuleExecutor[LogicalPlan]#Batch]
collectTempVars(child)
}
+ // Visits `child` itself as well as its descendants, so that an identifier expression which is
+ // just a variable reference is collected too.
private def collectTemporaryVariablesInExpressionTree(child: Expression): Seq[Seq[String]] = {
- def collectTempVars(child: Expression): Seq[Seq[String]] = {
- child.flatMap { expr =>
- expr.children.flatMap(_.flatMap {
- case e: SubqueryExpression => collectTemporaryVariablesInLogicalPlan(e.plan)
- case r: VariableReference => Seq(r.originalNameParts)
- case _ => Seq.empty
- })
- }.distinct
- }
- collectTempVars(child)
+ child.flatMap {
+ case e: SubqueryExpression => collectTemporaryVariablesInLogicalPlan(e.plan)
+ case r: VariableReference => Seq(r.originalNameParts)
+ case _ => Seq.empty
+ }.distinct
Review Comment:
Is this change needed for the fix?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #58606:
URL: https://github.com/apache/spark/pull/58606#discussion_r3999374136
##########
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:
Addressed in the current head: SQL-function analysis now shares the enclosing accumulator, and the composed function-in-view regression is covered.
<!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"inline:3995050244","thread_id":"inline:3995050244","verdict_sha256":"12b7f981564f986cf8d4f4a9b51e54d32eca69438988a6670d47d8c183663030"} -->
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "stevomitric (via GitHub)" <gi...@apache.org>.
stevomitric commented on code in PR #58606:
URL: https://github.com/apache/spark/pull/58606#discussion_r4000134367
##########
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:
added.
##########
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:
reworded the comment.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #58606:
URL: https://github.com/apache/spark/pull/58606#discussion_r4028292060
##########
sql/core/src/test/resources/sql-tests/inputs/identifier-clause.sql:
##########
@@ -468,3 +468,169 @@ SELECT EXTRACT(IDENTIFIER('YEAR') FROM DATE'2024-01-15');
SELECT TIMESTAMPADD(IDENTIFIER('YEAR'), 1, DATE'2024-01-15');
DROP SCHEMA identifier_clause_test_schema;
+
+-- A variable read only by a temporary view's IDENTIFIER clause is still a variable the view refers
+-- to, so it stays resolvable when the stored view text is analyzed again.
+CREATE OR REPLACE TEMPORARY VIEW identifier_var_target AS SELECT 1 AS c1;
+DECLARE OR REPLACE VARIABLE identifier_var_name STRING DEFAULT 'identifier_var_target';
+CREATE OR REPLACE TEMPORARY VIEW identifier_var_view AS
+SELECT * FROM IDENTIFIER(identifier_var_name);
+SELECT count(*) AS row_count FROM identifier_var_view;
+DROP VIEW identifier_var_view;
+DROP VIEW identifier_var_target;
+DROP TEMPORARY VARIABLE identifier_var_name;
+
+-- A permanent view must still reject a temporary variable read via an IDENTIFIER clause.
+CREATE TEMPORARY VIEW identifier_perm_target AS SELECT 1 AS c1;
+DECLARE OR REPLACE VARIABLE identifier_perm_name STRING DEFAULT 'identifier_perm_target';
+CREATE VIEW identifier_perm_view AS SELECT * FROM IDENTIFIER(identifier_perm_name);
+DROP VIEW identifier_perm_target;
+DROP TEMPORARY VARIABLE identifier_perm_name;
+
+-- A permanent ALTER VIEW must also reject a temporary variable read via an IDENTIFIER clause.
+-- The IDENTIFIER target is a permanent table so the variable is the only temporary object.
+CREATE SCHEMA identifier_alter_schema;
+USE identifier_alter_schema;
+CREATE TABLE identifier_alter_target (c1 INT) USING parquet;
+DECLARE OR REPLACE VARIABLE identifier_alter_name STRING DEFAULT 'identifier_alter_target';
+CREATE VIEW identifier_alter_view AS SELECT 1 AS c1;
+ALTER VIEW identifier_alter_view AS SELECT * FROM IDENTIFIER(identifier_alter_name);
+SELECT * FROM identifier_alter_view;
+DROP VIEW identifier_alter_view;
+DROP TABLE identifier_alter_target;
+DROP TEMPORARY VARIABLE identifier_alter_name;
+USE default;
+DROP SCHEMA identifier_alter_schema;
+
+-- ALTER VIEW whose TARGET is picked by a variable, with a body that has no temp dependency, must
+-- still succeed: the variable is not a dependency of the view definition.
+CREATE SCHEMA ivt_schema;
+USE ivt_schema;
+CREATE VIEW ivt_target AS SELECT 1 AS c1;
+DECLARE OR REPLACE VARIABLE ivt_name STRING DEFAULT 'ivt_target';
+ALTER VIEW IDENTIFIER(ivt_name) AS SELECT 2 AS c1;
+SELECT * FROM ivt_target;
+DROP VIEW ivt_target;
+DROP TEMPORARY VARIABLE ivt_name;
+USE default;
+DROP SCHEMA ivt_schema;
+
+-- A temporary view whose body reads a variable via an IDENTIFIER clause in expression position
+-- (here as a column name). Unlike the table-position case above, this identifier expression
+-- resolves to a bare variable reference, so recording it requires visiting the root of the
+-- expression tree. If it is not recorded, the variable is missing from the stored view text and
+-- reading the view back fails.
+DECLARE OR REPLACE VARIABLE identifier_expr_col STRING DEFAULT 'c1';
+CREATE OR REPLACE TEMPORARY VIEW identifier_expr_view AS
+SELECT IDENTIFIER(identifier_expr_col) AS x FROM VALUES(1) AS t(c1);
+SELECT * FROM identifier_expr_view;
+DROP VIEW identifier_expr_view;
+DROP TEMPORARY VARIABLE identifier_expr_col;
+
+-- A GLOBAL TEMPORARY VIEW reads a variable via an IDENTIFIER clause the same way a session-local
+-- temporary view does, so the variable stays resolvable when the stored view text is analyzed again.
+DECLARE OR REPLACE VARIABLE identifier_gtv_col STRING DEFAULT 'c1';
+CREATE OR REPLACE GLOBAL TEMPORARY VIEW identifier_gtv_view AS
+SELECT IDENTIFIER(identifier_gtv_col) AS x FROM VALUES(1) AS t(c1);
+SELECT * FROM global_temp.identifier_gtv_view;
+DROP VIEW global_temp.identifier_gtv_view;
+DROP TEMPORARY VARIABLE identifier_gtv_col;
+
+-- A variable used only to supply a view's NAME via an IDENTIFIER clause is not part of the view
+-- definition, so it must not be recorded as a referred variable of the view.
+DECLARE OR REPLACE VARIABLE identifier_view_name STRING DEFAULT 'identifier_named_view';
+CREATE OR REPLACE TEMPORARY VIEW IDENTIFIER(identifier_view_name) AS SELECT 1 AS c1;
+SELECT * FROM identifier_named_view;
+DROP VIEW identifier_named_view;
+DROP TEMPORARY VARIABLE identifier_view_name;
+
+-- When both the NAME and the BODY use an IDENTIFIER clause, only the body variable is a dependency
+-- of the view; the name variable must not be recorded.
+DECLARE OR REPLACE VARIABLE identifier_name_part STRING DEFAULT 'identifier_named_view2';
+DECLARE OR REPLACE VARIABLE identifier_body_part STRING DEFAULT 'c1';
+CREATE OR REPLACE TEMPORARY VIEW IDENTIFIER(identifier_name_part) AS
+SELECT IDENTIFIER(identifier_body_part) AS x FROM VALUES(1) AS t(c1);
+SELECT * FROM identifier_named_view2;
+DROP VIEW identifier_named_view2;
+DROP TEMPORARY VARIABLE identifier_name_part;
+DROP TEMPORARY VARIABLE identifier_body_part;
+
+-- ALTER VIEW honors `spark.sql.legacy.allowSessionVariableInPersistedView` just like CREATE VIEW:
+-- with it set, the DDL is allowed instead of rejected (the legacy permissive behavior). The flag
+-- does not persist the variable into the stored view, so the view is not guaranteed to resolve in a
+-- fresh session; the default (tested by `identifier_alter_view` above) rejects it outright.
+SET spark.sql.legacy.allowSessionVariableInPersistedView=true;
+DECLARE OR REPLACE VARIABLE identifier_p2a_col STRING DEFAULT 'c1';
+CREATE VIEW identifier_p2a_view AS SELECT 1 AS c1;
+ALTER VIEW identifier_p2a_view AS SELECT IDENTIFIER(identifier_p2a_col) FROM VALUES(1) AS t(c1);
+DROP VIEW identifier_p2a_view;
+DROP TEMPORARY VARIABLE identifier_p2a_col;
+SET spark.sql.legacy.allowSessionVariableInPersistedView=false;
+
+-- Nested temporary views lock in the `withAnalysisContext` accumulator reset: creating the outer
Review Comment:
Confirmed: the current analyzer-result coverage checks that the nested dependency belongs to the inner view and is absent from the outer view.
<!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"inline:4015151247","thread_id":"inline:4015151247","verdict_sha256":"f8b9e9e8019e17927c80e73ee028149c331391e2c415bc53aa6ac479a6ce91bb"} -->
##########
sql/core/src/test/resources/sql-tests/inputs/identifier-clause.sql:
##########
@@ -468,3 +468,169 @@ SELECT EXTRACT(IDENTIFIER('YEAR') FROM DATE'2024-01-15');
SELECT TIMESTAMPADD(IDENTIFIER('YEAR'), 1, DATE'2024-01-15');
DROP SCHEMA identifier_clause_test_schema;
+
+-- A variable read only by a temporary view's IDENTIFIER clause is still a variable the view refers
+-- to, so it stays resolvable when the stored view text is analyzed again.
+CREATE OR REPLACE TEMPORARY VIEW identifier_var_target AS SELECT 1 AS c1;
+DECLARE OR REPLACE VARIABLE identifier_var_name STRING DEFAULT 'identifier_var_target';
+CREATE OR REPLACE TEMPORARY VIEW identifier_var_view AS
+SELECT * FROM IDENTIFIER(identifier_var_name);
+SELECT count(*) AS row_count FROM identifier_var_view;
+DROP VIEW identifier_var_view;
+DROP VIEW identifier_var_target;
+DROP TEMPORARY VARIABLE identifier_var_name;
+
+-- A permanent view must still reject a temporary variable read via an IDENTIFIER clause.
+CREATE TEMPORARY VIEW identifier_perm_target AS SELECT 1 AS c1;
+DECLARE OR REPLACE VARIABLE identifier_perm_name STRING DEFAULT 'identifier_perm_target';
+CREATE VIEW identifier_perm_view AS SELECT * FROM IDENTIFIER(identifier_perm_name);
+DROP VIEW identifier_perm_target;
+DROP TEMPORARY VARIABLE identifier_perm_name;
+
+-- A permanent ALTER VIEW must also reject a temporary variable read via an IDENTIFIER clause.
+-- The IDENTIFIER target is a permanent table so the variable is the only temporary object.
+CREATE SCHEMA identifier_alter_schema;
+USE identifier_alter_schema;
+CREATE TABLE identifier_alter_target (c1 INT) USING parquet;
+DECLARE OR REPLACE VARIABLE identifier_alter_name STRING DEFAULT 'identifier_alter_target';
+CREATE VIEW identifier_alter_view AS SELECT 1 AS c1;
+ALTER VIEW identifier_alter_view AS SELECT * FROM IDENTIFIER(identifier_alter_name);
+SELECT * FROM identifier_alter_view;
+DROP VIEW identifier_alter_view;
+DROP TABLE identifier_alter_target;
+DROP TEMPORARY VARIABLE identifier_alter_name;
+USE default;
+DROP SCHEMA identifier_alter_schema;
+
+-- ALTER VIEW whose TARGET is picked by a variable, with a body that has no temp dependency, must
+-- still succeed: the variable is not a dependency of the view definition.
+CREATE SCHEMA ivt_schema;
+USE ivt_schema;
+CREATE VIEW ivt_target AS SELECT 1 AS c1;
+DECLARE OR REPLACE VARIABLE ivt_name STRING DEFAULT 'ivt_target';
+ALTER VIEW IDENTIFIER(ivt_name) AS SELECT 2 AS c1;
+SELECT * FROM ivt_target;
+DROP VIEW ivt_target;
+DROP TEMPORARY VARIABLE ivt_name;
+USE default;
+DROP SCHEMA ivt_schema;
+
+-- A temporary view whose body reads a variable via an IDENTIFIER clause in expression position
+-- (here as a column name). Unlike the table-position case above, this identifier expression
+-- resolves to a bare variable reference, so recording it requires visiting the root of the
+-- expression tree. If it is not recorded, the variable is missing from the stored view text and
+-- reading the view back fails.
+DECLARE OR REPLACE VARIABLE identifier_expr_col STRING DEFAULT 'c1';
+CREATE OR REPLACE TEMPORARY VIEW identifier_expr_view AS
+SELECT IDENTIFIER(identifier_expr_col) AS x FROM VALUES(1) AS t(c1);
+SELECT * FROM identifier_expr_view;
+DROP VIEW identifier_expr_view;
+DROP TEMPORARY VARIABLE identifier_expr_col;
+
+-- A GLOBAL TEMPORARY VIEW reads a variable via an IDENTIFIER clause the same way a session-local
+-- temporary view does, so the variable stays resolvable when the stored view text is analyzed again.
+DECLARE OR REPLACE VARIABLE identifier_gtv_col STRING DEFAULT 'c1';
+CREATE OR REPLACE GLOBAL TEMPORARY VIEW identifier_gtv_view AS
+SELECT IDENTIFIER(identifier_gtv_col) AS x FROM VALUES(1) AS t(c1);
+SELECT * FROM global_temp.identifier_gtv_view;
+DROP VIEW global_temp.identifier_gtv_view;
+DROP TEMPORARY VARIABLE identifier_gtv_col;
+
+-- A variable used only to supply a view's NAME via an IDENTIFIER clause is not part of the view
Review Comment:
Confirmed: the current suite exposes the temporary-view metadata for target-only and mixed cases and adds a persisted target-only CREATE that would fail if the target variable leaked.
<!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"inline:4015151251","thread_id":"inline:4015151251","verdict_sha256":"f8b9e9e8019e17927c80e73ee028149c331391e2c415bc53aa6ac479a6ce91bb"} -->
##########
sql/core/src/test/resources/sql-tests/inputs/identifier-clause.sql:
##########
@@ -468,3 +468,169 @@ SELECT EXTRACT(IDENTIFIER('YEAR') FROM DATE'2024-01-15');
SELECT TIMESTAMPADD(IDENTIFIER('YEAR'), 1, DATE'2024-01-15');
DROP SCHEMA identifier_clause_test_schema;
+
+-- A variable read only by a temporary view's IDENTIFIER clause is still a variable the view refers
+-- to, so it stays resolvable when the stored view text is analyzed again.
+CREATE OR REPLACE TEMPORARY VIEW identifier_var_target AS SELECT 1 AS c1;
+DECLARE OR REPLACE VARIABLE identifier_var_name STRING DEFAULT 'identifier_var_target';
+CREATE OR REPLACE TEMPORARY VIEW identifier_var_view AS
+SELECT * FROM IDENTIFIER(identifier_var_name);
+SELECT count(*) AS row_count FROM identifier_var_view;
+DROP VIEW identifier_var_view;
+DROP VIEW identifier_var_target;
+DROP TEMPORARY VARIABLE identifier_var_name;
+
+-- A permanent view must still reject a temporary variable read via an IDENTIFIER clause.
+CREATE TEMPORARY VIEW identifier_perm_target AS SELECT 1 AS c1;
+DECLARE OR REPLACE VARIABLE identifier_perm_name STRING DEFAULT 'identifier_perm_target';
+CREATE VIEW identifier_perm_view AS SELECT * FROM IDENTIFIER(identifier_perm_name);
+DROP VIEW identifier_perm_target;
+DROP TEMPORARY VARIABLE identifier_perm_name;
+
+-- A permanent ALTER VIEW must also reject a temporary variable read via an IDENTIFIER clause.
+-- The IDENTIFIER target is a permanent table so the variable is the only temporary object.
+CREATE SCHEMA identifier_alter_schema;
+USE identifier_alter_schema;
+CREATE TABLE identifier_alter_target (c1 INT) USING parquet;
+DECLARE OR REPLACE VARIABLE identifier_alter_name STRING DEFAULT 'identifier_alter_target';
+CREATE VIEW identifier_alter_view AS SELECT 1 AS c1;
+ALTER VIEW identifier_alter_view AS SELECT * FROM IDENTIFIER(identifier_alter_name);
+SELECT * FROM identifier_alter_view;
+DROP VIEW identifier_alter_view;
+DROP TABLE identifier_alter_target;
+DROP TEMPORARY VARIABLE identifier_alter_name;
+USE default;
+DROP SCHEMA identifier_alter_schema;
+
+-- ALTER VIEW whose TARGET is picked by a variable, with a body that has no temp dependency, must
+-- still succeed: the variable is not a dependency of the view definition.
+CREATE SCHEMA ivt_schema;
+USE ivt_schema;
+CREATE VIEW ivt_target AS SELECT 1 AS c1;
+DECLARE OR REPLACE VARIABLE ivt_name STRING DEFAULT 'ivt_target';
+ALTER VIEW IDENTIFIER(ivt_name) AS SELECT 2 AS c1;
+SELECT * FROM ivt_target;
+DROP VIEW ivt_target;
+DROP TEMPORARY VARIABLE ivt_name;
+USE default;
+DROP SCHEMA ivt_schema;
+
+-- A temporary view whose body reads a variable via an IDENTIFIER clause in expression position
+-- (here as a column name). Unlike the table-position case above, this identifier expression
+-- resolves to a bare variable reference, so recording it requires visiting the root of the
+-- expression tree. If it is not recorded, the variable is missing from the stored view text and
Review Comment:
Resolved in the current head: the comment now says the stored text retains the IDENTIFIER reference and that the missing referred-variable metadata breaks re-analysis.
<!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"inline:4015151259","thread_id":"inline:4015151259","verdict_sha256":"f8b9e9e8019e17927c80e73ee028149c331391e2c415bc53aa6ac479a6ce91bb"} -->
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -166,6 +166,19 @@ case class AnalysisContext(
// lookup a temporary function. And export to the view metadata.
referredTempFunctionNames: mutable.Set[String] = mutable.Set.empty,
referredTempVariableNames: Seq[Seq[String]] = Seq.empty,
+ // Like `referredTempFunctionNames`, this is populated only by fixed-point analysis (by
+ // `ResolveIdentifierClause`, the sole writer). A temporary view, temporary ALTER VIEW, or
+ // CACHE TABLE AS SELECT stores the names in its metadata so they resolve when the stored view
+ // text is analyzed again. A persisted CREATE/ALTER VIEW instead rejects them -- usually in
+ // `ResolveIdentifierClause` while resolving the body, and otherwise (e.g. an IDENTIFIER nested
+ // in a scalar subquery) in `verifyTemporaryObjectsNotExists` at run time. When
Review Comment:
Confirmed: the current comment now distinguishes v1 execution-time validation from the v2 `CheckViewReferences` analysis-time path.
<!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"inline:4015151264","thread_id":"inline:4015151264","verdict_sha256":"f8b9e9e8019e17927c80e73ee028149c331391e2c415bc53aa6ac479a6ce91bb"} -->
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #58606:
URL: https://github.com/apache/spark/pull/58606#discussion_r4015151240
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -499,6 +523,48 @@ class Analyzer(
private def executeSameContext(plan: LogicalPlan): LogicalPlan =
runWithSessionConf(super.execute(plan))
+ /**
+ * Like [[executeAndCheck]], but also returns the temporary variables recorded via IDENTIFIER
+ * clauses during this analysis (`AnalysisContext.referredTempVariableNamesUnderIdentifier`).
+ *
+ * Those variables are absent from the analyzed plan (the placeholder is replaced by the plan
+ * built from the evaluated name), and the accumulator that holds them is discarded when the
+ * analysis scope exits. A caller that separately validates a freshly analyzed body against
+ * persisted-view rules (metric-view creation) therefore cannot recover them afterwards, so this
+ * entry point reads them inside the owning scope and freezes them into the returned result.
+ *
+ * It runs the fixed-point analyzer directly (in a context it owns, so the accumulator stays
+ * readable). The only caller analyzes a metric-view placeholder, which is explicitly unsupported
+ * by the single-pass resolver (see the resolver's unsupported-feature list), so
+ * [[executeAndCheck]] would fall back to this same fixed-point path anyway.
+ */
+ def executeAndCheckReferredTempVariablesUnderIdentifier(
+ plan: LogicalPlan,
+ tracker: QueryPlanningTracker): (LogicalPlan, Seq[Seq[String]]) = {
+ if (plan.analyzed) {
+ (plan, Seq.empty)
+ } else {
+ def analyze(): (LogicalPlan, Seq[Seq[String]]) = AnalysisHelper.markInAnalyzer {
+ val analyzed = QueryPlanningTracker.withTracker(tracker) {
+ executeSameContext(plan)
Review Comment:
**Nit (P3):** Could this snapshot the IDENTIFIER dependencies while retaining the existing `HybridAnalyzer` dispatch? Calling `executeSameContext` directly here silently bypasses `spark.sql.analyzer.singlePassResolver.enabled=true`. Since `MetricViewPlaceholder` is explicitly unsupported by the single-pass resolver, forced mode should surface that incompatibility instead of succeeding through fixed-point analysis. A forced-mode metric-view regression would lock down the routing contract.
##########
sql/core/src/test/resources/sql-tests/inputs/identifier-clause.sql:
##########
@@ -468,3 +468,169 @@ SELECT EXTRACT(IDENTIFIER('YEAR') FROM DATE'2024-01-15');
SELECT TIMESTAMPADD(IDENTIFIER('YEAR'), 1, DATE'2024-01-15');
DROP SCHEMA identifier_clause_test_schema;
+
+-- A variable read only by a temporary view's IDENTIFIER clause is still a variable the view refers
+-- to, so it stays resolvable when the stored view text is analyzed again.
+CREATE OR REPLACE TEMPORARY VIEW identifier_var_target AS SELECT 1 AS c1;
+DECLARE OR REPLACE VARIABLE identifier_var_name STRING DEFAULT 'identifier_var_target';
+CREATE OR REPLACE TEMPORARY VIEW identifier_var_view AS
+SELECT * FROM IDENTIFIER(identifier_var_name);
+SELECT count(*) AS row_count FROM identifier_var_view;
+DROP VIEW identifier_var_view;
+DROP VIEW identifier_var_target;
+DROP TEMPORARY VARIABLE identifier_var_name;
+
+-- A permanent view must still reject a temporary variable read via an IDENTIFIER clause.
+CREATE TEMPORARY VIEW identifier_perm_target AS SELECT 1 AS c1;
+DECLARE OR REPLACE VARIABLE identifier_perm_name STRING DEFAULT 'identifier_perm_target';
+CREATE VIEW identifier_perm_view AS SELECT * FROM IDENTIFIER(identifier_perm_name);
+DROP VIEW identifier_perm_target;
+DROP TEMPORARY VARIABLE identifier_perm_name;
+
+-- A permanent ALTER VIEW must also reject a temporary variable read via an IDENTIFIER clause.
+-- The IDENTIFIER target is a permanent table so the variable is the only temporary object.
+CREATE SCHEMA identifier_alter_schema;
+USE identifier_alter_schema;
+CREATE TABLE identifier_alter_target (c1 INT) USING parquet;
+DECLARE OR REPLACE VARIABLE identifier_alter_name STRING DEFAULT 'identifier_alter_target';
+CREATE VIEW identifier_alter_view AS SELECT 1 AS c1;
+ALTER VIEW identifier_alter_view AS SELECT * FROM IDENTIFIER(identifier_alter_name);
+SELECT * FROM identifier_alter_view;
+DROP VIEW identifier_alter_view;
+DROP TABLE identifier_alter_target;
+DROP TEMPORARY VARIABLE identifier_alter_name;
+USE default;
+DROP SCHEMA identifier_alter_schema;
+
+-- ALTER VIEW whose TARGET is picked by a variable, with a body that has no temp dependency, must
+-- still succeed: the variable is not a dependency of the view definition.
+CREATE SCHEMA ivt_schema;
+USE ivt_schema;
+CREATE VIEW ivt_target AS SELECT 1 AS c1;
+DECLARE OR REPLACE VARIABLE ivt_name STRING DEFAULT 'ivt_target';
+ALTER VIEW IDENTIFIER(ivt_name) AS SELECT 2 AS c1;
+SELECT * FROM ivt_target;
+DROP VIEW ivt_target;
+DROP TEMPORARY VARIABLE ivt_name;
+USE default;
+DROP SCHEMA ivt_schema;
+
+-- A temporary view whose body reads a variable via an IDENTIFIER clause in expression position
+-- (here as a column name). Unlike the table-position case above, this identifier expression
+-- resolves to a bare variable reference, so recording it requires visiting the root of the
+-- expression tree. If it is not recorded, the variable is missing from the stored view text and
+-- reading the view back fails.
+DECLARE OR REPLACE VARIABLE identifier_expr_col STRING DEFAULT 'c1';
+CREATE OR REPLACE TEMPORARY VIEW identifier_expr_view AS
+SELECT IDENTIFIER(identifier_expr_col) AS x FROM VALUES(1) AS t(c1);
+SELECT * FROM identifier_expr_view;
+DROP VIEW identifier_expr_view;
+DROP TEMPORARY VARIABLE identifier_expr_col;
+
+-- A GLOBAL TEMPORARY VIEW reads a variable via an IDENTIFIER clause the same way a session-local
+-- temporary view does, so the variable stays resolvable when the stored view text is analyzed again.
+DECLARE OR REPLACE VARIABLE identifier_gtv_col STRING DEFAULT 'c1';
+CREATE OR REPLACE GLOBAL TEMPORARY VIEW identifier_gtv_view AS
+SELECT IDENTIFIER(identifier_gtv_col) AS x FROM VALUES(1) AS t(c1);
+SELECT * FROM global_temp.identifier_gtv_view;
+DROP VIEW global_temp.identifier_gtv_view;
+DROP TEMPORARY VARIABLE identifier_gtv_col;
+
+-- A variable used only to supply a view's NAME via an IDENTIFIER clause is not part of the view
+-- definition, so it must not be recorded as a referred variable of the view.
+DECLARE OR REPLACE VARIABLE identifier_view_name STRING DEFAULT 'identifier_named_view';
+CREATE OR REPLACE TEMPORARY VIEW IDENTIFIER(identifier_view_name) AS SELECT 1 AS c1;
+SELECT * FROM identifier_named_view;
+DROP VIEW identifier_named_view;
+DROP TEMPORARY VARIABLE identifier_view_name;
+
+-- When both the NAME and the BODY use an IDENTIFIER clause, only the body variable is a dependency
+-- of the view; the name variable must not be recorded.
+DECLARE OR REPLACE VARIABLE identifier_name_part STRING DEFAULT 'identifier_named_view2';
+DECLARE OR REPLACE VARIABLE identifier_body_part STRING DEFAULT 'c1';
+CREATE OR REPLACE TEMPORARY VIEW IDENTIFIER(identifier_name_part) AS
+SELECT IDENTIFIER(identifier_body_part) AS x FROM VALUES(1) AS t(c1);
+SELECT * FROM identifier_named_view2;
+DROP VIEW identifier_named_view2;
+DROP TEMPORARY VARIABLE identifier_name_part;
+DROP TEMPORARY VARIABLE identifier_body_part;
+
+-- ALTER VIEW honors `spark.sql.legacy.allowSessionVariableInPersistedView` just like CREATE VIEW:
+-- with it set, the DDL is allowed instead of rejected (the legacy permissive behavior). The flag
+-- does not persist the variable into the stored view, so the view is not guaranteed to resolve in a
+-- fresh session; the default (tested by `identifier_alter_view` above) rejects it outright.
+SET spark.sql.legacy.allowSessionVariableInPersistedView=true;
+DECLARE OR REPLACE VARIABLE identifier_p2a_col STRING DEFAULT 'c1';
+CREATE VIEW identifier_p2a_view AS SELECT 1 AS c1;
+ALTER VIEW identifier_p2a_view AS SELECT IDENTIFIER(identifier_p2a_col) FROM VALUES(1) AS t(c1);
+DROP VIEW identifier_p2a_view;
+DROP TEMPORARY VARIABLE identifier_p2a_col;
+SET spark.sql.legacy.allowSessionVariableInPersistedView=false;
+
+-- Nested temporary views lock in the `withAnalysisContext` accumulator reset: creating the outer
Review Comment:
**Nit (P3):** This readback does not actually test the accumulator reset: if the reset is removed, `identifier_nested_col` leaks into the outer view's allow-list, but the SELECT still succeeds because the extra entry is not eagerly resolved and the variable remains declared. Could the test assert the referred-variable metadata instead, verifying that the inner view owns this name and the outer view does not?
##########
sql/core/src/test/resources/sql-tests/inputs/identifier-clause.sql:
##########
@@ -468,3 +468,169 @@ SELECT EXTRACT(IDENTIFIER('YEAR') FROM DATE'2024-01-15');
SELECT TIMESTAMPADD(IDENTIFIER('YEAR'), 1, DATE'2024-01-15');
DROP SCHEMA identifier_clause_test_schema;
+
+-- A variable read only by a temporary view's IDENTIFIER clause is still a variable the view refers
+-- to, so it stays resolvable when the stored view text is analyzed again.
+CREATE OR REPLACE TEMPORARY VIEW identifier_var_target AS SELECT 1 AS c1;
+DECLARE OR REPLACE VARIABLE identifier_var_name STRING DEFAULT 'identifier_var_target';
+CREATE OR REPLACE TEMPORARY VIEW identifier_var_view AS
+SELECT * FROM IDENTIFIER(identifier_var_name);
+SELECT count(*) AS row_count FROM identifier_var_view;
+DROP VIEW identifier_var_view;
+DROP VIEW identifier_var_target;
+DROP TEMPORARY VARIABLE identifier_var_name;
+
+-- A permanent view must still reject a temporary variable read via an IDENTIFIER clause.
+CREATE TEMPORARY VIEW identifier_perm_target AS SELECT 1 AS c1;
+DECLARE OR REPLACE VARIABLE identifier_perm_name STRING DEFAULT 'identifier_perm_target';
+CREATE VIEW identifier_perm_view AS SELECT * FROM IDENTIFIER(identifier_perm_name);
+DROP VIEW identifier_perm_target;
+DROP TEMPORARY VARIABLE identifier_perm_name;
+
+-- A permanent ALTER VIEW must also reject a temporary variable read via an IDENTIFIER clause.
+-- The IDENTIFIER target is a permanent table so the variable is the only temporary object.
+CREATE SCHEMA identifier_alter_schema;
+USE identifier_alter_schema;
+CREATE TABLE identifier_alter_target (c1 INT) USING parquet;
+DECLARE OR REPLACE VARIABLE identifier_alter_name STRING DEFAULT 'identifier_alter_target';
+CREATE VIEW identifier_alter_view AS SELECT 1 AS c1;
+ALTER VIEW identifier_alter_view AS SELECT * FROM IDENTIFIER(identifier_alter_name);
+SELECT * FROM identifier_alter_view;
+DROP VIEW identifier_alter_view;
+DROP TABLE identifier_alter_target;
+DROP TEMPORARY VARIABLE identifier_alter_name;
+USE default;
+DROP SCHEMA identifier_alter_schema;
+
+-- ALTER VIEW whose TARGET is picked by a variable, with a body that has no temp dependency, must
+-- still succeed: the variable is not a dependency of the view definition.
+CREATE SCHEMA ivt_schema;
+USE ivt_schema;
+CREATE VIEW ivt_target AS SELECT 1 AS c1;
+DECLARE OR REPLACE VARIABLE ivt_name STRING DEFAULT 'ivt_target';
+ALTER VIEW IDENTIFIER(ivt_name) AS SELECT 2 AS c1;
+SELECT * FROM ivt_target;
+DROP VIEW ivt_target;
+DROP TEMPORARY VARIABLE ivt_name;
+USE default;
+DROP SCHEMA ivt_schema;
+
+-- A temporary view whose body reads a variable via an IDENTIFIER clause in expression position
+-- (here as a column name). Unlike the table-position case above, this identifier expression
+-- resolves to a bare variable reference, so recording it requires visiting the root of the
+-- expression tree. If it is not recorded, the variable is missing from the stored view text and
+-- reading the view back fails.
+DECLARE OR REPLACE VARIABLE identifier_expr_col STRING DEFAULT 'c1';
+CREATE OR REPLACE TEMPORARY VIEW identifier_expr_view AS
+SELECT IDENTIFIER(identifier_expr_col) AS x FROM VALUES(1) AS t(c1);
+SELECT * FROM identifier_expr_view;
+DROP VIEW identifier_expr_view;
+DROP TEMPORARY VARIABLE identifier_expr_col;
+
+-- A GLOBAL TEMPORARY VIEW reads a variable via an IDENTIFIER clause the same way a session-local
+-- temporary view does, so the variable stays resolvable when the stored view text is analyzed again.
+DECLARE OR REPLACE VARIABLE identifier_gtv_col STRING DEFAULT 'c1';
+CREATE OR REPLACE GLOBAL TEMPORARY VIEW identifier_gtv_view AS
+SELECT IDENTIFIER(identifier_gtv_col) AS x FROM VALUES(1) AS t(c1);
+SELECT * FROM global_temp.identifier_gtv_view;
+DROP VIEW global_temp.identifier_gtv_view;
+DROP TEMPORARY VARIABLE identifier_gtv_col;
+
+-- A variable used only to supply a view's NAME via an IDENTIFIER clause is not part of the view
Review Comment:
**Nit (P3):** These target-name cases do not observe the exclusion invariant. If the target variable is accidentally recorded, the temporary CREATE/CACHE reads still succeed because extra allow-list entries are inert while the variable remains declared. Could the tests assert the resulting referred-variable metadata (including the mixed target/body case), and add a persistent CREATE target-only case so an erroneous dependency would trigger the normal persisted-view rejection?
##########
sql/core/src/test/resources/sql-tests/inputs/identifier-clause.sql:
##########
@@ -468,3 +468,169 @@ SELECT EXTRACT(IDENTIFIER('YEAR') FROM DATE'2024-01-15');
SELECT TIMESTAMPADD(IDENTIFIER('YEAR'), 1, DATE'2024-01-15');
DROP SCHEMA identifier_clause_test_schema;
+
+-- A variable read only by a temporary view's IDENTIFIER clause is still a variable the view refers
+-- to, so it stays resolvable when the stored view text is analyzed again.
+CREATE OR REPLACE TEMPORARY VIEW identifier_var_target AS SELECT 1 AS c1;
+DECLARE OR REPLACE VARIABLE identifier_var_name STRING DEFAULT 'identifier_var_target';
+CREATE OR REPLACE TEMPORARY VIEW identifier_var_view AS
+SELECT * FROM IDENTIFIER(identifier_var_name);
+SELECT count(*) AS row_count FROM identifier_var_view;
+DROP VIEW identifier_var_view;
+DROP VIEW identifier_var_target;
+DROP TEMPORARY VARIABLE identifier_var_name;
+
+-- A permanent view must still reject a temporary variable read via an IDENTIFIER clause.
+CREATE TEMPORARY VIEW identifier_perm_target AS SELECT 1 AS c1;
+DECLARE OR REPLACE VARIABLE identifier_perm_name STRING DEFAULT 'identifier_perm_target';
+CREATE VIEW identifier_perm_view AS SELECT * FROM IDENTIFIER(identifier_perm_name);
+DROP VIEW identifier_perm_target;
+DROP TEMPORARY VARIABLE identifier_perm_name;
+
+-- A permanent ALTER VIEW must also reject a temporary variable read via an IDENTIFIER clause.
+-- The IDENTIFIER target is a permanent table so the variable is the only temporary object.
+CREATE SCHEMA identifier_alter_schema;
+USE identifier_alter_schema;
+CREATE TABLE identifier_alter_target (c1 INT) USING parquet;
+DECLARE OR REPLACE VARIABLE identifier_alter_name STRING DEFAULT 'identifier_alter_target';
+CREATE VIEW identifier_alter_view AS SELECT 1 AS c1;
+ALTER VIEW identifier_alter_view AS SELECT * FROM IDENTIFIER(identifier_alter_name);
+SELECT * FROM identifier_alter_view;
+DROP VIEW identifier_alter_view;
+DROP TABLE identifier_alter_target;
+DROP TEMPORARY VARIABLE identifier_alter_name;
+USE default;
+DROP SCHEMA identifier_alter_schema;
+
+-- ALTER VIEW whose TARGET is picked by a variable, with a body that has no temp dependency, must
+-- still succeed: the variable is not a dependency of the view definition.
+CREATE SCHEMA ivt_schema;
+USE ivt_schema;
+CREATE VIEW ivt_target AS SELECT 1 AS c1;
+DECLARE OR REPLACE VARIABLE ivt_name STRING DEFAULT 'ivt_target';
+ALTER VIEW IDENTIFIER(ivt_name) AS SELECT 2 AS c1;
+SELECT * FROM ivt_target;
+DROP VIEW ivt_target;
+DROP TEMPORARY VARIABLE ivt_name;
+USE default;
+DROP SCHEMA ivt_schema;
+
+-- A temporary view whose body reads a variable via an IDENTIFIER clause in expression position
+-- (here as a column name). Unlike the table-position case above, this identifier expression
+-- resolves to a bare variable reference, so recording it requires visiting the root of the
+-- expression tree. If it is not recorded, the variable is missing from the stored view text and
Review Comment:
**Nit (P3):** The variable does not disappear from the stored view text: that text still contains `IDENTIFIER(identifier_expr_col)`. Without this fix, its name is missing from the referred-variable metadata, so view-context re-analysis rejects the reference. Could this comment name the metadata loss rather than a stored-text loss?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -166,6 +166,19 @@ case class AnalysisContext(
// lookup a temporary function. And export to the view metadata.
referredTempFunctionNames: mutable.Set[String] = mutable.Set.empty,
referredTempVariableNames: Seq[Seq[String]] = Seq.empty,
+ // Like `referredTempFunctionNames`, this is populated only by fixed-point analysis (by
+ // `ResolveIdentifierClause`, the sole writer). A temporary view, temporary ALTER VIEW, or
+ // CACHE TABLE AS SELECT stores the names in its metadata so they resolve when the stored view
+ // text is analyzed again. A persisted CREATE/ALTER VIEW instead rejects them -- usually in
+ // `ResolveIdentifierClause` while resolving the body, and otherwise (e.g. an IDENTIFIER nested
+ // in a scalar subquery) in `verifyTemporaryObjectsNotExists` at run time. When
Review Comment:
**Nit (P3):** `verifyTemporaryObjectsNotExists` is not uniformly a run-time check. The v1 runnable commands call it while executing, but non-session v2 CREATE/ALTER reaches it from `CheckViewReferences` during analysis. Could this wording distinguish those phases, or simply say the captured names are used for persisted-view validation?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "stevomitric (via GitHub)" <gi...@apache.org>.
stevomitric commented on code in PR #58606:
URL: https://github.com/apache/spark/pull/58606#discussion_r3987978396
##########
sql/core/src/test/resources/sql-tests/inputs/identifier-clause.sql:
##########
@@ -468,3 +468,91 @@ SELECT EXTRACT(IDENTIFIER('YEAR') FROM DATE'2024-01-15');
SELECT TIMESTAMPADD(IDENTIFIER('YEAR'), 1, DATE'2024-01-15');
DROP SCHEMA identifier_clause_test_schema;
+
Review Comment:
Expanded to expression-position IDENTIFIER(var), ALTER VIEW reject and ALTER VIEW honoring the flag
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "stevomitric (via GitHub)" <gi...@apache.org>.
stevomitric commented on code in PR #58606:
URL: https://github.com/apache/spark/pull/58606#discussion_r3987982362
##########
sql/core/src/test/resources/sql-tests/analyzer-results/identifier-clause-legacy.sql.out:
##########
@@ -3177,3 +3177,466 @@ DROP SCHEMA identifier_clause_test_schema
-- !query analysis
DropNamespace false, false
+- ResolvedNamespace V2SessionCatalog(spark_catalog), [identifier_clause_test_schema]
+
Review Comment:
yea, makes sense. reworded.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on code in PR #58606:
URL: https://github.com/apache/spark/pull/58606#discussion_r3978247651
##########
sql/core/src/test/resources/sql-tests/analyzer-results/identifier-clause-legacy.sql.out:
##########
@@ -3177,3 +3177,466 @@ DROP SCHEMA identifier_clause_test_schema
-- !query analysis
DropNamespace false, false
+- ResolvedNamespace V2SessionCatalog(spark_catalog), [identifier_clause_test_schema]
+
Review Comment:
identifier-clause-legacy.sql ALTERs a permanent view under the flag and drops it without SELECT. prepareTable / alterPermanentView never pass the recorded names into generateViewProperties, so the view stays unqueryable. Either persist the names or drop the implication that the flag repairs IDENTIFIER views.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "stevomitric (via GitHub)" <gi...@apache.org>.
stevomitric commented on code in PR #58606:
URL: https://github.com/apache/spark/pull/58606#discussion_r3976802982
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveIdentifierClause.scala:
##########
@@ -110,16 +125,13 @@ class ResolveIdentifierClause(earlyBatches: Seq[RuleExecutor[LogicalPlan]#Batch]
collectTempVars(child)
}
+ // Visits `child` itself as well as its descendants, so that an identifier expression which is
+ // just a variable reference is collected too.
private def collectTemporaryVariablesInExpressionTree(child: Expression): Seq[Seq[String]] = {
- def collectTempVars(child: Expression): Seq[Seq[String]] = {
- child.flatMap { expr =>
- expr.children.flatMap(_.flatMap {
- case e: SubqueryExpression => collectTemporaryVariablesInLogicalPlan(e.plan)
- case r: VariableReference => Seq(r.originalNameParts)
- case _ => Seq.empty
- })
- }.distinct
- }
- collectTempVars(child)
+ child.flatMap {
+ case e: SubqueryExpression => collectTemporaryVariablesInLogicalPlan(e.plan)
+ case r: VariableReference => Seq(r.originalNameParts)
+ case _ => Seq.empty
+ }.distinct
Review Comment:
yes, the old behavior doesn't pickup `VariableReference` at the root.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #58606:
URL: https://github.com/apache/spark/pull/58606#issuecomment-5874652601
Thank you @stevomitric and @cloud-fan! Seems like we are getting a bit caught up in nits here... Let's just merge and if there are any worthwhile followups, we can do them separately?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #58606:
URL: https://github.com/apache/spark/pull/58606#discussion_r4028287135
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -499,6 +524,53 @@ class Analyzer(
private def executeSameContext(plan: LogicalPlan): LogicalPlan =
runWithSessionConf(super.execute(plan))
+ /**
+ * Like [[executeAndCheck]], but also returns the temporary variables recorded via IDENTIFIER
+ * clauses during this analysis (`AnalysisContext.referredTempVariableNamesUnderIdentifier`).
+ *
+ * Those variables are absent from the analyzed plan (the placeholder is replaced by the plan
+ * built from the evaluated name), and the accumulator that holds them is discarded when the
+ * analysis scope exits. A caller that separately validates a freshly analyzed body against
+ * persisted-view rules (metric-view creation) therefore cannot recover them afterwards, so this
+ * entry point reads them inside the owning scope and freezes them into the returned result.
+ *
+ * When single-pass resolution is forced on, this defers to [[executeAndCheck]] so the configured
+ * routing is preserved: the only caller analyzes a metric-view placeholder, which is explicitly
+ * unsupported by the single-pass resolver, and forced mode must surface that incompatibility
+ * rather than silently succeeding through fixed-point analysis (no IDENTIFIER variables are
+ * captured on that path -- the call fails before persisted-view validation is reached).
+ * Otherwise it runs the fixed-point analyzer directly, in a context it owns so the accumulator
+ * stays readable.
+ */
+ def executeAndCheckReferredTempVariablesUnderIdentifier(
+ plan: LogicalPlan,
+ tracker: QueryPlanningTracker): (LogicalPlan, Seq[Seq[String]]) = {
+ if (plan.analyzed) {
+ (plan, Seq.empty)
+ } else if (conf.getConf(SQLConf.ANALYZER_SINGLE_PASS_RESOLVER_ENABLED)) {
Review Comment:
**Nit (P3):** Could this helper keep route selection, analysis, dependency snapshotting, and `checkAnalysis` within the Analyzer's session-conf scope? It currently reads ambient `SQLConf` before that boundary and checks after `executeSameContext` restores it. If session A's analyzer is invoked while session B is active with the opposite single-pass setting, the call can bypass A's forced-mode failure or return an empty dependency snapshot. A conflicting-active-session regression would lock down the ownership boundary.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/MetricViewSuite.scala:
##########
@@ -81,6 +82,54 @@ abstract class MetricViewSuite extends QueryTest {
}
}
+ test("metric view SQL source rejects a temporary variable read via an IDENTIFIER clause") {
+ // A variable read via an IDENTIFIER clause in the metric-view SQL source is absent from the
+ // analyzed plan, so it is captured during analysis and must be rejected for a persisted metric
+ // view, unless `spark.sql.legacy.allowSessionVariableInPersistedView` is enabled.
+ val metricView = MetricView(
+ "0.1",
+ SQLSource("SELECT region, count FROM IDENTIFIER(mv_ident_source)"),
+ None,
+ Seq(
+ Column("region", DimensionExpression("region"), 0),
+ Column("count_sum", MeasureExpression("sum(count)"), 1)))
+ sql(s"DECLARE OR REPLACE VARIABLE mv_ident_source STRING DEFAULT '$testTableName'")
+ try {
+ val ex = intercept[AnalysisException] {
+ createMetricView("mv_ident_metric_view", metricView)
+ }
+ assert(ex.getCondition == "INVALID_TEMP_OBJ_REFERENCE")
+
+ // With the legacy flag enabled, creation is allowed (the flag does not persist the variable).
+ withSQLConf(SQLConf.VARIABLES_UNDER_IDENTIFIER_IN_VIEW.key -> "true") {
+ withView("mv_ident_metric_view") {
+ createMetricView("mv_ident_metric_view", metricView)
+ }
+ }
+ } finally {
+ sql("DROP TEMPORARY VARIABLE mv_ident_source")
+ }
+ }
+
+ test("metric view creation preserves configured analyzer routing under forced single-pass") {
+ // `MetricViewPlaceholder` is explicitly unsupported by the single-pass resolver. Metric-view
+ // creation analyzes the source through `executeAndCheckReferredTempVariablesUnderIdentifier`,
+ // which must route through HybridAnalyzer when single-pass is forced on so that incompatibility
+ // surfaces, rather than silently analyzing through fixed-point.
+ val metricView = MetricView(
+ "0.1",
+ SQLSource("SELECT region, count FROM test_table"),
+ None,
+ Seq(
+ Column("region", DimensionExpression("region"), 0),
+ Column("count_sum", MeasureExpression("sum(count)"), 1)))
+ withSQLConf(SQLConf.ANALYZER_SINGLE_PASS_RESOLVER_ENABLED.key -> "true") {
+ intercept[Exception] {
Review Comment:
**Nit (P3):** Could this assert the specific forced-single-pass incompatibility, preferably via its stable error condition or a narrowly typed equivalent? As written, any unrelated parser, resolution, analysis, or catalog exception satisfies the test, so it does not prove that the intended `MetricViewPlaceholder` routing failure occurred.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala:
##########
@@ -134,8 +138,20 @@ case class CreateViewCommand(
// When creating a permanent view, not allowed to reference temporary objects.
// This should be called after `qe.assertAnalyzed()` (i.e., `child` can be resolved)
+ // A variable read via an IDENTIFIER clause is absent from the analyzed plan (the placeholder is
+ // replaced by the evaluated name), so it cannot be rediscovered by scanning `analyzedPlan` --
+ // notably when the IDENTIFIER sits inside a scalar subquery. Pass the recorded names explicitly
+ // so a persisted view still rejects the dependency, honoring
+ // `spark.sql.legacy.allowSessionVariableInPersistedView` like ALTER VIEW.
+ val referredVarsUnderIdentifier =
+ if (sparkSession.sessionState.conf.getConf(SQLConf.VARIABLES_UNDER_IDENTIFIER_IN_VIEW)) {
Review Comment:
**Nit (P3):** Could the v1 SQL coverage also create a view successfully with `spark.sql.legacy.allowSessionVariableInPersistedView` enabled? This CREATE-specific gate is independent of ALTER: removing or inverting it still leaves the default CREATE rejection and current legacy ALTER case green.
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2MetadataViewSuite.scala:
##########
@@ -287,6 +287,59 @@ class DataSourceV2MetadataViewSuite extends SharedSparkSession {
}
}
+ test("v2 CREATE / ALTER VIEW rejects a temporary variable read via an IDENTIFIER clause") {
+ // A variable read via an IDENTIFIER clause is not present in the analyzed view body (the
+ // placeholder is replaced by the plan built from the evaluated name), so it is captured during
+ // analysis (referredTempVariablesUnderIdentifier) and must be rejected for a permanent v2 view,
+ // just like a temporary function. The ALTER path is only covered once the v2 command threads
+ // the captured variables into CheckViewReferences.
+ withTable("spark_catalog.default.t") {
+ Seq(1, 2, 3).toDF("x").write.saveAsTable("spark_catalog.default.t")
+ sql("DECLARE OR REPLACE VARIABLE v2_ident_col STRING DEFAULT 'x'")
+ try {
+ val createEx = intercept[AnalysisException] {
+ sql("CREATE VIEW view_catalog.default.v_ident_var AS " +
+ "SELECT IDENTIFIER(v2_ident_col) FROM spark_catalog.default.t")
+ }
+ assert(createEx.getCondition == "INVALID_TEMP_OBJ_REFERENCE")
+
+ sql("CREATE VIEW view_catalog.default.v_ident_var AS SELECT x FROM spark_catalog.default.t")
Review Comment:
**Nit (P3):** Please drop `view_catalog.default.v_ident_var` in the unconditional cleanup as well, tolerating the path where CREATE was rejected. The setup CREATE succeeds before the ALTER assertion, but the current `finally` block removes only the table, so retrying this case can fail with `VIEW_ALREADY_EXISTS` before it reaches the behavior under test.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #58606:
URL: https://github.com/apache/spark/pull/58606#discussion_r4000506575
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala:
##########
@@ -629,7 +663,8 @@ object ViewHelper extends SQLConfHelper with Logging with CapturesConfig {
isTemporary: Boolean,
viewNameParts: Seq[String],
child: LogicalPlan,
- referredTempFunctions: Seq[String]): Unit = {
+ referredTempFunctions: Seq[String],
+ referredTempVariablesUnderIdentifier: Seq[Seq[String]] = Seq.empty): Unit = {
Review Comment:
**Non-blocking (P2):** Metric-view SQL is analyzed by `analyzer.executeAndCheck`, which restores `AnalysisContext` before this validation. If a source uses `IDENTIFIER(sessionVar)`, analysis removes the placeholder from the plan and the captured variable remains only in the discarded context, so this call receives the default empty set. Creation can then persist a metric view whose SQL cannot be resolved later. Could this path retain and pass the captured dependencies to `verifyTemporaryObjectsNotExists`, with a regression case?
**Recommended change:** Add an analyzer entry point that returns the analyzed plan together with the IDENTIFIER session-variable dependencies captured by that invocation, consume that result in metric-view write analysis, apply the existing legacy configuration gate, and pass the resulting names to ViewHelper.verifyTemporaryObjectsNotExists. Add metric-view SQL-source regressions for default rejection and legacy-enabled creation on the supported catalog paths.
**Why this works:** Capture AnalysisContext.referredTempVariableNamesUnderIdentifier inside executeAndCheck's owned analysis scope immediately after successful analysis, freeze it into the returned result, and validate the metric view from that result after the scope is restored. This keeps dependency ownership tied to the producing analysis and avoids consulting ambient state.
**Scope:** sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis, sql/core/src/main/scala/org/apache/spark/sql/execution/command, sql/core/src/test/scala/org/apache/spark/sql/execution
**Compatibility:** Metric views without IDENTIFIER session-variable dependencies, including ordinary asset and SQL sources, keep their existing schema derivation and catalog behavior.
**Risks:** A new analyzer result API could accidentally expose mutable AnalysisContext state or capture dependencies from an enclosing analysis. Applying the legacy gate at a different point from ordinary persisted views could make metric views diverge from existing CREATE and ALTER behavior.
**Constraints:** Return an immutable snapshot owned by the exact invocation; do not expose or retain the mutable thread-local AnalysisContext. Preserve nested-view accumulator reset and SQL-function accumulator sharing semantics. Honor spark.sql.legacy.allowSessionVariableInPersistedView exactly as other persisted-view paths do.
**Success:** A metric-view SQL source whose IDENTIFIER reads a session variable is rejected at creation with INVALID_TEMP_OBJ_REFERENCE when the legacy option is disabled. The same definition continues through the compatibility branch when the legacy option is enabled. Validation reports the actual metric-view target and referenced variable from the same analysis invocation.
##########
sql/core/src/test/resources/sql-tests/results/identifier-clause.sql.out:
##########
@@ -3197,3 +3197,826 @@ DROP SCHEMA identifier_clause_test_schema
struct<>
-- !query output
+
+
+-- !query
+CREATE OR REPLACE TEMPORARY VIEW identifier_var_target AS SELECT 1 AS c1
+-- !query schema
+struct<>
+-- !query output
+
+
+
+-- !query
+DECLARE OR REPLACE VARIABLE identifier_var_name STRING DEFAULT 'identifier_var_target'
+-- !query schema
+struct<>
+-- !query output
+
+
+
+-- !query
+CREATE OR REPLACE TEMPORARY VIEW identifier_var_view AS
+SELECT * FROM IDENTIFIER(identifier_var_name)
+-- !query schema
+struct<>
+-- !query output
+
+
+
+-- !query
+SELECT count(*) AS row_count FROM identifier_var_view
+-- !query schema
+struct<row_count:bigint>
+-- !query output
+1
+
+
+-- !query
+DROP VIEW identifier_var_view
+-- !query schema
+struct<>
+-- !query output
+
+
+
+-- !query
+DROP VIEW identifier_var_target
+-- !query schema
+struct<>
+-- !query output
+
+
+
+-- !query
+DROP TEMPORARY VARIABLE identifier_var_name
+-- !query schema
+struct<>
+-- !query output
+
+
+
+-- !query
+CREATE TEMPORARY VIEW identifier_perm_target AS SELECT 1 AS c1
+-- !query schema
+struct<>
+-- !query output
+
+
+
+-- !query
+DECLARE OR REPLACE VARIABLE identifier_perm_name STRING DEFAULT 'identifier_perm_target'
+-- !query schema
+struct<>
+-- !query output
+
+
+
+-- !query
+CREATE VIEW identifier_perm_view AS SELECT * FROM IDENTIFIER(identifier_perm_name)
+-- !query schema
+struct<>
+-- !query output
+org.apache.spark.sql.AnalysisException
+{
+ "errorClass" : "INVALID_TEMP_OBJ_REFERENCE",
+ "sqlState" : "42K0F",
+ "messageParameters" : {
+ "obj" : "VIEW",
+ "objName" : "`unknown`",
Review Comment:
**Non-blocking (P2):** This early rejection substitutes `unknown` even though the CREATE VIEW target has already been resolved. A relation-position IDENTIFIER dependency now reaches this branch and reports an unidentifiable object, while the scalar-subquery validation path reports the actual view name. Could this use the resolved target identifier so both invalid definition shapes produce the same actionable diagnostic?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #58606:
URL: https://github.com/apache/spark/pull/58606#discussion_r3999375254
##########
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:
Addressed in the current head: CACHE TABLE AS SELECT now forwards query-only IDENTIFIER dependencies through planning and execution, with target-name exclusion covered.
<!-- SPARK_DEV_REVIEW_REPLY {"feedback_id":"inline:3995050248","thread_id":"inline:3995050248","verdict_sha256":"12b7f981564f986cf8d4f4a9b51e54d32eca69438988a6670d47d8c183663030"} -->
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59323][SQL] Record temp variables read by an IDENTIFIER clause as referred by the view [spark]
Posted by "stevomitric (via GitHub)" <gi...@apache.org>.
stevomitric commented on PR #58606:
URL: https://github.com/apache/spark/pull/58606#issuecomment-5632892769
Adding @cloud-fan too. Could you please take a look as well?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org