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