[SPARK-34920][CORE][SQL] Add error classes with SQLSTATE #32850

karenfeng · 2021-06-09T20:34:32Z

What changes were proposed in this pull request?

Unifies exceptions thrown from Spark under a single base trait SparkError, which unifies:

Error classes
Parametrized error messages
SQLSTATE, as discussed in http://apache-spark-developers-list.1001551.n3.nabble.com/DISCUSS-Add-error-IDs-td31126.html.

Why are the changes needed?

Adding error classes creates a consistent label for exceptions, even as error messages change
Creating a single, centralized source-of-truth for parametrized error messages improves auditing for error message quality
Adding SQLSTATE helps ODBC/JDBC users receive standardized error codes

Does this PR introduce any user-facing change?

Yes, changes ODBC experience by:

Adding error classes to error messages
Adding SQLSTATE to TStatus

How was this patch tested?

Unit tests, as well as local tests with PyODBC.

Signed-off-by: Karen Feng <karen.feng@databricks.com>

SparkQA · 2021-06-09T21:39:07Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44122/

SparkQA · 2021-06-09T22:15:50Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44122/

SparkQA · 2021-06-09T22:22:45Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44123/

SparkQA · 2021-06-09T22:58:33Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44123/

SparkQA · 2021-06-09T23:01:26Z

Test build #139595 has finished for PR 32850 at commit 4e5e410.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

karenfeng · 2021-06-09T23:16:49Z

@wangyum, this is similar to your work in #32013. Can you take a look?

SparkQA · 2021-06-09T23:45:47Z

Test build #139596 has finished for PR 32850 at commit 92e275f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

wangyum · 2021-06-10T05:03:08Z

core/src/main/resources/error/error-classes.json

+    "sqlState" : "40000",
+    "messageFormatLines" : [ "Writing job aborted" ]
+  }
+}


Do you mean sqlState always unique?

Not necessarily; sqlState can be re-used, especially for common error classes without known subclasses, like 42000 (syntax or semantic error). In those cases, the error class will be the disambiguator.

core/src/main/resources/error/README.md

cloud-fan · 2021-06-11T13:18:57Z

core/src/main/resources/error/README.md

+
+## Fields
+
+All fields, excluding error messages, should be consistent across releases.


what do you mean by "consistent"? we can't change the error message in a new release?

This sounds a pretty strict requirement.

Error messages can (and probably will) change between releases; that's why I included the caveat of "excluding error messages." This is actually a major driver for this - so we can improve the quality of error messages over time.

To clarify, I think it would be beneficial for our user base if we were consistent across error class and SQLSTATE across releases. With consistent error classes, users can build in known work-arounds or catch and re-throw known error types. With consistent SQLSTATEs, clients will also have predictable behavior.

core/src/main/resources/error/error-classes.json

cloud-fan · 2021-06-11T13:31:50Z

cc @viirya @maropu @dongjoon-hyun

viirya · 2021-06-11T22:33:09Z

core/src/main/resources/error/README.md

+
+To throw an exception, do the following.
+
+1. Check if an appropriate error class already exists in `error-class.json`.


I have a concern that this will be another burden. E.g. naming a error class, finding error class.

I also worry that the number of error classes will grow quickly and hard to maintain.

Today, the error messages thrown from Spark are distributed across the entire code base with no single source of truth - as a result, it is hard to audit error message for quality and redundancy. With error classes, I'm hoping that we can improve the auditing process. However, I do recognize that the number of error classes will likely grow quickly, and that maintaining a high level of quality will require vigilant pruning. What do you think will help reduce the burden here?

Why do we need to load these error states from a json file instead of defining them statically in a companion object?

I think json file is more general for auditing, doc generation, internationalization (each error message has translations for different languages).

viirya · 2021-06-12T06:24:57Z

core/src/main/scala/org/apache/spark/SparkError.scala

