[SPARK-48344][SQL] Add Frames and Scopes to support Exception Handlers and Local Variables#49006
[SPARK-48344][SQL] Add Frames and Scopes to support Exception Handlers and Local Variables#49006miland-db wants to merge 38 commits intoapache:masterfrom
Conversation
# Conflicts: # sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingExecution.scala # sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionNode.scala # sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingInterpreter.scala # sql/core/src/test/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionNodeSuite.scala # sql/core/src/test/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionSuite.scala # sql/core/src/test/scala/org/apache/spark/sql/scripting/SqlScriptingInterpreterSuite.scala
miland-db
changed the title
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
Show resolved
Hide resolved
sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingExecution.scala
Show resolved
Hide resolved
sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionContext.scala
Outdated
Show resolved
Hide resolved
sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingInterpreter.scala
Outdated
Show resolved
Hide resolved
sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingExecution.scala
Outdated
Show resolved
Hide resolved
sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionNode.scala
Show resolved
Hide resolved
davidm-db
left a comment
davidm-db
left a comment
There was a problem hiding this comment.
Minor comments, LGTM otherwise
# Conflicts: # sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
|
@MaxGekk @cloud-fan Can someone please review this PR when you have some time? It's been waiting for a couple of days for a review. |
# Conflicts: # sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingInterpreter.scala
sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionNode.scala
Outdated
Show resolved
Hide resolved
sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionNode.scala
Outdated
Show resolved
Hide resolved
sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionNode.scala
Outdated
Show resolved
Hide resolved
sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingInterpreter.scala
Show resolved
Hide resolved
sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionNode.scala
Outdated
Show resolved
Hide resolved
sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionContext.scala
Outdated
Show resolved
Hide resolved
sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionContext.scala
Outdated
Show resolved
Hide resolved
|
+1, LGTM. All GAs (job link) passed. Merging to master. |
| collection: Seq[CompoundPlanStatement], | ||
| label: Option[String]) extends Command with CompoundPlanStatement { | ||
| label: Option[String], | ||
| isScope: Boolean) extends Command with CompoundPlanStatement { |
There was a problem hiding this comment.
let's add parameter doc for this new parameter.
| } | ||
|
|
||
| def enterScope(label: String): Unit = { | ||
| scopes.addOne(new SqlScriptingExecutionScope(label)) |
There was a problem hiding this comment.
is addOne the same as append?
There was a problem hiding this comment.
It is the same. It will be updated in the follow-up PRs.
|
|
||
| // Remove all scopes until the one with the given label. | ||
| while (scopes.nonEmpty && scopes.last.label != label) { | ||
| scopes.remove(scopes.length - 1) |
There was a problem hiding this comment.
is it the same as scopes.dropRight(1)?
There was a problem hiding this comment.
Yes, semantically it is doing the same thing.
There was a problem hiding this comment.
remove is modifying ListBuffer scopes, and dropRight returns new ListBuffer instance. I would keep remove for performance reasons.
| * @param label | ||
| * Label set by user to CompoundBody or None otherwise. | ||
| * @param isScope | ||
| * Flag that indicates whether Compound Body is scope or not. |
There was a problem hiding this comment.
This doesn't tell anything... what does is scope mean?
There was a problem hiding this comment.
I will provide more detailed explanation in the follow-up PR.
| // This check makes this operation idempotent. | ||
| if (isScope && scopeStatus == ScopeStatus.NOT_ENTERED) { | ||
| scopeStatus = ScopeStatus.INSIDE | ||
| context.enterScope(label.get) |
There was a problem hiding this comment.
only labeled statement has scope?
There was a problem hiding this comment.
Yes, only labeled statements can actually be scope, but not all labeled statements are scope. Only CompoundBody can be scope.
| */ | ||
| class SqlScriptingExecutionContext { | ||
| // List of frames that are currently active. | ||
| val frames: ListBuffer[SqlScriptingExecutionFrame] = ListBuffer.empty |
There was a problem hiding this comment.
to confirm: as of today, we can only have at most one frame, right?
6 participants
What changes were proposed in this pull request?
This PR is third in series of refactoring and introducing SQL Scripting Execution Framework:
SqlScriptingExecutionContext, object to keep current state of script execution.FramesandScopesto support Local Variables and Error Handlers resolution.SqlScriptingIteratorfromSqlScriptExecution.sql()API.SqlScriptingExecutionNodeSuiteso tests remain independent of concept of Frames and Scopes.First PR
Second PR
Why are the changes needed?
This changes are needed to enable introduction of Error Handling mechanism and Local Variables.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing tests that were updated to support new concepts introduced in this PR.
Was this patch authored or co-authored using generative AI tooling?
No.