You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "mihailoale-db (via GitHub)" <gi...@apache.org> on 2026/06/10 13:36:18 UTC
[PR] [SPARK-57369] [SQL] Move main EXECUTE IMMEDIATE resolution logic to common code [spark]
mihailoale-db opened a new pull request, #56429:
URL: https://github.com/apache/spark/pull/56429
### What changes were proposed in this pull request?
In this PR I propose to move main EXECUTE IMMEDIATE resolution logic to common code in order to ease the single-pass implementation.
### Why are the changes needed?
In order to ease the single-pass implementation.
### Does this PR introduce _any_ user-facing change?
No, it's just a refactor.
### How was this patch tested?
Existing tests.
### Was this patch authored or co-authored using generative AI tooling?
Yes.
--
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-57369] [SQL] Move main EXECUTE IMMEDIATE resolution logic to common code [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #56429:
URL: https://github.com/apache/spark/pull/56429#issuecomment-4684941853
one CI job timeout, merging to master/4.x!
--
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-57369] [SQL] Move main EXECUTE IMMEDIATE resolution logic to common code [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #56429:
URL: https://github.com/apache/spark/pull/56429#discussion_r3389600803
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveExecuteImmediate.scala:
##########
@@ -43,23 +43,45 @@ case class ResolveExecuteImmediate(sparkSession: SparkSession, catalogManager: C
plan.resolveOperatorsWithPruning(_.containsPattern(EXECUTE_IMMEDIATE), ruleId) {
case node @ UnresolvedExecuteImmediate(sqlStmtStr, args, targetVariables) =>
if (sqlStmtStr.resolved && targetVariables.forall(_.resolved) && args.forall(_.resolved)) {
- // All resolved - execute immediately and handle INTO clause if present
- if (targetVariables.nonEmpty) {
- // EXECUTE IMMEDIATE ... INTO should generate SetVariable plan with eagerly executed
- // source
- val finalTargetVars = extractTargetVariables(targetVariables)
- val executedSource = executeImmediateQuery(sqlStmtStr, args, hasIntoClause = true)
- SetVariable(finalTargetVars, executedSource)
- } else {
- // Regular EXECUTE IMMEDIATE without INTO - execute and return result directly
- executeImmediateQuery(sqlStmtStr, args, hasIntoClause = false)
- }
+ ResolveExecuteImmediate.resolveExecuteImmediate(
Review Comment:
This delegation only passes `sparkSession` to the companion object. The `catalogManager: CatalogManager` parameter declared alongside it in the class constructor is never used — not in the class body, not in the companion object. Since the PR is already restructuring the class, consider removing `catalogManager` from the constructor and updating the two callers: `BaseSessionStateBuilder.scala:242` and `HiveSessionStateBuilder.scala:137`.
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveExecuteImmediate.scala:
##########
@@ -43,23 +43,45 @@ case class ResolveExecuteImmediate(sparkSession: SparkSession, catalogManager: C
plan.resolveOperatorsWithPruning(_.containsPattern(EXECUTE_IMMEDIATE), ruleId) {
case node @ UnresolvedExecuteImmediate(sqlStmtStr, args, targetVariables) =>
if (sqlStmtStr.resolved && targetVariables.forall(_.resolved) && args.forall(_.resolved)) {
- // All resolved - execute immediately and handle INTO clause if present
- if (targetVariables.nonEmpty) {
- // EXECUTE IMMEDIATE ... INTO should generate SetVariable plan with eagerly executed
- // source
- val finalTargetVars = extractTargetVariables(targetVariables)
- val executedSource = executeImmediateQuery(sqlStmtStr, args, hasIntoClause = true)
- SetVariable(finalTargetVars, executedSource)
- } else {
- // Regular EXECUTE IMMEDIATE without INTO - execute and return result directly
- executeImmediateQuery(sqlStmtStr, args, hasIntoClause = false)
- }
+ ResolveExecuteImmediate.resolveExecuteImmediate(
+ sparkSession, sqlStmtStr, args, targetVariables)
} else {
- // Not all resolved yet - wait for next iteration
node
Review Comment:
The previous code had `// Not all resolved yet - wait for next iteration` here, explaining the iterative analysis idiom (returning `node` unchanged defers re-application to the next fixed-point pass). Consider restoring it:
```suggestion
} else {
// Not all resolved yet - wait for next iteration
node
```
--
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-57369] [SQL] Move main EXECUTE IMMEDIATE resolution logic to common code [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #56429:
URL: https://github.com/apache/spark/pull/56429#issuecomment-4684291911
@mihailoale-db Thank you for addressing review comments. Waiting for CI.
--
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-57369] [SQL] Move main EXECUTE IMMEDIATE resolution logic to common code [spark]
Posted by "mihailoale-db (via GitHub)" <gi...@apache.org>.
mihailoale-db commented on code in PR #56429:
URL: https://github.com/apache/spark/pull/56429#discussion_r3389897740
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveExecuteImmediate.scala:
##########
@@ -43,23 +43,45 @@ case class ResolveExecuteImmediate(sparkSession: SparkSession, catalogManager: C
plan.resolveOperatorsWithPruning(_.containsPattern(EXECUTE_IMMEDIATE), ruleId) {
case node @ UnresolvedExecuteImmediate(sqlStmtStr, args, targetVariables) =>
if (sqlStmtStr.resolved && targetVariables.forall(_.resolved) && args.forall(_.resolved)) {
- // All resolved - execute immediately and handle INTO clause if present
- if (targetVariables.nonEmpty) {
- // EXECUTE IMMEDIATE ... INTO should generate SetVariable plan with eagerly executed
- // source
- val finalTargetVars = extractTargetVariables(targetVariables)
- val executedSource = executeImmediateQuery(sqlStmtStr, args, hasIntoClause = true)
- SetVariable(finalTargetVars, executedSource)
- } else {
- // Regular EXECUTE IMMEDIATE without INTO - execute and return result directly
- executeImmediateQuery(sqlStmtStr, args, hasIntoClause = false)
- }
+ ResolveExecuteImmediate.resolveExecuteImmediate(
Review Comment:
I would do it in a separate PR since this one should be just a code move.
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveExecuteImmediate.scala:
##########
@@ -43,23 +43,45 @@ case class ResolveExecuteImmediate(sparkSession: SparkSession, catalogManager: C
plan.resolveOperatorsWithPruning(_.containsPattern(EXECUTE_IMMEDIATE), ruleId) {
case node @ UnresolvedExecuteImmediate(sqlStmtStr, args, targetVariables) =>
if (sqlStmtStr.resolved && targetVariables.forall(_.resolved) && args.forall(_.resolved)) {
- // All resolved - execute immediately and handle INTO clause if present
- if (targetVariables.nonEmpty) {
- // EXECUTE IMMEDIATE ... INTO should generate SetVariable plan with eagerly executed
- // source
- val finalTargetVars = extractTargetVariables(targetVariables)
- val executedSource = executeImmediateQuery(sqlStmtStr, args, hasIntoClause = true)
- SetVariable(finalTargetVars, executedSource)
- } else {
- // Regular EXECUTE IMMEDIATE without INTO - execute and return result directly
- executeImmediateQuery(sqlStmtStr, args, hasIntoClause = false)
- }
+ ResolveExecuteImmediate.resolveExecuteImmediate(
+ sparkSession, sqlStmtStr, args, targetVariables)
} else {
- // Not all resolved yet - wait for next iteration
node
Review Comment:
Nice catch. Moved the comment to Scala doc.
--
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-57369] [SQL] Move main EXECUTE IMMEDIATE resolution logic to common code [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #56429: [SPARK-57369] [SQL] Move main EXECUTE IMMEDIATE resolution logic to common code
URL: https://github.com/apache/spark/pull/56429
--
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-57369] [SQL] Move main EXECUTE IMMEDIATE resolution logic to common code [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #56429:
URL: https://github.com/apache/spark/pull/56429#discussion_r3396923807
##########
sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveExecuteImmediate.scala:
##########
@@ -43,23 +46,45 @@ case class ResolveExecuteImmediate(sparkSession: SparkSession, catalogManager: C
plan.resolveOperatorsWithPruning(_.containsPattern(EXECUTE_IMMEDIATE), ruleId) {
case node @ UnresolvedExecuteImmediate(sqlStmtStr, args, targetVariables) =>
if (sqlStmtStr.resolved && targetVariables.forall(_.resolved) && args.forall(_.resolved)) {
- // All resolved - execute immediately and handle INTO clause if present
- if (targetVariables.nonEmpty) {
- // EXECUTE IMMEDIATE ... INTO should generate SetVariable plan with eagerly executed
- // source
- val finalTargetVars = extractTargetVariables(targetVariables)
- val executedSource = executeImmediateQuery(sqlStmtStr, args, hasIntoClause = true)
- SetVariable(finalTargetVars, executedSource)
- } else {
- // Regular EXECUTE IMMEDIATE without INTO - execute and return result directly
- executeImmediateQuery(sqlStmtStr, args, hasIntoClause = false)
- }
+ ResolveExecuteImmediate.resolveExecuteImmediate(
+ sparkSession, sqlStmtStr, args, targetVariables)
} else {
- // Not all resolved yet - wait for next iteration
node
}
}
}
+}
+
+/**
+ * Companion object containing shared resolution logic for `EXECUTE IMMEDIATE` statements.
+ */
+object ResolveExecuteImmediate {
+
+ /**
+ * Resolves an [[UnresolvedExecuteImmediate]] node into an executable plan.
+ *
+ * All expressions (`sqlStmtStr`, `args`, `targetVariables`) must already be resolved
+ * before calling this method.
+ *
+ * When an `INTO` clause is present, the dynamic SQL is executed eagerly and the result
+ * is wrapped in a [[SetVariable]] plan that assigns output columns to the target
+ * variables. Without `INTO`, the dynamic SQL is executed and its plan is returned
+ * directly.
Review Comment:
"Executed eagerly" overstates what happens here: only parsing and analysis are eager. Queries come back as `queryExecution.analyzed` precisely to avoid eager evaluation (per the comment inside `executeImmediateQuery`), and the `INTO` source in particular is never eagerly executed since commands are rejected for `INTO` — execution happens when the `SetVariable` command runs.
```suggestion
* When an `INTO` clause is present, the dynamic SQL is eagerly parsed and analyzed, and
* the analyzed plan is wrapped in a [[SetVariable]] plan that assigns output columns to
* the target variables. Without `INTO`, the resulting plan is returned directly
* (commands are executed eagerly; queries are returned analyzed but unexecuted).
```
--
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-57369] [SQL] Move main EXECUTE IMMEDIATE resolution logic to common code [spark]
Posted by "mihailoale-db (via GitHub)" <gi...@apache.org>.
mihailoale-db commented on PR #56429:
URL: https://github.com/apache/spark/pull/56429#issuecomment-4680143795
@cloud-fan @MaxGekk could you PTAL when you find time? 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