Skip to content

[SPARK-46490][SQL] Require error classes in SparkThrowable sub-classes #44464

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
MaxGekk wants to merge 15 commits into from
Closed

[SPARK-46490][SQL] Require error classes in SparkThrowable sub-classes #44464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
2b3478d
Require error classes in SparkThrowable sub-classes
MaxGekk Dec 22, 2023
12bb7e8
Trigger build
MaxGekk Dec 22, 2023
d00ef90
Fix formatting
MaxGekk Dec 22, 2023
384e879
Fix tests
MaxGekk Dec 23, 2023
ef8091f
Try to fix
MaxGekk Dec 23, 2023
753c36e
Trigger build
MaxGekk Dec 23, 2023
f45a37e
Fix formatting
MaxGekk Dec 23, 2023
80a58bc
Revert "Fix formatting"
MaxGekk Dec 28, 2023
d79e461
Revert "Try to fix"
MaxGekk Dec 28, 2023
8a194d3
Merge remote-tracking branch 'origin/master' into ban-messages-SparkT…
MaxGekk Dec 28, 2023
56c1c85
Handle default cases
MaxGekk Dec 29, 2023
a79c586
Fix formatting
MaxGekk Dec 29, 2023
b132aa7
Address review comments
MaxGekk Dec 29, 2023
d583c8c
Remove s
MaxGekk Dec 29, 2023
8b18ac2
Improve a test
MaxGekk Dec 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions common/utils/src/main/resources/error/error-classes.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>"
Expand Down
112 changes: 29 additions & 83 deletions common/utils/src/main/scala/org/apache/spark/SparkException.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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
Expand All @@ -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],
Expand All @@ -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

Expand All @@ -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])
Expand All @@ -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
Expand Down Expand Up @@ -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],
Expand All @@ -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

Expand All @@ -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],
Expand All @@ -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

Expand All @@ -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],
Expand All @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -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],
Expand All @@ -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

Expand Down
47 changes: 30 additions & 17 deletions ...ient/jvm/src/test/scala/org/apache/spark/sql/connect/client/SparkConnectClientSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

@heyihong heyihong Dec 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to update the test to cover both errorClass = Some(...) and errorClass = None

Copy link
Member Author

@MaxGekk MaxGekk Dec 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

assert(error.getMessage.contains(testParams.message))
assert(error.getCause == null)
}
}
}
Expand Down
Loading