[SPARK-48375][SQL] Add support for SIGNAL statement #49726

miland-db · 2025-01-29T16:17:10Z

What changes were proposed in this pull request?

SIGNAL statement to enable users to raise an error from arbitrary place in code.

Why are the changes needed?

The intent is to add the possibility for user to raise builtin and user defined conditions/sqlstates. This will enable users to do error handling in a way similar to other popular programming languages.

Limitations

It is only possible to raise errors with user defined conditions declared in the same scope as signal statement.

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

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

No

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

sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingRuntimeException.scala

davidm-db · 2025-02-02T12:40:56Z

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

+      sqlState = sqlState,
+      message = getMessageText,
+      cause = null,
+      origin = CurrentOrigin.get


this ain't right, will update the comment tomorrow, writing just so it doesn't get forgotten

# Conflicts: # common/utils/src/main/resources/error/error-conditions.json # sql/api/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4 # sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala # sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/SqlScriptingLogicalPlans.scala # sql/catalyst/src/main/scala/org/apache/spark/sql/errors/SqlScriptingErrors.scala # sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/SqlScriptingParserSuite.scala # sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingExecution.scala # sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingExecutionContext.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/SqlScriptingExecutionSuite.scala # sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/ThriftServerWithSparkContextSuite.scala

miland-db · 2025-02-05T17:22:58Z

Adding reviewers: @cloud-fan, @davidm-db, @dejankrak-db, @dusantism-db, @MaxGekk

cloud-fan · 2025-02-06T08:33:36Z

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

+case class SignalStatement(
+    var isBuiltinError: Boolean = false,
+    errorCondition: Option[String] = None,
+    var sqlState: Option[String] = None,


why do we put mutable var in a case class?

sqlState is resolved later for some kind of SIGNAL statements and not during parsing. I need a way to set it when I resolve it. One way is to create a new instance where we copy everything from the existing instance and provide new resolved sqlState, or to use var. Any thoughts @cloud-fan?

creating new instance is more scala-style and let's do it

cloud-fan · 2025-02-06T08:34:56Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/SqlScriptingParserSuite.scala

@@ -2413,6 +2413,21 @@ class SqlScriptingParserSuite extends SparkFunSuite with SQLHelper {
      head.asInstanceOf[SingleStatement].getText == "SELECT 3")
  }

+  test("signal statement - invalid sqlstate") {


shall we add some positive tests?

I will add additional parser and execution tests for SIGNAL.

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

cloud-fan · 2025-02-06T08:38:09Z

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

+    val errorCondition: Option[String] = None,
+    val sqlState: Option[String] = None,
+    val message: Either[String, UnresolvedAttribute],
+    val msgArguments: Option[UnresolvedAttribute],


shall we allow users to specify parameters like the TBLPROPERTIES syntax?

@srielau suggested that for the first version providing the variable that holds the map of arguments is enough. Next steps will be to add support for other ways to specify message arguments, e.g. directly using map(...) function etc.

MaxGekk · 2025-02-06T10:38:31Z

common/utils/src/main/resources/error/error-conditions.json

@@ -3602,6 +3602,12 @@
    ],
    "sqlState" : "42K09"
  },
