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: [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]
