Skip to content

Commit 80cd867

Browse files
davidm-dbcloud-fan
authored andcommitted
[SPARK-48353][FOLLOW UP][SQL] Exception handling improvements
### What changes were proposed in this pull request? This pull request proposes an improvement in how conditions are matched with exception handlers. Previously, condition would match with an exception handler only if the match was full/complete - i.e. condition `<MAIN>.<SUBCLASS>` would match only to the handler defined for `<MAIN>.<SUBCLASS>`. With this improvement, `<MAIN>.<SUBCLASS>` condition would match to the handlers defined for `<MAIN>` condition as well, with correct precedence. This pull requests also proposes a number of fixes for different missing things: - `CompoundBodyExec.reset()` is not resetting the `scopeStatus` field. - Exception handler body is never reset (this includes internal iterator reset). This causes issues if the handler is defined in a loop and gets matched multiple times. - When searching for a place to inject `LEAVE` statement, after the `EXIT` handler has been executed, the logic considered only `CompoundBodyExec` nodes, whereas it should have included all non-leaf statements. - Exception handling for exceptions that happen in conditions (for each statement type - if/else, while, case, etc) is not working properly because the injected `LEAVE` statement is not recognized properly. - Exception handling for exceptions that happen in the last statement of the body of if/else and searched case statement is not working properly because the injected `LEAVE` statement is not recognized properly. - `hasNext()` in `ForStatementExec` is executing the query. This causes the issues in cases when `FOR` is the first statement in the compound and the query fails. It means that exception would happen during the `hasNext()` checks instead of the actual iteration. In such case, the parent compound (of the `FOR` statement) is not entered (because that happens during the iteration) and call stack is not properly setup for exception handling. Changes corresponding to this problems, in the same order: - Reset the `scopeStatus` field in `CompoundBodyExec.reset()`. - Call `reset()` on handler before starting its execution in `SqlScriptingExecution.handleException()`. - Move `curr` pointer to `NonLeafStatementExec` and use that in `SqlScriptingExecution. getNextStatement()` in case when `EXIT` handler is finished and removed from the stack. - Add special case handling for `LeaveStatementExec` in `*.Condition` cases in all of the relevant statement types. When exception happens during the condition execution, the `LeaveStatementExec` is injected into `curr` field, but the state hasn't changed. This means that when the `LEAVE` statement is to be executed, the state would correspond to the condition. **I don't know how to do this better, so any suggestions are more than welcome!** - Add special case handling for `LeaveStatementExec` in `*Body` cases of if/else and searched case statement - equivalent to the previous bullet. - Reorder conditions in `ForStatementExec.hasNext()`. ### Why are the changes needed? These changes are fixing wrong logic and improving some of the already existing exception handling mechanisms. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? New unit tests are added for to test/guard all of the introduced improvements. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #51034 from davidm-db/exception_handling_improvements. Authored-by: David Milicevic <david.milicevic@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
1 parent abc72f2 commit 80cd867

File tree

5 files changed

+696
-111
lines changed

5 files changed

+696
-111
lines changed

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/SqlScriptingLogicalPlans.scala

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -292,6 +292,7 @@ case class SimpleCaseStatement(
292292
conditionExpressions: Seq[Expression],
293293
conditionalBodies: Seq[CompoundBody],
294294
elseBody: Option[CompoundBody]) extends CompoundPlanStatement {
295+
assert(conditionExpressions.nonEmpty)
295296
assert(conditionExpressions.length == conditionalBodies.length)
296297

297298
override def output: Seq[Attribute] = Seq.empty

sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingExecution.scala

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,21 @@ class SqlScriptingExecution(
7171
contextManagerHandle.runWith(f)
7272
}
7373

74+
/**
75+
* Helper method to inject leave statement into the execution plan.
76+
* @param executionPlan Execution plan to inject leave statement into.
77+
* @param label Label of the leave statement.
78+
*/
79+
private def injectLeaveStatement(executionPlan: NonLeafStatementExec, label: String): Unit = {
80+
// Go as deep as possible, to find a leaf node. Instead of a statement that
81+
// should be executed next, inject LEAVE statement in its place.
82+
var currExecPlan = executionPlan
83+
while (currExecPlan.curr.exists(_.isInstanceOf[NonLeafStatementExec])) {
84+
currExecPlan = currExecPlan.curr.get.asInstanceOf[NonLeafStatementExec]
85+
}
86+
currExecPlan.curr = Some(new LeaveStatementExec(label))
87+
}
88+
7489
/** Helper method to iterate get next statements from the first available frame. */
7590
private def getNextStatement: Option[CompoundStatementExec] = {
7691
// Remove frames that are already executed.
@@ -94,12 +109,8 @@ class SqlScriptingExecution(
94109
&& lastFrame.scopeLabel.get == context.firstHandlerScopeLabel.get) {
95110
context.firstHandlerScopeLabel = None
96111
}
97-
98-
var execPlan: CompoundBodyExec = context.frames.last.executionPlan
99-
while (execPlan.curr.exists(_.isInstanceOf[CompoundBodyExec])) {
100-
execPlan = execPlan.curr.get.asInstanceOf[CompoundBodyExec]
101-
}
102-
execPlan.curr = Some(new LeaveStatementExec(lastFrame.scopeLabel.get))
112+
// Inject leave statement into the execution plan of the last frame.
113+
injectLeaveStatement(context.frames.last.executionPlan, lastFrame.scopeLabel.get)
103114
}
104115
}
105116
// If there are still frames available, get the next statement.
@@ -164,6 +175,7 @@ class SqlScriptingExecution(
164175
context.frames.append(
165176
handlerFrame
166177
)
178+
handler.reset()
167179
handlerFrame.executionPlan.enterScope()
168180
case None =>
169181
throw e.asInstanceOf[Throwable]

sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionContext.scala

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,15 @@ class SqlScriptingExecutionScope(
205205

206206
errorHandler = triggerToExceptionHandlerMap.getHandlerForCondition(uppercaseCondition)
207207

208+
if (errorHandler.isEmpty) {
209+
if (uppercaseCondition.contains('.')) {
210+
// If the condition contains a dot, it has a main error class and a subclass.
211+
// Check if the error class is defined in the triggerToExceptionHandlerMap.
212+
val errorClass = uppercaseCondition.split('.').head
213+
errorHandler = triggerToExceptionHandlerMap.getHandlerForCondition(errorClass)
214+
}
215+
}
216+
208217
if (errorHandler.isEmpty) {
209218
// Check if there is a specific handler for the given SQLSTATE.
210219
errorHandler = triggerToExceptionHandlerMap.getHandlerForSqlState(uppercaseSqlState)

0 commit comments

Comments
 (0)