Skip to content

Commit e312318

Browse files
TeodorDjelicgengliangwang
authored andcommitted
[SPARK-53621][CORE][FOLLOWUP] Adding Comments And Enriching Tests of Continue Handler
### What changes were proposed in this pull request? This is a follow up PR in which are added: - Additional comments to further explain the implementation - Expand the existing tests to increase the code coverage by adding branches to conditional statements. - Remove duplicate tests ### Why are the changes needed? - Parts of the initial PR were left unclear and undocumented, and this PR serves to help describe what the code does. - Better test coverage is needed to ensure maintaining correctness. - Duplicate of the same test has no reason to exist. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? This change only introduces test changes and comments, so it was tested only by running the tests. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #52508 from TeodorDjelic/continue-handler-enriching-tests-and-adding-comments. Authored-by: Teodor Djelic <[email protected]> Signed-off-by: Gengliang Wang <[email protected]>
1 parent be1de5b commit e312318

File tree

5 files changed

+32
-68
lines changed

5 files changed

+32
-68
lines changed

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ class SqlScriptingExecution(
122122

123123
context.frames.remove(context.frames.size - 1)
124124

125-
// If the last frame is a handler, set leave statement to be the next one in the
125+
// If the last frame is an EXIT handler, set leave statement to be the next one in the
126126
// innermost scope that should be exited.
127127
if (lastFrame.frameType == SqlScriptingFrameType.EXIT_HANDLER
128128
&& context.frames.nonEmpty) {
@@ -136,6 +136,8 @@ class SqlScriptingExecution(
136136
injectLeaveStatement(context.frames.last.executionPlan, lastFrame.scopeLabel.get)
137137
}
138138

139+
// If the last frame is a CONTINUE handler, leave the handler without injecting anything, but
140+
// skip the conditional statement if the exception originated from its conditional expression.
139141
if (lastFrame.frameType == SqlScriptingFrameType.CONTINUE_HANDLER
140142
&& context.frames.nonEmpty) {
141143
// Remove the scope if handler is executed.

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,8 @@ object SqlScriptingFrameType extends Enumeration {
9494
* @param executionPlan CompoundBody which need to be executed.
9595
* @param frameType Type of the frame.
9696
* @param scopeLabel Label of the scope where handler is defined.
97-
* Available only for frameType = HANDLER.
97+
* Available only for frameType = EXIT_HANDLER, frameType = CONTINUE_HANDLER
98+
* and frameType = SQL_STORED_PROCEDURE.
9899
*/
99100
class SqlScriptingExecutionFrame(
100101
val executionPlan: CompoundBodyExec,

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,15 @@ trait NonLeafStatementExec extends CompoundStatementExec {
110110
* Conditional node in the execution tree. It is a conditional non-leaf node.
111111
*/
112112
trait ConditionalStatementExec extends NonLeafStatementExec {
113+
/**
114+
* Interrupted flag indicates if the statement has been interrupted, and is used
115+
* for skipping the execution of the conditional statements, by setting the hasNext
116+
* to be false.
117+
* Interrupt is issued by the CONTINUE HANDLER when the conditional statement's
118+
* conditional expression throws an exception, and is issued by the Leave Statement
119+
* when the ForStatementExec executes the Leave Statement injected by the EXIT
120+
* HANDLER.
121+
*/
113122
protected[scripting] var interrupted: Boolean = false
114123
}
115124

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -87,11 +87,11 @@ case class SqlScriptingInterpreter(session: SparkSession) {
8787
args,
8888
context)
8989

90-
// Scope label should be Some(compoundBody.label.get) for both handler types
9190
val handlerExec = new ExceptionHandlerExec(
92-
handlerBodyExec,
93-
handler.handlerType,
94-
Some(compoundBody.label.get))
91+
body = handlerBodyExec,
92+
handlerType = handler.handlerType,
93+
// Used as a parent label of where the handler is declared (label of compoundBody used).
94+
scopeLabel = Some(compoundBody.label.get))
9595

9696
// For each condition handler is defined for, add corresponding key value pair
9797
// to the conditionHandlerMap.

sql/core/src/test/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionSuite.scala

Lines changed: 14 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -402,68 +402,6 @@ class SqlScriptingExecutionSuite extends QueryTest with SharedSparkSession {
402402
verifySqlScriptResult(sqlScript, expected = expected)
403403
}
404404

405-
test("handler - exit resolve in the same block when if condition fails") {
406-
val sqlScript =
407-
"""
408-
|BEGIN
409-
| DECLARE VARIABLE flag INT = -1;
410-
| scope_to_exit: BEGIN
411-
| DECLARE EXIT HANDLER FOR SQLSTATE '22012'
412-
| BEGIN
413-
| SELECT flag;
414-
| SET flag = 1;
415-
| END;
416-
| SELECT 2;
417-
| SELECT 3;
418-
| IF 1 > 1/0 THEN
419-
| SELECT 10;
420-
| END IF;
421-
| SELECT 4;
422-
| SELECT 5;
423-
| END;
424-
| SELECT flag;
425-
|END
426-
|""".stripMargin
427-
val expected = Seq(
428-
Seq(Row(2)), // select
429-
Seq(Row(3)), // select
430-
Seq(Row(-1)), // select flag
431-
Seq(Row(1)) // select flag from the outer body
432-
)
433-
verifySqlScriptResult(sqlScript, expected = expected)
434-
}
435-
436-
test("continue handler - continue after the if statement when if condition fails") {
437-
val sqlScript =
438-
"""
439-
|BEGIN
440-
| DECLARE VARIABLE flag INT = -1;
441-
| DECLARE CONTINUE HANDLER FOR DIVIDE_BY_ZERO
442-
| BEGIN
443-
| SELECT flag;
444-
| SET flag = 1;
445-
| END;
446-
| SELECT 2;
447-
| SELECT 3;
448-
| IF (1 > 1/0) THEN
449-
| SELECT 4;
450-
| END IF;
451-
| SELECT 5;
452-
| SELECT 6;
453-
| SELECT flag;
454-
|END
455-
|""".stripMargin
456-
val expected = Seq(
457-
Seq(Row(2)), // select
458-
Seq(Row(3)), // select
459-
Seq(Row(-1)), // select flag
460-
Seq(Row(5)), // select
461-
Seq(Row(6)), // select
462-
Seq(Row(1)) // select flag from the outer body
463-
)
464-
verifySqlScriptResult(sqlScript, expected = expected)
465-
}
466-
467405
test("handler - exit resolve in outer block") {
468406
val sqlScript =
469407
"""
@@ -956,6 +894,10 @@ class SqlScriptingExecutionSuite extends QueryTest with SharedSparkSession {
956894
| END;
957895
| IF 1 > 1/0 THEN
958896
| SELECT 10;
897+
| ELSEIF (1 = 1) THEN
898+
| SELECT 11;
899+
| ELSE
900+
| SELECT 12;
959901
| END IF;
960902
| SELECT 4;
961903
| SELECT 5;
@@ -1012,6 +954,8 @@ class SqlScriptingExecutionSuite extends QueryTest with SharedSparkSession {
1012954
| END;
1013955
| CASE 1/0
1014956
| WHEN flag THEN SELECT 10;
957+
| WHEN 1 THEN SELECT 11;
958+
| ELSE SELECT 12;
1015959
| END CASE;
1016960
| SELECT 4;
1017961
| SELECT 5;
@@ -1068,6 +1012,9 @@ class SqlScriptingExecutionSuite extends QueryTest with SharedSparkSession {
10681012
| END;
10691013
| CASE flag
10701014
| WHEN 1/0 THEN SELECT 10;
1015+
| WHEN -1 THEN SELECT 11;
1016+
| WHEN 1 THEN SELECT 12;
1017+
| ELSE SELECT 13;
10711018
| END CASE;
10721019
| SELECT 4;
10731020
| SELECT 5;
@@ -1124,6 +1071,9 @@ class SqlScriptingExecutionSuite extends QueryTest with SharedSparkSession {
11241071
| END;
11251072
| CASE flag
11261073
| WHEN 'teststr' THEN SELECT 10;
1074+
| WHEN -1 THEN SELECT 11;
1075+
| WHEN 1 THEN SELECT 12;
1076+
| ELSE SELECT 13;
11271077
| END CASE;
11281078
| SELECT 4;
11291079
| SELECT 5;
@@ -1180,6 +1130,8 @@ class SqlScriptingExecutionSuite extends QueryTest with SharedSparkSession {
11801130
| END;
11811131
| CASE
11821132
| WHEN flag = 1/0 THEN SELECT 10;
1133+
| WHEN 1 = 1 THEN SELECT 11;
1134+
| ELSE SELECT 12;
11831135
| END CASE;
11841136
| SELECT 4;
11851137
| SELECT 5;

0 commit comments

Comments
 (0)