You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "ilicmarkodb (via GitHub)" <gi...@apache.org> on 2026/03/09 17:17:21 UTC
[PR] [SPARK-55891][SQL] Preserve the SQL scripting context inside EXECUTE IMMEDIATE [spark]
ilicmarkodb opened a new pull request, #54691:
URL: https://github.com/apache/spark/pull/54691
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
In this PR, I propose preserving the SQL Scripting context inside `EXECUTE IMMEDIATE`, instead of setting it to `null` while executing the command. I propose persisting everything except the variable manager, since local variables should not be resolvable inside the `EXECUTE IMMEDIATE` body.
### Why are the changes needed?
Improvement.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
New tests.
### Was this patch authored or co-authored using generative AI tooling?
Yes, Claude.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55891][SQL] Preserve the SQL scripting context inside EXECUTE IMMEDIATE [spark]
Posted by "miland-db (via GitHub)" <gi...@apache.org>.
miland-db commented on code in PR #54691:
URL: https://github.com/apache/spark/pull/54691#discussion_r2911022514
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveExecuteImmediate.scala:
##########
@@ -185,14 +186,32 @@ case class ResolveExecuteImmediate(sparkSession: SparkSession, catalogManager: C
}
/**
- * Temporarily isolates the SQL scripting context during EXECUTE IMMEDIATE execution.
- * This makes withinSqlScript() return false, ensuring that statements within EXECUTE IMMEDIATE
- * are not affected by the outer SQL script context (e.g., local variables, script-specific
- * errors).
+ * Temporarily hides the local variable context while executing the `EXECUTE IMMEDIATE` command.
+ * This is expected behavior, as local variables cannot be resolved within the body of this
+ * command. This does not apply to session variables. The rest of the SQL scripting context is
+ * preserved.
+ *
+ * {{{
+ * DECLARE VARIABLE v1 = 1; -- Session variable.
+ * BEGIN
+ * DECLARE v2 = 2; -- Local variable.
+ * EXECUTE IMMEDIATE 'SELECT v1'; -- Should work.
+ * EXECUTE IMMEDIATE 'SELECT v2'; -- Should fail.
+ * EXECUTE IMMEDIATE 'SELECT ?' USING v2; -- Should work.
+ * END
+ * }}}
*/
- private def withIsolatedLocalVariableContext[A](f: => A): A = {
- // Completely clear the SQL scripting context to make withinSqlScript() return false
- val handle = SqlScriptingContextManager.create(null)
+ private def withHiddenLocalVariables[A](f: => A): A = {
+ val newContextManager = SqlScriptingContextManager.get() match {
+ case Some(contextManager) =>
+ new SqlScriptingContextManagerTrait {
Review Comment:
Maybe we could add a comment explaining that this refers to `trait SqlScriptingContextManager`, which is renamed in the imports.
This part of the code is already a bit difficult to follow, with traits, implementations, and handles involved, so a small clarification like that would make it easier to understand.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55891][SQL] Preserve the SQL scripting context inside EXECUTE IMMEDIATE [spark]
Posted by "ilicmarkodb (via GitHub)" <gi...@apache.org>.
ilicmarkodb commented on code in PR #54691:
URL: https://github.com/apache/spark/pull/54691#discussion_r2911130969
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveExecuteImmediate.scala:
##########
@@ -185,14 +186,32 @@ case class ResolveExecuteImmediate(sparkSession: SparkSession, catalogManager: C
}
/**
- * Temporarily isolates the SQL scripting context during EXECUTE IMMEDIATE execution.
- * This makes withinSqlScript() return false, ensuring that statements within EXECUTE IMMEDIATE
- * are not affected by the outer SQL script context (e.g., local variables, script-specific
- * errors).
+ * Temporarily hides the local variable context while executing the `EXECUTE IMMEDIATE` command.
+ * This is expected behavior, as local variables cannot be resolved within the body of this
+ * command. This does not apply to session variables. The rest of the SQL scripting context is
+ * preserved.
+ *
+ * {{{
+ * DECLARE VARIABLE v1 = 1; -- Session variable.
+ * BEGIN
+ * DECLARE v2 = 2; -- Local variable.
+ * EXECUTE IMMEDIATE 'SELECT v1'; -- Should work.
+ * EXECUTE IMMEDIATE 'SELECT v2'; -- Should fail.
+ * EXECUTE IMMEDIATE 'SELECT ?' USING v2; -- Should work.
+ * END
+ * }}}
*/
- private def withIsolatedLocalVariableContext[A](f: => A): A = {
- // Completely clear the SQL scripting context to make withinSqlScript() return false
- val handle = SqlScriptingContextManager.create(null)
+ private def withHiddenLocalVariables[A](f: => A): A = {
+ val newContextManager = SqlScriptingContextManager.get() match {
+ case Some(contextManager) =>
+ new SqlScriptingContextManagerTrait {
Review Comment:
done!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55891][SQL] Preserve the SQL scripting context inside EXECUTE IMMEDIATE [spark]
Posted by "ilicmarkodb (via GitHub)" <gi...@apache.org>.
ilicmarkodb commented on code in PR #54691:
URL: https://github.com/apache/spark/pull/54691#discussion_r2907569327
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveExecuteImmediate.scala:
##########
@@ -185,14 +185,32 @@ case class ResolveExecuteImmediate(sparkSession: SparkSession, catalogManager: C
}
/**
- * Temporarily isolates the SQL scripting context during EXECUTE IMMEDIATE execution.
- * This makes withinSqlScript() return false, ensuring that statements within EXECUTE IMMEDIATE
- * are not affected by the outer SQL script context (e.g., local variables, script-specific
- * errors).
+ * Temporary hides the local variable context while executing the `EXECUTE IMMEDIATE` command.
Review Comment:
thanks!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55891][SQL] Preserve the SQL scripting context inside EXECUTE IMMEDIATE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54691:
URL: https://github.com/apache/spark/pull/54691#discussion_r2916668487
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveExecuteImmediate.scala:
##########
@@ -185,14 +186,32 @@ case class ResolveExecuteImmediate(sparkSession: SparkSession, catalogManager: C
}
/**
- * Temporarily isolates the SQL scripting context during EXECUTE IMMEDIATE execution.
- * This makes withinSqlScript() return false, ensuring that statements within EXECUTE IMMEDIATE
- * are not affected by the outer SQL script context (e.g., local variables, script-specific
- * errors).
+ * Temporarily hides the local variable context while executing the `EXECUTE IMMEDIATE` command.
+ * This is expected behavior, as local variables cannot be resolved within the body of this
+ * command. This does not apply to session variables. The rest of the SQL scripting context is
+ * preserved.
+ *
+ * {{{
+ * DECLARE VARIABLE v1 = 1; -- Session variable.
+ * BEGIN
+ * DECLARE v2 = 2; -- Local variable.
+ * EXECUTE IMMEDIATE 'SELECT v1'; -- Should work.
+ * EXECUTE IMMEDIATE 'SELECT v2'; -- Should fail.
+ * EXECUTE IMMEDIATE 'SELECT ?' USING v2; -- Should work.
+ * END
+ * }}}
*/
- private def withIsolatedLocalVariableContext[A](f: => A): A = {
- // Completely clear the SQL scripting context to make withinSqlScript() return false
- val handle = SqlScriptingContextManager.create(null)
- handle.runWith(f)
+ private def withHiddenLocalVariables[A](f: => A): A = {
+ val newContextManager = SqlScriptingContextManager.get() match {
+ case Some(contextManager) =>
+ // SqlScriptingContextManagerTrait is catalog.SqlScriptingContextManager, renamed on import
+ // to distinguish from the companion object.
Review Comment:
The `SqlScriptingContextManager` object (in `org.apache.spark.sql.catalyst`) and the trait (in `org.apache.spark.sql.catalyst.catalog`) are in different packages, so they are not companions by Scala's definition.
```suggestion
// to distinguish from the object of the same name in org.apache.spark.sql.catalyst.
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55891][SQL] Preserve the SQL scripting context inside EXECUTE IMMEDIATE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #54691:
URL: https://github.com/apache/spark/pull/54691#issuecomment-4039802939
thanks, merging to master!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55891][SQL] Preserve the SQL scripting context inside EXECUTE IMMEDIATE [spark]
Posted by "ilicmarkodb (via GitHub)" <gi...@apache.org>.
ilicmarkodb commented on PR #54691:
URL: https://github.com/apache/spark/pull/54691#issuecomment-4038249172
@cloud-fan can you please merge? thanks!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55891][SQL] Preserve the SQL scripting context inside EXECUTE IMMEDIATE [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #54691:
URL: https://github.com/apache/spark/pull/54691#discussion_r2907338480
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveExecuteImmediate.scala:
##########
@@ -185,14 +185,32 @@ case class ResolveExecuteImmediate(sparkSession: SparkSession, catalogManager: C
}
/**
- * Temporarily isolates the SQL scripting context during EXECUTE IMMEDIATE execution.
- * This makes withinSqlScript() return false, ensuring that statements within EXECUTE IMMEDIATE
- * are not affected by the outer SQL script context (e.g., local variables, script-specific
- * errors).
+ * Temporary hides the local variable context while executing the `EXECUTE IMMEDIATE` command.
Review Comment:
```suggestion
* Temporarily hides the local variable context while executing the `EXECUTE IMMEDIATE` command.
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55891][SQL] Preserve the SQL scripting context inside EXECUTE IMMEDIATE [spark]
Posted by "dejankrak-db (via GitHub)" <gi...@apache.org>.
dejankrak-db commented on code in PR #54691:
URL: https://github.com/apache/spark/pull/54691#discussion_r2908359951
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveExecuteImmediate.scala:
##########
@@ -185,14 +186,32 @@ case class ResolveExecuteImmediate(sparkSession: SparkSession, catalogManager: C
}
/**
- * Temporarily isolates the SQL scripting context during EXECUTE IMMEDIATE execution.
- * This makes withinSqlScript() return false, ensuring that statements within EXECUTE IMMEDIATE
- * are not affected by the outer SQL script context (e.g., local variables, script-specific
- * errors).
+ * Temporarily hides the local variable context while executing the `EXECUTE IMMEDIATE` command.
+ * This is expected behavior, as local variables cannot be resolved within the body of this
+ * command. This does not apply to session variables. The rest of the SQL scripting context is
+ * preserved.
+ *
+ * {{{
+ * DECLARE VARIABLE v1 = 1; -- Session variable.
+ * BEGIN
+ * DECLARE v2 = 2; -- Local variable.
+ * EXECUTE IMMEDIATE 'SELECT v1'; -- Should work.
+ * EXECUTE IMMEDIATE 'SELECT v2'; -- Should fail.
+ * EXECUTE IMMEDIATE 'SELECT ?' USING v2; -- Should work.
+ * END
+ * }}}
*/
- private def withIsolatedLocalVariableContext[A](f: => A): A = {
- // Completely clear the SQL scripting context to make withinSqlScript() return false
- val handle = SqlScriptingContextManager.create(null)
+ private def withHiddenLocalVariables[A](f: => A): A = {
+ val newContextManager = SqlScriptingContextManager.get() match {
+ case Some(contextManager) =>
+ new SqlScriptingContextManagerTrait {
+ override def getContext = contextManager.getContext
+ // Empty variable manager so the local variable can't be resolved.
Review Comment:
nit: variable -> variables
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55891][SQL] Preserve the SQL scripting context inside EXECUTE IMMEDIATE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #54691: [SPARK-55891][SQL] Preserve the SQL scripting context inside EXECUTE IMMEDIATE
URL: https://github.com/apache/spark/pull/54691
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org