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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
