[SPARK-48353][SQL] Introduction of Exception Handling mechanism in SQL Scripting #49427

miland-db · 2025-01-09T18:43:04Z

What changes were proposed in this pull request?

This pull request introduces the logic of error handling inside SQL Scripting language. Now, it is possible to:

declare conditions for specific SQL States (currently only valid in scope where they are defined)
declare handlers to catch and process errors that are risen during statement execution

Rules for selecting the most appropriate handler:

Named condition handlers are most specific.
SQLSTATE handlers are next in specificity.
Generic NOT FOUND and SQLEXCEPTION handlers are least specific.

Note: Handlers defined in the innermost compound statement where the exception was raised are considered.

Why are the changes needed?

The intent is to add the possibility for user to handle SQL errors in a custom defined way.

Limitations

Currently, only EXIT handler is supported. Support for CONTINUE handlers will come in the future.
It is only possible to declare condition in its full form by specifying SQLSTATE. Short form with default SQLSTATE is not yet supported.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

There are already existing test suites for SQL scripting that have been improved to test new functionalities:

SqlScriptingParserSuite
SqlScriptingExecutionSuite
SqlScriptingE2eSuite

Was this patch authored or co-authored using generative AI tooling?

No.

…4-condition-handlers

# Conflicts: # common/utils/src/main/resources/error/error-conditions.json # sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingExecution.scala # sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/ThriftServerWithSparkContextSuite.scala

miland-db · 2025-01-14T13:30:16Z

Adding @davidm-db @dejankrak-db @dusantism-db @cloud-fan @srielau

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

cloud-fan · 2025-02-02T13:00:13Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

