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