+  "INVALID_VARIABLE_TYPE_FOR_SIGNAL_STATEMENT" : {
+    "message" : [
+      "Variable type must be Map<String, String> type but got <varType>."


Could you output Map<String, String> in the same way as toSQLType

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

davidm-db · 2025-02-07T10:47:54Z

sql/api/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4

@@ -65,6 +65,7 @@ compoundStatement
    | beginEndCompoundBlock
    | declareConditionStatement


[Can be done in a follow-up]

We should fix the DECLARE CONDITION statement. The simple case when we declare an "empty" condition, like DECLARE cond CONDITION; fails currently - CONDITION is recognized as a type and an exception is thrown that the type is not recognized.
I think the simplest (and best) way to do this is to reorder the parser rules and add a comment that declareConditionStatement needs to go before statement. I think the parser already relies on the rules order in multiple places and that this should be fine. So, I think we should go with this approach, but add a comment in a PR description about other possible approaches that we discussed offline.
Let's add the tests for this case as well.

davidm-db · 2025-02-07T10:49:33Z

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

+
+    SignalStatement(
+      errorCondition = Some(ctx.conditionName.getText.toUpperCase(Locale.ROOT)),
+      sqlState = None,


Let's add a comment that explains that we set the sqlState to None here and fill it in in visitCompoundBodyImpl once all the conditions have been resolved.

davidm-db · 2025-02-07T10:50:14Z

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

+          // Try to get sqlState if condition is user defined.
+          signalStatement.sqlState = conditions.get(signalStatement.errorCondition.get)
+
+          // If condition is not user defined, check if it is a spark defined error condition.
+          if (signalStatement.sqlState.isEmpty) {


Also, a small comment explaining why we have this logic of filling in sqlState here would be nice.

davidm-db · 2025-02-07T10:57:02Z

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

+  override def visitSignalStatementWithCondition(
+      ctx: SignalStatementWithConditionContext): SignalStatement = {
+    val messageString = Option(ctx.msgStr)
+      .map(sl => Left(string(visitStringLit(sl)).replace("'", "")))


We are a bit inconsistent with visiting string literals in visitSignalStatementWithCondition and visitSignalStatementWithSqlState - let's double check what is the easiest/most compact way to do this. Also, do we need to replace quotes?

davidm-db · 2025-02-07T10:58:24Z

sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingRuntimeException.scala

+      messageParameters,
+      isBuiltinError),
+    cause)
+    with SparkThrowable {


nit: indentation, should be the same level as extends

davidm-db · 2025-02-07T11:03:43Z

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

+    errorCondition: Option[String] = None,
+    var sqlState: Option[String] = None,


it's fine to keep both errorCondition and sqlState as Options in the logical plan (due to the resolution in the visitors), but since we need to have both filled in, I would remove the Options from the exec node and add assert in the interpreter's bulidExecutionPlan

davidm-db · 2025-02-07T11:09:09Z

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

+        val argsValue = argsReference.eval(null)
+
+        if (argsValue == null) {
+          throw SqlScriptingErrors.nullVariableSignalStatement(CurrentOrigin.get, args.name)


cannot do CurrentOrigin.get here as well, we have the origin as the param of SingleStatementExec

davidm-db · 2025-02-07T11:09:26Z

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

+
+        if (varReferenceValue == null) {
+          throw SqlScriptingErrors
+            .nullVariableSignalStatement(CurrentOrigin.get, u.name)


same thing for CurrentOrigin.get

davidm-db · 2025-02-07T11:10:13Z

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

+
+        if (!argsReference.dataType.sameType(MapType(StringType, StringType))) {
+          throw SqlScriptingErrors
+            .invalidSignalStatementVariableType(CurrentOrigin.get, argsReference.dataType)


replace CurrentOrigin.get here as well

davidm-db · 2025-02-07T11:10:20Z

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

+
+        if (!varReference.dataType.sameType(StringType)) {
+          throw SqlScriptingErrors
+            .invalidSignalStatementVariableType(CurrentOrigin.get, varReference.dataType)


replace CurrentOrigin.get here as well

davidm-db · 2025-02-07T11:16:24Z

sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingRuntimeException.scala

+     messageParameters: Map[String, String] = Map.empty,
+     isBuiltinError: Boolean = false)
+  extends Exception(
+    errorMessageWithLineNumber(


We are not adding SQL State to the exception message and I think we should do it.
The regular exceptions are in format [<sql_state>] <message_text>. For SqlScriptingException we did {<error_line>} [<sql_state>] <message_text> and we should do the same for SqlScriptingRuntimeException

just noticed that it looks like this is fixed in SqlScriptingRuntimeException.errorMessageWithLineNumber?

davidm-db · 2025-02-07T11:18:07Z

sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingRuntimeException.scala

+     condition: String,
+     sqlState: Option[String] = None,
+     message: String,
+     cause: Throwable,
+     val origin: Origin,
+     messageParameters: Map[String, String] = Map.empty,
+     isBuiltinError: Boolean = false)


nit: indentation seems off here

davidm-db · 2025-02-07T11:23:34Z

sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingRuntimeException.scala

+import org.apache.spark.sql.catalyst.trees.Origin
+import org.apache.spark.sql.exceptions.SqlScriptingRuntimeException.errorMessageWithLineNumber
+
+class SqlScriptingRuntimeException (


[Can be done in a follow-up PR - together with the other related comment in SqlScriptingExecution]

nit: maybe adding a comment explaining what is this exception used for (wrapping all of the SparkThrowable exceptions thrown from the script into this, etc) would be nice

davidm-db · 2025-02-07T11:32:23Z

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

@@ -100,13 +100,13 @@ class SqlScriptingExecution(
    // While we don't have a result statement, execute the statements.
    while (currentStatement.isDefined) {


[Can be done in a follow-up PR]

In handleException we should add logic that converts all of the SparkThrowable exceptions to the SqlScriptingRuntimeException.

This has a few consequences:

We need to start propagating the exception to findHandler functions instead of condition and SQL state.

We need to start tracking the currentStatementOrigin for each statement here.

Propagate currentStatementOrigin to handle exception in order to properly construct SqlScriptingRuntimeException.

withErrorHandling has to have Origin as an input param as well.

I've sent you the example commit offline regarding this.

davidm-db · 2025-02-07T11:37:43Z

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

+    val session: SparkSession,
+    override val origin: Origin)
+  extends LeafStatementExec
+    with ColumnResolutionHelper with WithOrigin {


nit: indentation should match the extends

davidm-db · 2025-02-07T11:41:27Z

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

+ * Executable node for Signal Statement.
+ * @param errorCondition Name of the error condition/SQL State for error that will be thrown.
+ * @param sqlState SQL State of the error that will be thrown.
+ * @param message Error message (either string or variable name).
+ * @param msgArguments Error message parameters for builtin conditions.
+ * @param session Spark session that SQL script is executed within.


nit: missing some of the params in the comment

davidm-db · 2025-02-07T11:50:44Z

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

+    val errorCondition: Option[String] = None,
+    val sqlState: Option[String] = None,


as mentioned in one of the previous comments, let's make these mandatory

davidm-db · 2025-02-07T11:51:16Z

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

@@ -274,6 +274,16 @@ case class SqlScriptingInterpreter(session: SparkSession) {
      case iterateStatement: IterateStatement =>
        new IterateStatementExec(iterateStatement.label)

+      case signalStatement: SignalStatement =>
+        new SignalStatementExec(


as mentioned in one of the previous comments, let's add assert that errorCondition and sqlState are non-empty in signalStatement

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

miland-db marked this pull request as draft January 29, 2025 16:17

davidm-db reviewed Jan 30, 2025

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/scripting/SqlScriptingExecution.scala Outdated Show resolved Hide resolved

davidm-db reviewed Jan 30, 2025

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/exceptions/SqlScriptingRuntimeException.scala Outdated Show resolved Hide resolved

HANDLERS + SIGNAL

ca6f439

miland-db force-pushed the milan-dankovic_data/signal-statement-1 branch from 3e7ab54 to ca6f439 Compare January 31, 2025 13:09

davidm-db reviewed Feb 2, 2025

View reviewed changes

miland-db added 2 commits February 4, 2025 11:28

Sync with the latest master

e539198

github-actions bot removed the DOCS label Feb 4, 2025

Add message params

98b1611

github-actions bot added the DOCS label Feb 4, 2025

miland-db added 2 commits February 5, 2025 16:46

Fix keywords and add origin to signal statement

7be82ae

Add tests with signal and handler together

123d151

miland-db changed the title ~~[DRAFT][SPARK-48375][SQL] Add support for SIGNAL statement~~ [SPARK-48375][SQL] Add support for SIGNAL statement Feb 5, 2025

miland-db marked this pull request as ready for review February 5, 2025 17:16

Fix parser error

7868040

cloud-fan reviewed Feb 6, 2025

View reviewed changes

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

cloud-fan reviewed Feb 6, 2025

View reviewed changes

MaxGekk reviewed Feb 6, 2025

View reviewed changes

davidm-db reviewed Feb 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-48375][SQL] Add support for SIGNAL statement #49726

[SPARK-48375][SQL] Add support for SIGNAL statement #49726

miland-db commented Jan 29, 2025 •

edited

Loading

davidm-db Feb 2, 2025

miland-db commented Feb 5, 2025

cloud-fan Feb 6, 2025

miland-db Feb 6, 2025

cloud-fan Feb 6, 2025

cloud-fan Feb 6, 2025

miland-db Feb 6, 2025

cloud-fan Feb 6, 2025

miland-db Feb 6, 2025

MaxGekk Feb 6, 2025

davidm-db Feb 7, 2025

davidm-db Feb 7, 2025

davidm-db Feb 7, 2025

davidm-db Feb 7, 2025

davidm-db Feb 7, 2025

davidm-db Feb 7, 2025

davidm-db Feb 7, 2025

davidm-db Feb 7, 2025

davidm-db Feb 7, 2025

davidm-db Feb 7, 2025

davidm-db Feb 7, 2025

davidm-db Feb 7, 2025

davidm-db Feb 7, 2025

davidm-db Feb 7, 2025

davidm-db Feb 7, 2025

davidm-db Feb 7, 2025

davidm-db Feb 7, 2025

davidm-db Feb 7, 2025

davidm-db Feb 7, 2025

		@@ -65,6 +65,7 @@ compoundStatement
		\| beginEndCompoundBlock
		\| declareConditionStatement

		errorCondition: Option[String] = None,
		var sqlState: Option[String] = None,

		@@ -100,13 +100,13 @@ class SqlScriptingExecution(
		// While we don't have a result statement, execute the statements.
		while (currentStatement.isDefined) {

		val errorCondition: Option[String] = None,
		val sqlState: Option[String] = None,

[SPARK-48375][SQL] Add support for SIGNAL statement #49726

Are you sure you want to change the base?

[SPARK-48375][SQL] Add support for SIGNAL statement #49726

Conversation

miland-db commented Jan 29, 2025 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Limitations

Does this PR introduce any user-facing change?

How was this patch tested?

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

Choose a reason for hiding this comment

miland-db commented Feb 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miland-db commented Jan 29, 2025 •

edited

Loading