+    Option(ctx.multipartIdentifier())
+      .foreach { conditionContext =>
+        val conditionNameParts = visitMultipartIdentifier(conditionContext)
+        val conditionNameString = conditionNameParts.mkString(".").toUpperCase(Locale.ROOT)


nit: we only allow one name part, so this can be moved after the length check and do conditionNameParts.head.toUpperCase...

Here we also allow multipart names because of builtin conditions. I agree that we can move toUpperCase after the length check.

oh, I see. Shall we check the user-defined error condition name to not contain special chars (only allow A-Z, digits and underscore)? In case it conflicts with the qualified builtin error condition name.

I will add assertConditionName check in visitDeclareConditionStatementImpl to only allow A-B, digits and underscore.

Anyway, we check if the condition name in handler declaration is either user defined or builtin condition name, so for user defined stuff it is ok to just check it during condition declaration.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

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

cloud-fan · 2025-02-02T13:06:09Z

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

+ */
+case class ErrorCondition(
+    conditionName: String,
+    sqlState: String) extends CompoundPlanStatement {


Does this need to be a CompoundPlanStatement? It seems like an intermediate data structure we use in the parser, and will end up as a Map[String, String] in CompoundBody

Yes, there is no need for this to be CompoundPlanStatement. I will update this.

When I think about this, ErrorCondition is in a way a CompoundPlanStatement because we use it to declare condition and store the details there. In visitCompoundStatementImpl we are returning CompoundPlanStatement and ErrorCondition is one of them (also in the parser rule).

Looking at this, ErrorCondition (or DeclareConditionStatement) can either extend single statement, or we can leave it as it is now. What's your opinion @cloud-fan?

I'm fine to keep it as it is

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

cloud-fan

LGTM except for some nits

miland-db · 2025-02-02T16:09:28Z

@cloud-fan Thank you for review(s)!

davidm-db · 2025-02-03T08:47:49Z

@cloud-fan Please don't merge until I take one last look, I was doing something locally last night and I just need to check first if the comments I had are applicable to this PR. I'll let you know once I'm done.

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

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

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

davidm-db · 2025-02-03T10:15:35Z

Minor comments, otherwise LGTM.
I have one concern regarding all the case insensitive checks in the code - for example, in SqlScriptingLabelContext.checkLabels we are doing conversion to lowercase in the place of a check, rather than relying on propagating uppercase/lowercase strings and then having them in the correct case at the time of a check (which we are doing here). The approach we have with labels seems more robust and localized.
Here (in visitCompoundBodyImpl, for example, instead of propagating uppercase conditions, we would propagate just the input values, and then when adding to conditions map we can do a lowercase conversion and then each time we check if the condition is in the map, we do the conversion as well. This way, it's localized to the place where we do the checks and if something changes in the future we don't need to rely that someone will take care of this, it would simply work.
Thoughts?

miland-db · 2025-02-03T10:38:36Z

Basically, we are doing uniqueness check for condition names in ExceptionHandlerTriggers.addUnique* methods where case sensitivity is centralized and strings are converted to the same casing .
When creating ErrorCondition object, we pass upper-case arguments.
We use upper-case when we check if condition is builtin Spark condition.

I agree more centralized approach should be used, but with current tests, I think we covered all possible checks in code to be case-insensitive. As a follow up, I will try to think of some more clever way of ensuring case-insensitivity because this can be a problem if somebody change or add new code without case-sensitivity in mind. Thanks for the comment!

cloud-fan · 2025-02-04T04:51:33Z

thanks, merging to master/4.0!

…L Scripting ### What changes were proposed in this pull request? This pull request introduces the logic of error handling inside SQL Scripting language. Now, it is possible to: - declare conditions for specific SQL States (currently only valid in scope where they are defined) - declare handlers to catch and process errors that are risen during statement execution Rules for selecting the most appropriate handler: - Named condition handlers are most specific. - SQLSTATE handlers are next in specificity. - Generic NOT FOUND and SQLEXCEPTION handlers are least specific. Note: Handlers defined in the innermost compound statement where the exception was raised are considered. ### Why are the changes needed? The intent is to add the possibility for user to handle SQL errors in a custom defined way. ### Limitations - Currently, only `EXIT` handler is supported. Support for `CONTINUE` handlers will come in the future. - It is only possible to declare condition in its full form by specifying SQLSTATE. Short form with default SQLSTATE is not yet supported. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? There are already existing test suites for SQL scripting that have been improved to test new functionalities: - `SqlScriptingParserSuite` - `SqlScriptingExecutionSuite` - `SqlScriptingE2eSuite` ### Was this patch authored or co-authored using generative AI tooling? No. Closes #49427 from miland-db/milan-dankovic_data/refactor-execution-4-condition-handlers. Authored-by: Milan Dankovic <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 0f163a5) Signed-off-by: Wenchen Fan <[email protected]>

LuciferYang · 2025-02-06T08:09:15Z

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

+      parameters = Map("sqlState" -> "12345"))
+  }
+
+  test("Specific condition takes precedence over sqlState") {


There are 13 new tests in SqlScriptingExecutionSuiteandSqlScriptingE2eSuite` will fail to execute in non-ANSI mode and the NON-ANSI daily test failed:

https://github.com/apache/spark/actions/runs/13148342209/job/36691119394

we can reproduce it using the following command: SPARK_ANSI_SQL_MODE=false build/sbt "sql/testOnly org.apache.spark.sql.scripting.SqlScriptingE2eSuite org.apache.spark.sql.scripting.SqlScriptingExecutionSuite"

Do you have time to fix this issue? @miland-db

I will fix this. The issue is that zero division is not throwing an error when SPARK_ANSI_SQL_MODE=false. The solution can be to override this config for this suite, or to find some other error that has the same behavior regardless of the ANSI. Any thoughts @LuciferYang?

let's just enable ansi for the entire suite, by overriding def sparkConf. example:

class AvroV1Suite extends AvroSuite { override protected def sparkConf: SparkConf = super .sparkConf .set(SQLConf.USE_V1_SOURCE_LIST, "avro")

let's just enable ansi for the entire suite, by overriding def sparkConf. example:

class AvroV1Suite extends AvroSuite { override protected def sparkConf: SparkConf = super .sparkConf .set(SQLConf.USE_V1_SOURCE_LIST, "avro")

If the 13 failed cases are all due to the same reason, I think it's ok

…suites ### What changes were proposed in this pull request? Keep ANSI enabled for SQL Scripting execution suites to enable proper testing of exception handling. ### Why are the changes needed? 13 new tests in `SqlScriptingExecutionSuite` and `SqlScriptingE2eSuite` introduced in #49427 will fail to execute in non-ANSI mode and the NON-ANSI daily test failed: ![image](https://github.com/user-attachments/assets/78a0b6fd-1b98-4b85-80e5-688e4b447cae) If ANSI is off, code that should trigger exception fails silently (zero division does not throw). We want to keep ANSI behavior so that we can test exception handling properly. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing tests. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #49852 from miland-db/milan-dankovic_data/exception-handlers-followup-fix-test. Authored-by: Milan Dankovic <[email protected]> Signed-off-by: Max Gekk <[email protected]>

…suites ### What changes were proposed in this pull request? Keep ANSI enabled for SQL Scripting execution suites to enable proper testing of exception handling. ### Why are the changes needed? 13 new tests in `SqlScriptingExecutionSuite` and `SqlScriptingE2eSuite` introduced in #49427 will fail to execute in non-ANSI mode and the NON-ANSI daily test failed: ![image](https://github.com/user-attachments/assets/78a0b6fd-1b98-4b85-80e5-688e4b447cae) If ANSI is off, code that should trigger exception fails silently (zero division does not throw). We want to keep ANSI behavior so that we can test exception handling properly. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing tests. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #49852 from miland-db/milan-dankovic_data/exception-handlers-followup-fix-test. Authored-by: Milan Dankovic <[email protected]> Signed-off-by: Max Gekk <[email protected]> (cherry picked from commit ad13a88) Signed-off-by: Max Gekk <[email protected]>

miland-db added 14 commits December 24, 2024 18:34

Minor improvements

b3fdd8c

Merge branch 'master' into refactor-execution-3-followup

9b47d09

Replace noop with collect

4c3c142

Merge branch 'master' into refactor-execution-3-followup

90f08cc

Add new keywords and grammar rules to support handlers

3ad692d

Replace addOne with append

f6741a5

Add visit methods in AstBuilder

4b1e7d1

Add comments to explain SSE iteration logic and contract

a98302e

Add checks for sqlState

2a71714

Remove iterator interface from SqlScriptingExecution

dca7fd9

Merge branch 'refactor-execution-3-followup' into refactor-execution-…

94459b7

…4-condition-handlers

getNextResult is now calling itself when an error happens

edc1115

Add new keywords to sql-ref-ansi-comilance

325f3e6

Add parser tests

039b807

miland-db marked this pull request as draft January 9, 2025 18:43

github-actions bot added SQL DOCS labels Jan 9, 2025

miland-db added 4 commits January 9, 2025 19:52

Add keywords to thrift teswt suite

cebe2c3

Add new error-conditions

0f5cb11

Add frame types

5a0024c

miland-db changed the title ~~[SPARK-48353][SQL] Introduction of Error Handling mechanism GRAMMAR in SQL Scripting~~ [SPARK-48353][SQL] Introduction of Error Handling mechanism in SQL Scripting Jan 13, 2025

miland-db added 6 commits January 13, 2025 17:14

Propagate frame type everywhere

98d9c3c

Remove duplicate test

1cb2f94

Add error handling execution logic

4ccc9d4

Add tests and improved logic for choosing handler

aacb250

Regenerate golden files for SQL keywords

533fc71

Add tests and new error for continue handler

b0c235c

miland-db marked this pull request as ready for review January 14, 2025 13:20

miland-db added 3 commits January 30, 2025 15:40

Exit the script first frame after executing all statements

becb6cf

Fix private[scripting] and comments

8530371

Fix ThriftServer suite

85d668e

cloud-fan reviewed Feb 2, 2025

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Feb 2, 2025

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Feb 2, 2025

View reviewed changes

...st/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/SqlScriptingLogicalPlans.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Feb 2, 2025

View reviewed changes

...st/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/SqlScriptingLogicalPlans.scala Outdated Show resolved Hide resolved

cloud-fan approved these changes Feb 2, 2025

View reviewed changes

Address comments

5725284