sunchao commented on code in PR #58604:
URL: https://github.com/apache/spark/pull/58604#discussion_r3963645372
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -328,6 +328,9 @@ trait CheckAnalysis extends LookupCatalog with
QueryErrorsBase with PlanToString
}
preemptedError.clear()
try {
+ if (SQLConf.get.restrictedModeEnabled) {
+ checkRestrictedMode(inlinedPlan)
+ }
Review Comment:
[P1] Enforce restricted mode in the single-pass analyzer
This check runs only through the legacy analyzer. Both single-pass modes use
ResolverRunner, which supports reflection expressions and marks the resulting
plan analyzed without calling CheckAnalysis. Restricted mode can therefore
accept the functions it promises to reject. Apply the restriction across both
analyzer paths and add an ordinary reflection SELECT test across analyzer
modes; the current tests explicitly call legacy analysis or use commands that
fall back to it.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -339,6 +342,45 @@ trait CheckAnalysis extends LookupCatalog with
QueryErrorsBase with PlanToString
plan.setAnalyzed()
}
+ /**
+ * Rejects SQL features that load or execute externally provided code or
scripts when the
+ * restricted execution mode is enabled. Unlike `checkAnalysis0`, which
skips already-analyzed
+ * sub-plans (`case p if p.analyzed`), this walks the whole plan --
including analyzed sub-plans
+ * reused from a temporary view or a cached Dataset, and the bodies of
analysis-only commands
+ * (CTAS, `CACHE TABLE ... AS SELECT`, `CREATE`/`ALTER VIEW`) which move
into `innerChildren`
+ * once analyzed -- and descends into subquery plans, so these features
cannot slip through.
+ */
+ private def checkRestrictedMode(plan: LogicalPlan): Unit = {
+ def checkExpression(expr: Expression): Unit = expr.foreach {
+ // A try_reflect call stays a RuntimeReplaceable wrapper until
optimization, so match it
+ // before its CallMethodViaReflection replacement to report the right
function name.
+ case t: TryReflect =>
+ throw QueryCompilationErrors.restrictedModeFeatureError(
+ s"The ${toSQLId(t.prettyName)} function")
+ case c: CallMethodViaReflection =>
+ throw QueryCompilationErrors.restrictedModeFeatureError(
+ s"The ${toSQLId(c.prettyName)} function")
Review Comment:
[P1] Reject reflection before class initialization
Legacy analysis can initialize the referenced class before reaching this
rejection. Alias resolution reads Expression.resolved, which invokes
CallMethodViaReflection.checkInputDataTypes; that calls classExists/findMethod,
which use Utils.classForName with initialize=true. A previously uninitialized
class already on the classpath can thus execute its initializer despite the
eventual restricted-mode error. Add an early restriction before class lookup,
retaining this final traversal. A fixture whose initializer increments a
counter would verify rejection occurs without initialization.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -339,6 +342,45 @@ trait CheckAnalysis extends LookupCatalog with
QueryErrorsBase with PlanToString
plan.setAnalyzed()
}
+ /**
+ * Rejects SQL features that load or execute externally provided code or
scripts when the
+ * restricted execution mode is enabled. Unlike `checkAnalysis0`, which
skips already-analyzed
+ * sub-plans (`case p if p.analyzed`), this walks the whole plan --
including analyzed sub-plans
+ * reused from a temporary view or a cached Dataset, and the bodies of
analysis-only commands
+ * (CTAS, `CACHE TABLE ... AS SELECT`, `CREATE`/`ALTER VIEW`) which move
into `innerChildren`
+ * once analyzed -- and descends into subquery plans, so these features
cannot slip through.
+ */
+ private def checkRestrictedMode(plan: LogicalPlan): Unit = {
+ def checkExpression(expr: Expression): Unit = expr.foreach {
+ // A try_reflect call stays a RuntimeReplaceable wrapper until
optimization, so match it
+ // before its CallMethodViaReflection replacement to report the right
function name.
+ case t: TryReflect =>
+ throw QueryCompilationErrors.restrictedModeFeatureError(
+ s"The ${toSQLId(t.prettyName)} function")
+ case c: CallMethodViaReflection =>
+ throw QueryCompilationErrors.restrictedModeFeatureError(
+ s"The ${toSQLId(c.prettyName)} function")
+ case s: SubqueryExpression =>
+ checkPlan(s.plan)
+ case _ =>
+ }
+ def checkPlan(p: LogicalPlan): Unit = p.foreach {
+ case _: ScriptTransformation =>
+ throw QueryCompilationErrors.restrictedModeFeatureError(
+ "The TRANSFORM ... USING clause")
+ case node =>
+ node.expressions.foreach(checkExpression)
+ // The body of an analysis-only command (CTAS, `CACHE TABLE ... AS
SELECT`,
+ // `CREATE`/`ALTER VIEW`) moves from `children` into `innerChildren`
once the command is
+ // analyzed, so `foreach` (which follows `children`) would not
otherwise reach it.
+ node.innerChildren.foreach {
+ case inner: LogicalPlan => checkPlan(inner)
Review Comment:
[P2] Traverse each subquery only once
QueryPlan.innerChildren already returns the node’s subqueries, which
checkExpression also visits through SubqueryExpression.plan. Every nesting
level therefore doubles traversal of its descendants, making validation
exponential for allowed nested scalar queries. Four nested SELECTs cause 30
plan-node visits for eight distinct nodes. Preserve command-body coverage while
traversing each subquery once, and add coverage for nested queries containing
no restricted features.
--
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]