+ *                           The error message is constructed by concatenating the lines with
+ *                           linebreaks.
+ */
+case class ErrorInfo(sqlState: Option[String], messageFormatLines: Seq[String]) {


This is defined in core. But sql state is a SQL concept?

I agree; SQLSTATE is a SQL concept. Off the top of my head, I'm not sure how to have this only be set in the sql component without increasing the complexity of the implementation:

Create a JSON file that maps error classes to SQLSTATE in SQL, which may increase maintenance burden and decrease developer usability

Create a SqlErrorInfo(sqlState, messageFormatLines) class that extends ErrorInfo(messageFormatLines) in the sql component, and parse the JSON file again to get the sqlState field

What do you think?

In addition to the implementation difficulty, I think it also makes sense to have sqlState be accessible from any type of Spark exception (regardless of the base component), given that we currently throw SparkExceptions (from the core component) during query execution. We could introduce a SparkSqlError type, but we would have to do a major refactor of the existing exception types as well in that case, as well as a cleaner division between the core and sql components.

It's better to have a unified representation for all the errors. "error class" and "message" are required fields, and it's ok to have more optional fields for other purposes, like "sqlState" for JDBC compatibility.

Yup, maybe we can consider giving a general name for sqlState (e.g. errorState, errorStateNumber, or etc).

Postgres calls their SQLSTATEs error codes, but I'm not sure if it'd be safe for us to do the same thing, especially given that we could add Spark-specific error codes in the future that span across non-SQL functionality.

I prefer sqlState, to clearly indicate what it is, since SQL state is a standard.

wangyum · 2021-06-15T05:31:26Z

core/src/main/resources/error/error-classes.json

+  "DUPLICATE_KEY_ERROR" : {
+    "sqlState" : "23000",
+    "messageFormatLines" : [ "Found duplicate keys '%s'" ]
+  },


Do we have vendorCode? https://docs.oracle.com/en/java/javase/11/docs/api/java.sql/java/sql/SQLException.html#%3Cinit%3E(java.lang.String,java.lang.String,int)

We can add vendorCode in the future, but I think we would need to determine how error codes should be defined - whether there should be a class hierarchy, if they should be arbitrary, or something else (eg. hashed from the error class). There are some clients that except the vendor code to match Hive error code classes, given that they are thrown within a HiveSQLException.

wangyum · 2021-06-15T06:21:22Z

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala

@@ -316,7 +316,8 @@ object QueryParsingErrors {
  }

  def duplicateKeysError(key: String, ctx: ParserRuleContext): Throwable = {
-    new ParseException(s"Found duplicate keys '$key'.", ctx)
+    // Found duplicate keys '$key'
+    new ParseException(errorClass = "DUPLICATE_KEY_ERROR", messageParameters = Seq(key), ctx)


Hard coding is used here. Looks hard to maintain. This is why I use objects:
https://github.com/apache/spark/blob/89c6f523c832daf7cff5c1ba8aecfbcd5494d5af/core/src/main/scala/org/apache/spark/errors/coreErrors.scala

From a Scala-focused developer perspective, I agree that objects are easier to maintain. However, using string classes has a couple of benefits:

The JSON file can be easily translated into a auto-generated documentation page (without tricks like reflection)

Other clients (eg. Python and R) can natively throw errors with error classes, creating a single easily-auditable source of truth

What do you think? For a happy medium, we could auto-generate Scala from JSON, but I think that may increase maintenance burden.

Signed-off-by: Karen Feng <karen.feng@databricks.com>

SparkQA · 2021-06-21T20:12:54Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44625/

maropu · 2021-06-28T12:53:44Z

core/src/main/resources/error/README.md

+
+To throw an exception, do the following.
+
+1. Check if an appropriate error class already exists in `error-class.json`.


Why do we need to load these error states from a json file instead of defining them statically in a companion object?

maropu · 2021-06-28T12:56:12Z

core/src/main/resources/error/error-classes.json

+  "DIVIDE_BY_ZERO" : {
+    "sqlState" : "22012",
+    "message" : [ "divide by zero" ]
+  },


Just a comment; it would be nice to automatically generate a user's doc page (just like the PostgreSQL one) from this error state definition file.

The reasoning for having all of the error states is linked to having everything in a JSON file - it's relatively straightforward to generate docs pages. (It's also easier to use the same error states in Python/R.) I have code lined up for docs page generation as a followup, but want to make sure this goes in first.

core/src/main/scala/org/apache/spark/SparkException.scala

Signed-off-by: Karen Feng <karen.feng@databricks.com>

cloud-fan · 2021-06-29T06:53:45Z

retest this please

core/src/main/resources/error/README.md

core/src/main/resources/error/error-classes.json

cloud-fan · 2021-06-29T07:10:11Z

core/src/main/scala/org/apache/spark/SparkError.scala

+ * @param message C-style message format compatible with printf.
+ *                The error message is constructed by concatenating the lines with newlines.
+ */
+case class ErrorInfo(sqlState: Option[String], message: Seq[String]) {


If this is a public API, it's better to use class as case class exposes too many APIs (apply, unapply, copy, etc.)

I think we can make ErrorInfo a private API. These fields should be accessed from the exception type.

core/src/main/scala/org/apache/spark/SparkError.scala

cloud-fan

LGTM except some minor comments

Signed-off-by: Karen Feng <karen.feng@databricks.com>

SparkQA · 2021-06-29T20:38:49Z

Test build #140401 has finished for PR 32850 at commit 2a06bb1.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

Signed-off-by: Karen Feng <karen.feng@databricks.com>

SparkQA · 2021-06-30T02:57:09Z

Test build #140378 has finished for PR 32850 at commit e471e6b.

This patch fails from timeout after a configured wait of 500m.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class SparkArithmeticException(

Signed-off-by: Karen Feng <karen.feng@databricks.com>

SparkQA · 2021-06-30T04:26:44Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44934/

SparkQA · 2021-06-30T05:04:06Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44934/

SparkQA · 2021-06-30T05:05:44Z

Test build #140419 has finished for PR 32850 at commit d73bb83.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-06-30T05:24:05Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44937/

SparkQA · 2021-06-30T06:01:10Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44937/

wangyum · 2021-06-30T06:14:18Z

Last question. How do other common modules use these errors? For example: spark-unsafe, spark-network-common.

SparkQA · 2021-06-30T06:17:17Z

Test build #140422 has finished for PR 32850 at commit 6d8e915.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2021-06-30T09:00:08Z

How do other common modules use these errors? For example: spark-unsafe, spark-network-common.

Similar problems appear in the config framework as well. spark-unsafe, spark-network-common can only hardcode config names instead of using the config framework. I think we should create a new module to contain these basic infras such as config, error systems, etc., and other modules all depend on this basic module.

cloud-fan · 2021-06-30T09:22:00Z

thanks, merging to master!

karenfeng added 5 commits May 27, 2021 14:26

WIP

82de870

Signed-off-by: Karen Feng <karen.feng@databricks.com>

Add thriftserver change

624d8ca

Signed-off-by: Karen Feng <karen.feng@databricks.com>

Catch up

3770ac2

Signed-off-by: Karen Feng <karen.feng@databricks.com>

Remove unused conf

60876e1

Signed-off-by: Karen Feng <karen.feng@databricks.com>

Clean up

4e5e410

Signed-off-by: Karen Feng <karen.feng@databricks.com>

github-actions bot added CORE DOCS SQL labels Jun 9, 2021

karenfeng changed the title ~~[SPARK-34920] Add SQLSTATE to exceptions thrown from Spark~~ [SPARK-34920] Add error classes with SQLSTATE Jun 9, 2021

Fix docs issue

92e275f

Signed-off-by: Karen Feng <karen.feng@databricks.com>

wangyum reviewed Jun 10, 2021

View reviewed changes

yaooqinn mentioned this pull request Jun 10, 2021

[Umbrella] Support SQLSTATE and venderCode in KyuubiSQLException apache/kyuubi#670

Open

2 tasks

karenfeng changed the title ~~[SPARK-34920] Add error classes with SQLSTATE~~ [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE Jun 10, 2021

cloud-fan reviewed Jun 11, 2021

View reviewed changes

core/src/main/resources/error/README.md Outdated Show resolved Hide resolved

cloud-fan reviewed Jun 11, 2021

View reviewed changes

core/src/main/resources/error/error-classes.json Outdated Show resolved Hide resolved

viirya reviewed Jun 11, 2021

View reviewed changes

viirya reviewed Jun 12, 2021

View reviewed changes

wangyum reviewed Jun 15, 2021

View reviewed changes

Cleanup

bfdfc87

Signed-off-by: Karen Feng <karen.feng@databricks.com>

maropu reviewed Jun 28, 2021

View reviewed changes

Make SparkArithmeticException accessible

e471e6b

Signed-off-by: Karen Feng <karen.feng@databricks.com>

cloud-fan reviewed Jun 29, 2021

View reviewed changes

core/src/main/resources/error/README.md Outdated Show resolved Hide resolved

cloud-fan reviewed Jun 29, 2021

View reviewed changes

core/src/main/resources/error/README.md Outdated Show resolved Hide resolved

cloud-fan reviewed Jun 29, 2021

View reviewed changes

core/src/main/resources/error/error-classes.json Outdated Show resolved Hide resolved

cloud-fan reviewed Jun 29, 2021

View reviewed changes

core/src/main/scala/org/apache/spark/SparkError.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Jun 29, 2021

View reviewed changes

core/src/main/scala/org/apache/spark/SparkError.scala Show resolved Hide resolved

cloud-fan approved these changes Jun 29, 2021

View reviewed changes

Address comments

2a06bb1

Signed-off-by: Karen Feng <karen.feng@databricks.com>

ErrorInfo should be case class

d73bb83

Signed-off-by: Karen Feng <karen.feng@databricks.com>

karenfeng added 2 commits June 29, 2021 20:07

Remove unused messageFormat field

73b2930

Signed-off-by: Karen Feng <karen.feng@databricks.com>

Hide messageParameters

6d8e915

Signed-off-by: Karen Feng <karen.feng@databricks.com>

cloud-fan approved these changes Jun 30, 2021

View reviewed changes

wangyum approved these changes Jun 30, 2021

View reviewed changes

cloud-fan closed this in e3bd817 Jun 30, 2021

abellina mentioned this pull request Oct 17, 2022

[Audit][BUG] Handle updated messageParameters for any thrown Spark exceptions in Spark 3.4.x NVIDIA/spark-rapids#6671

Closed


		## Fields

		All fields, excluding error messages, should be consistent across releases.


		To throw an exception, do the following.

		1. Check if an appropriate error class already exists in `error-class.json`.

[SPARK-34920][CORE][SQL] Add error classes with SQLSTATE #32850

[SPARK-34920][CORE][SQL] Add error classes with SQLSTATE #32850

Conversation

karenfeng commented Jun 9, 2021

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

SparkQA commented Jun 9, 2021

SparkQA commented Jun 9, 2021

SparkQA commented Jun 9, 2021

SparkQA commented Jun 9, 2021

SparkQA commented Jun 9, 2021

karenfeng commented Jun 9, 2021

SparkQA commented Jun 9, 2021

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

cloud-fan commented Jun 11, 2021

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

karenfeng Jun 23, 2021 • edited Loading

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

SparkQA commented Jun 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Jun 29, 2021

cloud-fan Jun 29, 2021 • edited Loading

Choose a reason for hiding this comment

karenfeng Jun 29, 2021 • edited Loading

Choose a reason for hiding this comment

cloud-fan left a comment

Choose a reason for hiding this comment

SparkQA commented Jun 29, 2021

SparkQA commented Jun 30, 2021

SparkQA commented Jun 30, 2021

SparkQA commented Jun 30, 2021

SparkQA commented Jun 30, 2021

SparkQA commented Jun 30, 2021

SparkQA commented Jun 30, 2021

wangyum commented Jun 30, 2021

SparkQA commented Jun 30, 2021

cloud-fan commented Jun 30, 2021

cloud-fan commented Jun 30, 2021

karenfeng Jun 23, 2021 •

edited

Loading

cloud-fan Jun 29, 2021 •

edited

Loading

karenfeng Jun 29, 2021 •

edited

Loading