This is an automated email from the ASF dual-hosted git repository.
maxgekk pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/spark.git
The following commit(s) were added to refs/heads/master by this push:
new b05c61266d83 [SPARK-46490][SQL] Require error classes in
`SparkThrowable` sub-classes
b05c61266d83 is described below
commit b05c61266d83590dcec642ecae929d6529b0ad1d
Author: Max Gekk <max.g...@gmail.com>
AuthorDate: Sat Dec 30 12:28:23 2023 +0300
[SPARK-46490][SQL] Require error classes in `SparkThrowable` sub-classes
### What changes were proposed in this pull request?
In the PR, I propose to create `SparkThrowable` sub-classes only with an
error class by making the constructor with `message` private.
### Why are the changes needed?
To improve user experience with Spark SQL by unifying error exceptions: the
final goal is all Spark exception should contain an error class.
### Does this PR introduce _any_ user-facing change?
No since user's code shouldn't throw `SparkThrowable` sub-classes but it
can if it depends on error message formats.
### How was this patch tested?
By existing test test suites like:
```
$ PYSPARK_PYTHON=python3 build/sbt "sql/testOnly
org.apache.spark.sql.SQLQueryTestSuite"
```
### Was this patch authored or co-authored using generative AI tooling?
No.
Closes #44464 from MaxGekk/ban-messages-SparkThrowable-subclass.
Authored-by: Max Gekk <max.g...@gmail.com>
Signed-off-by: Max Gekk <max.g...@gmail.com>
---
.../src/main/resources/error/error-classes.json | 30 ++++++
.../scala/org/apache/spark/SparkException.scala | 112 ++++++---------------
.../connect/client/SparkConnectClientSuite.scala | 47 +++++----
.../connect/client/GrpcExceptionConverter.scala | 88 ++++++++--------
4 files changed, 135 insertions(+), 142 deletions(-)
diff --git a/common/utils/src/main/resources/error/error-classes.json
b/common/utils/src/main/resources/error/error-classes.json
index 9f68d4c5a53e..4f34ca29ea65 100644
--- a/common/utils/src/main/resources/error/error-classes.json
+++ b/common/utils/src/main/resources/error/error-classes.json
@@ -7073,6 +7073,36 @@
"Namespace '<namespace>' is non empty. <details>"
]
},
+ "_LEGACY_ERROR_TEMP_3104" : {
+ "message" : [
+ "<message>"
+ ]
+ },
+ "_LEGACY_ERROR_TEMP_3105" : {
+ "message" : [
+ "<message>"
+ ]
+ },
+ "_LEGACY_ERROR_TEMP_3106" : {
+ "message" : [
+ "<message>"
+ ]
+ },
+ "_LEGACY_ERROR_TEMP_3107" : {
+ "message" : [
+ "<message>"
+ ]
+ },
+ "_LEGACY_ERROR_TEMP_3108" : {
+ "message" : [
+ "<message>"
+ ]
+ },
+ "_LEGACY_ERROR_TEMP_3109" : {
+ "message" : [
+ "<message>"
+ ]
+ },
"_LEGACY_ERROR_USER_RAISED_EXCEPTION" : {
"message" : [
"<errorMessage>"
diff --git a/common/utils/src/main/scala/org/apache/spark/SparkException.scala
b/common/utils/src/main/scala/org/apache/spark/SparkException.scala
index 3bcdd0a7c29b..d2a1c6727730 100644
--- a/common/utils/src/main/scala/org/apache/spark/SparkException.scala
+++ b/common/utils/src/main/scala/org/apache/spark/SparkException.scala
@@ -133,11 +133,11 @@ private[spark] case class ExecutorDeadException(message:
String)
/**
* Exception thrown when Spark returns different result after upgrading to a
new version.
*/
-private[spark] class SparkUpgradeException(
- message: String,
- cause: Option[Throwable],
- errorClass: Option[String],
- messageParameters: Map[String, String])
+private[spark] class SparkUpgradeException private(
+ message: String,
+ cause: Option[Throwable],
+ errorClass: Option[String],
+ messageParameters: Map[String, String])
extends RuntimeException(message, cause.orNull) with SparkThrowable {
def this(
@@ -152,15 +152,6 @@ private[spark] class SparkUpgradeException(
)
}
- def this(message: String, cause: Option[Throwable]) = {
- this(
- message,
- cause = cause,
- errorClass = None,
- messageParameters = Map.empty
- )
- }
-
override def getMessageParameters: java.util.Map[String, String] =
messageParameters.asJava
override def getErrorClass: String = errorClass.orNull
@@ -169,7 +160,7 @@ private[spark] class SparkUpgradeException(
/**
* Arithmetic exception thrown from Spark with an error class.
*/
-private[spark] class SparkArithmeticException(
+private[spark] class SparkArithmeticException private(
message: String,
errorClass: Option[String],
messageParameters: Map[String, String],
@@ -189,14 +180,10 @@ private[spark] class SparkArithmeticException(
)
}
- def this(message: String) = {
- this(
- message,
- errorClass = None,
- messageParameters = Map.empty,
- context = Array.empty
- )
- }
+ def this(
+ errorClass: String,
+ messageParameters: Map[String, String],
+ context: Array[QueryContext]) = this(errorClass, messageParameters,
context, "")
override def getMessageParameters: java.util.Map[String, String] =
messageParameters.asJava
@@ -207,7 +194,7 @@ private[spark] class SparkArithmeticException(
/**
* Unsupported operation exception thrown from Spark with an error class.
*/
-private[spark] class SparkUnsupportedOperationException(
+private[spark] class SparkUnsupportedOperationException private(
message: String,
errorClass: Option[String],
messageParameters: Map[String, String])
@@ -223,14 +210,6 @@ private[spark] class SparkUnsupportedOperationException(
)
}
- def this(message: String) = {
- this(
- message,
- errorClass = None,
- messageParameters = Map.empty
- )
- }
-
override def getMessageParameters: java.util.Map[String, String] =
messageParameters.asJava
override def getErrorClass: String = errorClass.orNull
@@ -271,7 +250,7 @@ private[spark] class SparkConcurrentModificationException(
/**
* Datetime exception thrown from Spark with an error class.
*/
-private[spark] class SparkDateTimeException(
+private[spark] class SparkDateTimeException private(
message: String,
errorClass: Option[String],
messageParameters: Map[String, String],
@@ -291,14 +270,10 @@ private[spark] class SparkDateTimeException(
)
}
- def this(message: String) = {
- this(
- message,
- errorClass = None,
- messageParameters = Map.empty,
- context = Array.empty
- )
- }
+ def this(
+ errorClass: String,
+ messageParameters: Map[String, String],
+ context: Array[QueryContext]) = this(errorClass, messageParameters,
context, "")
override def getMessageParameters: java.util.Map[String, String] =
messageParameters.asJava
@@ -324,7 +299,7 @@ private[spark] class SparkFileNotFoundException(
/**
* Number format exception thrown from Spark with an error class.
*/
-private[spark] class SparkNumberFormatException private[spark](
+private[spark] class SparkNumberFormatException private(
message: String,
errorClass: Option[String],
messageParameters: Map[String, String],
@@ -345,14 +320,10 @@ private[spark] class SparkNumberFormatException
private[spark](
)
}
- def this(message: String) = {
- this(
- message,
- errorClass = None,
- messageParameters = Map.empty,
- context = Array.empty
- )
- }
+ def this(
+ errorClass: String,
+ messageParameters: Map[String, String],
+ context: Array[QueryContext]) = this(errorClass, messageParameters,
context, "")
override def getMessageParameters: java.util.Map[String, String] =
messageParameters.asJava
@@ -363,7 +334,7 @@ private[spark] class SparkNumberFormatException
private[spark](
/**
* Illegal argument exception thrown from Spark with an error class.
*/
-private[spark] class SparkIllegalArgumentException(
+private[spark] class SparkIllegalArgumentException private(
message: String,
cause: Option[Throwable],
errorClass: Option[String],
@@ -387,30 +358,19 @@ private[spark] class SparkIllegalArgumentException(
)
}
- def this(message: String, cause: Option[Throwable]) = {
- this(
- message,
- cause = cause,
- errorClass = None,
- messageParameters = Map.empty,
- context = Array.empty
- )
- }
-
override def getMessageParameters: java.util.Map[String, String] =
messageParameters.asJava
override def getErrorClass: String = errorClass.orNull
override def getQueryContext: Array[QueryContext] = context
}
-private[spark] class SparkRuntimeException(
+private[spark] class SparkRuntimeException private(
message: String,
cause: Option[Throwable],
errorClass: Option[String],
messageParameters: Map[String, String],
context: Array[QueryContext])
- extends RuntimeException(message, cause.orNull)
- with SparkThrowable {
+ extends RuntimeException(message, cause.orNull) with SparkThrowable {
def this(
errorClass: String,
@@ -427,16 +387,6 @@ private[spark] class SparkRuntimeException(
)
}
- def this(message: String, cause: Option[Throwable]) = {
- this(
- message,
- cause = cause,
- errorClass = None,
- messageParameters = Map.empty,
- context = Array.empty
- )
- }
-
override def getMessageParameters: java.util.Map[String, String] =
messageParameters.asJava
override def getErrorClass: String = errorClass.orNull
@@ -480,7 +430,7 @@ private[spark] class SparkSecurityException(
/**
* Array index out of bounds exception thrown from Spark with an error class.
*/
-private[spark] class SparkArrayIndexOutOfBoundsException(
+private[spark] class SparkArrayIndexOutOfBoundsException private(
message: String,
errorClass: Option[String],
messageParameters: Map[String, String],
@@ -501,14 +451,10 @@ private[spark] class SparkArrayIndexOutOfBoundsException(
)
}
- def this(message: String) = {
- this(
- message,
- errorClass = None,
- messageParameters = Map.empty,
- context = Array.empty
- )
- }
+ def this(
+ errorClass: String,
+ messageParameters: Map[String, String],
+ context: Array[QueryContext]) = this(errorClass, messageParameters,
context, "")
override def getMessageParameters: java.util.Map[String, String] =
messageParameters.asJava
diff --git
a/connector/connect/client/jvm/src/test/scala/org/apache/spark/sql/connect/client/SparkConnectClientSuite.scala
b/connector/connect/client/jvm/src/test/scala/org/apache/spark/sql/connect/client/SparkConnectClientSuite.scala
index 698457ddb91d..d14caebe5b81 100644
---
a/connector/connect/client/jvm/src/test/scala/org/apache/spark/sql/connect/client/SparkConnectClientSuite.scala
+++
b/connector/connect/client/jvm/src/test/scala/org/apache/spark/sql/connect/client/SparkConnectClientSuite.scala
@@ -211,23 +211,36 @@ class SparkConnectClientSuite extends ConnectFunSuite
with BeforeAndAfterEach {
}
}
- for ((name, constructor) <- GrpcExceptionConverter.errorFactory) {
- test(s"error framework parameters - $name") {
- val testParams = GrpcExceptionConverter.ErrorParams(
- message = "Found duplicate keys `abc`",
- cause = None,
- errorClass = Some("DUPLICATE_KEY"),
- messageParameters = Map("keyColumn" -> "`abc`"),
- queryContext = Array.empty)
- val error = constructor(testParams)
- assert(error.getMessage.contains(testParams.message))
- assert(error.getCause == null)
- error match {
- case sparkThrowable: SparkThrowable =>
- assert(sparkThrowable.getErrorClass == testParams.errorClass.get)
- assert(sparkThrowable.getMessageParameters.asScala ==
testParams.messageParameters)
- assert(sparkThrowable.getQueryContext.isEmpty)
- case _ =>
+ test("error framework parameters") {
+ val errors = GrpcExceptionConverter.errorFactory
+ for ((name, constructor) <- errors if name.startsWith("org.apache.spark"))
{
+ withClue(name) {
+ val testParams = GrpcExceptionConverter.ErrorParams(
+ message = "",
+ cause = None,
+ errorClass = Some("DUPLICATE_KEY"),
+ messageParameters = Map("keyColumn" -> "`abc`"),
+ queryContext = Array.empty)
+ val error = constructor(testParams).asInstanceOf[Throwable with
SparkThrowable]
+ assert(error.getMessage.contains(testParams.message))
+ assert(error.getCause == null)
+ assert(error.getErrorClass == testParams.errorClass.get)
+ assert(error.getMessageParameters.asScala ==
testParams.messageParameters)
+ assert(error.getQueryContext.isEmpty)
+ }
+ }
+
+ for ((name, constructor) <- errors if
!name.startsWith("org.apache.spark")) {
+ withClue(name) {
+ val testParams = GrpcExceptionConverter.ErrorParams(
+ message = "Found duplicate keys `abc`",
+ cause = None,
+ errorClass = None,
+ messageParameters = Map.empty,
+ queryContext = Array.empty)
+ val error = constructor(testParams)
+ assert(error.getMessage.contains(testParams.message))
+ assert(error.getCause == null)
}
}
}
diff --git
a/connector/connect/common/src/main/scala/org/apache/spark/sql/connect/client/GrpcExceptionConverter.scala
b/connector/connect/common/src/main/scala/org/apache/spark/sql/connect/client/GrpcExceptionConverter.scala
index cc47924de3b0..6641e8c73fc7 100644
---
a/connector/connect/common/src/main/scala/org/apache/spark/sql/connect/client/GrpcExceptionConverter.scala
+++
b/connector/connect/common/src/main/scala/org/apache/spark/sql/connect/client/GrpcExceptionConverter.scala
@@ -196,21 +196,15 @@ private[client] object GrpcExceptionConverter {
errorClass = params.errorClass.orNull,
messageParameters = params.messageParameters,
queryContext = params.queryContext)),
- errorConstructor(params => {
- if (params.errorClass.isEmpty) {
- new AnalysisException(
- errorClass = "_LEGACY_ERROR_TEMP_3100",
- messageParameters = Map("message" -> params.message),
- cause = params.cause,
- context = params.queryContext)
- } else {
- new AnalysisException(
- errorClass = params.errorClass.get,
- messageParameters = params.messageParameters,
- cause = params.cause,
- context = params.queryContext)
- }
- }),
+ errorConstructor(params =>
+ new AnalysisException(
+ errorClass = params.errorClass.getOrElse("_LEGACY_ERROR_TEMP_3100"),
+ messageParameters = params.errorClass match {
+ case Some(_) => params.messageParameters
+ case None => Map("message" -> params.message)
+ },
+ cause = params.cause,
+ context = params.queryContext)),
errorConstructor(params =>
new NamespaceAlreadyExistsException(params.errorClass.orNull,
params.messageParameters)),
errorConstructor(params =>
@@ -232,53 +226,63 @@ private[client] object GrpcExceptionConverter {
new NoSuchTableException(params.errorClass.orNull,
params.messageParameters, params.cause)),
errorConstructor[NumberFormatException](params =>
new SparkNumberFormatException(
- params.message,
- params.errorClass,
- params.messageParameters,
+ errorClass = params.errorClass.getOrElse("_LEGACY_ERROR_TEMP_3104"),
+ messageParameters = params.errorClass match {
+ case Some(_) => params.messageParameters
+ case None => Map("message" -> params.message)
+ },
params.queryContext)),
errorConstructor[IllegalArgumentException](params =>
new SparkIllegalArgumentException(
- params.message,
- params.cause,
- params.errorClass,
- params.messageParameters,
- params.queryContext)),
+ errorClass = params.errorClass.getOrElse("_LEGACY_ERROR_TEMP_3105"),
+ messageParameters = params.errorClass match {
+ case Some(_) => params.messageParameters
+ case None => Map("message" -> params.message)
+ },
+ params.queryContext,
+ cause = params.cause.orNull)),
errorConstructor[ArithmeticException](params =>
new SparkArithmeticException(
- params.message,
- params.errorClass,
- params.messageParameters,
+ errorClass = params.errorClass.getOrElse("_LEGACY_ERROR_TEMP_3106"),
+ messageParameters = params.errorClass match {
+ case Some(_) => params.messageParameters
+ case None => Map("message" -> params.message)
+ },
params.queryContext)),
errorConstructor[UnsupportedOperationException](params =>
new SparkUnsupportedOperationException(
- params.message,
- params.errorClass,
- params.messageParameters)),
+ errorClass = params.errorClass.getOrElse("_LEGACY_ERROR_TEMP_3107"),
+ messageParameters = params.errorClass match {
+ case Some(_) => params.messageParameters
+ case None => Map("message" -> params.message)
+ })),
errorConstructor[ArrayIndexOutOfBoundsException](params =>
new SparkArrayIndexOutOfBoundsException(
- params.message,
- params.errorClass,
- params.messageParameters,
+ errorClass = params.errorClass.getOrElse("_LEGACY_ERROR_TEMP_3108"),
+ messageParameters = params.errorClass match {
+ case Some(_) => params.messageParameters
+ case None => Map("message" -> params.message)
+ },
params.queryContext)),
errorConstructor[DateTimeException](params =>
new SparkDateTimeException(
- params.message,
- params.errorClass,
- params.messageParameters,
+ errorClass = params.errorClass.getOrElse("_LEGACY_ERROR_TEMP_3109"),
+ messageParameters = params.errorClass match {
+ case Some(_) => params.messageParameters
+ case None => Map("message" -> params.message)
+ },
params.queryContext)),
errorConstructor(params =>
new SparkRuntimeException(
- params.message,
- params.cause,
- params.errorClass,
+ params.errorClass.orNull,
params.messageParameters,
+ params.cause.orNull,
params.queryContext)),
errorConstructor(params =>
new SparkUpgradeException(
- params.message,
- params.cause,
- params.errorClass,
- params.messageParameters)),
+ params.errorClass.orNull,
+ params.messageParameters,
+ params.cause.orNull)),
errorConstructor(params =>
new SparkException(
message = params.message,
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@spark.apache.org
For additional commands, e-mail: commits-h...@spark.apache.org
