You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/06/09 20:34:45 UTC
[GitHub] [spark] karenfeng opened a new pull request #32850: [SPARK-34920] Add SQLSTATE to exceptions thrown from Spark
karenfeng opened a new pull request #32850:
URL: https://github.com/apache/spark/pull/32850
### 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-865287328
**[Test build #140097 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/140097/testReport)** for PR 32850 at commit [`bfdfc87`](https://github.com/apache/spark/commit/bfdfc87ce696fedbfd7deb5b556e7248972aab46).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-871222996
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] karenfeng commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
karenfeng commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r655592044
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -0,0 +1,22 @@
+{
+ "DIVIDE_BY_ZERO_ERROR" : {
+ "sqlState" : "22012",
+ "messageFormatLines" : [ "divide by zero" ]
+ },
+ "DUPLICATE_KEY_ERROR" : {
+ "sqlState" : "23000",
+ "messageFormatLines" : [ "Found duplicate keys '%s'" ]
+ },
Review comment:
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](https://issues.apache.org/jira/browse/HIVE-3001), given that they are thrown within a `HiveSQLException`.
##########
File path: core/src/main/resources/error/README.md
##########
@@ -0,0 +1,79 @@
+# Guidelines
+
+To throw a standardized exception, developers should use an error class and message parameters
+rather than an arbitrary error message.
+
+## Usage
+
+To throw an exception, do the following.
+
+1. Check if an appropriate error class already exists in `error-class.json`.
Review comment:
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?
##########
File path: 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)
Review comment:
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.
##########
File path: core/src/main/scala/org/apache/spark/SparkError.scala
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import java.net.URL
+
+import scala.collection.immutable.SortedMap
+
+import com.fasterxml.jackson.annotation.JsonIgnore
+import com.fasterxml.jackson.core.`type`.TypeReference
+import com.fasterxml.jackson.databind.SerializationFeature
+import com.fasterxml.jackson.databind.json.JsonMapper
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+
+import org.apache.spark.util.Utils
+
+/**
+ * Information associated with an error class.
+ *
+ * @param sqlState SQLSTATE associated with this class.
+ * @param messageFormatLines C-style message format compatible with printf.
+ * The error message is constructed by concatenating the lines with
+ * linebreaks.
+ */
+case class ErrorInfo(sqlState: Option[String], messageFormatLines: Seq[String]) {
Review comment:
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?
##########
File path: core/src/main/resources/error/README.md
##########
@@ -0,0 +1,79 @@
+# Guidelines
+
+To throw a standardized exception, developers should use an error class and message parameters
+rather than an arbitrary error message.
+
+## Usage
+
+To throw an exception, do the following.
+
+1. Check if an appropriate error class already exists in `error-class.json`.
+ If true, skip to step 3. Otherwise, continue to step 2.
+2. Add a new class to `error-class.json`; keep in mind the invariants below.
+3. Check if the exception type already extends `SparkError`.
+ If true, skip to step 5. Otherwise, continue to step 4.
+4. Mix `SparkError` into the exception.
+5. Throw the exception with the error class and message parameters.
+
+### Before
+
+Throw exception:
+
+ throw new TestException("Problem A because B")
+
+
+### After
+
+`error-class.json`
+
+ "PROBLEM_BECAUSE": {
+ "sqlState": "XXXXX",
+ "messageFormatLines": ["Problem %s because %s"]
+ }
+
+`SparkException.scala`
+
+ class SparkTestException(
+ val errorClass: String,
+ val messageParameters: Seq[String])
+ extends TestException(SparkError.getMessage(errorClass, messageParameters))
+ with SparkError
+
+Throw exception:
+
+ throw new SparkTestException("PROBLEM_BECAUSE", Seq("A", "B"))
+
+## Access fields
+
+To access error fields, catch exceptions that extend `org.apache.spark.SparkError` and access
+ - Error class with `errorClass`
+ - SQLSTATE with `sqlState`
+
+
+ try {
+ ...
+ } catch {
+ case e: SparkError if e.sqlState.forall(_.startsWith("42")) =>
+ warn("Syntax error")
+ }
+
+## Fields
+
+All fields, excluding error messages, should be consistent across releases.
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-865321108
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/44625/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r660333239
##########
File path: core/src/main/resources/error/README.md
##########
@@ -0,0 +1,79 @@
+# Guidelines
+
+To throw a standardized exception, developers should use an error class and message parameters
+rather than an arbitrary error message.
+
+## Usage
+
+To throw an exception, do the following.
+
+1. Check if an appropriate error class already exists in `error-class.json`.
Review comment:
I think json file is more general for auditing, doc generation, internationalization (each error message has translations for different languages).
##########
File path: core/src/main/resources/error/README.md
##########
@@ -0,0 +1,89 @@
+# Guidelines
+
+To throw a standardized user-facing exception, developers should specify the error class and
+message parameters rather than an arbitrary error message.
+
+## Usage
+
+To throw an exception, do the following.
+
+1. Check if an appropriate error class already exists in `error-class.json`.
+ If true, skip to step 3. Otherwise, continue to step 2.
+2. Add a new class to `error-class.json`; keep in mind the invariants below.
+3. Check if the exception type already extends `SparkError`.
+ If true, skip to step 5. Otherwise, continue to step 4.
+4. Mix `SparkError` into the exception.
+5. Throw the exception with the error class and message parameters.
+
+### Before
+
+Throw exception:
+
+ throw new TestException("Problem A because B")
+
+
+### After
+
+`error-class.json`
+
+ "PROBLEM_BECAUSE": {
+ "sqlState": "XXXXX",
+ "message": ["Problem %s because %s"]
Review comment:
nit:
```
"PROBLEM_BECAUSE": {
"message": ["Problem %s because %s"],
"sqlState": "XXXXX" (this is optional)
}
```
##########
File path: core/src/main/resources/error/README.md
##########
@@ -0,0 +1,89 @@
+# Guidelines
+
+To throw a standardized user-facing exception, developers should specify the error class and
+message parameters rather than an arbitrary error message.
+
+## Usage
+
+To throw an exception, do the following.
+
+1. Check if an appropriate error class already exists in `error-class.json`.
+ If true, skip to step 3. Otherwise, continue to step 2.
+2. Add a new class to `error-class.json`; keep in mind the invariants below.
+3. Check if the exception type already extends `SparkError`.
+ If true, skip to step 5. Otherwise, continue to step 4.
+4. Mix `SparkError` into the exception.
+5. Throw the exception with the error class and message parameters.
+
+### Before
+
+Throw exception:
+
+ throw new TestException("Problem A because B")
+
+
+### After
+
+`error-class.json`
+
+ "PROBLEM_BECAUSE": {
+ "sqlState": "XXXXX",
+ "message": ["Problem %s because %s"]
+ }
+
+`SparkException.scala`
+
+ class SparkTestException(
+ val errorClass: String,
+ val messageParameters: Seq[String])
+ extends TestException(SparkError.getMessage(errorClass, messageParameters))
+ with SparkError
+
+Throw exception:
+
+ throw new SparkTestException("PROBLEM_BECAUSE", Seq("A", "B"))
+
+## Access fields
+
+To access error fields, catch exceptions that extend `org.apache.spark.SparkError` and access
+ - Error class with `errorClass`
+ - SQLSTATE with `sqlState`
+
+
+ try {
+ ...
+ } catch {
+ case e: SparkError if e.sqlState.forall(_.startsWith("42")) =>
+ warn("Syntax error")
+ }
+
+## Fields
+
+### Error class
+
+Error classes are a succinct, human-readable representation of the error category.
+
+Invariants:
+
+- Unique
+- Consistent across releases
+- Sorted alphabetically
+
+### SQLSTATE
Review comment:
let's put it after `Message`, as it's optional
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -0,0 +1,22 @@
+{
+ "DIVIDE_BY_ZERO" : {
+ "sqlState" : "22012",
Review comment:
ditto, let's put optional fields at the end.
##########
File path: core/src/main/scala/org/apache/spark/SparkError.scala
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import java.net.URL
+
+import scala.collection.immutable.SortedMap
+
+import com.fasterxml.jackson.annotation.JsonIgnore
+import com.fasterxml.jackson.core.`type`.TypeReference
+import com.fasterxml.jackson.databind.json.JsonMapper
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+
+import org.apache.spark.util.Utils
+
+/**
+ * Information associated with an error class.
+ *
+ * @param sqlState SQLSTATE associated with this class.
+ * @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]) {
Review comment:
This is a public API, it's better to use class as `case class` exposes to many APIs (apply, unapply, copy, etc.)
##########
File path: core/src/main/scala/org/apache/spark/SparkError.scala
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import java.net.URL
+
+import scala.collection.immutable.SortedMap
+
+import com.fasterxml.jackson.annotation.JsonIgnore
+import com.fasterxml.jackson.core.`type`.TypeReference
+import com.fasterxml.jackson.databind.json.JsonMapper
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+
+import org.apache.spark.util.Utils
+
+/**
+ * Information associated with an error class.
+ *
+ * @param sqlState SQLSTATE associated with this class.
+ * @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]) {
Review comment:
This is a public API, it's better to use class as `case class` exposes too many APIs (apply, unapply, copy, etc.)
##########
File path: core/src/main/scala/org/apache/spark/SparkError.scala
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import java.net.URL
+
+import scala.collection.immutable.SortedMap
+
+import com.fasterxml.jackson.annotation.JsonIgnore
+import com.fasterxml.jackson.core.`type`.TypeReference
+import com.fasterxml.jackson.databind.json.JsonMapper
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+
+import org.apache.spark.util.Utils
+
+/**
+ * Information associated with an error class.
+ *
+ * @param sqlState SQLSTATE associated with this class.
+ * @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]) {
+ // For compatibility with multi-line error messages
+ @JsonIgnore
+ val messageFormat: String = message.mkString("\n")
+}
+
+/**
+ * Companion object used by instances of [[SparkError]] to access error class information and
+ * construct error messages.
+ */
+object SparkError {
Review comment:
this can be `private[spark]`
##########
File path: core/src/main/scala/org/apache/spark/SparkError.scala
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import java.net.URL
+
+import scala.collection.immutable.SortedMap
+
+import com.fasterxml.jackson.annotation.JsonIgnore
+import com.fasterxml.jackson.core.`type`.TypeReference
+import com.fasterxml.jackson.databind.json.JsonMapper
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+
+import org.apache.spark.util.Utils
+
+/**
+ * Information associated with an error class.
+ *
+ * @param sqlState SQLSTATE associated with this class.
+ * @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]) {
+ // For compatibility with multi-line error messages
+ @JsonIgnore
+ val messageFormat: String = message.mkString("\n")
+}
+
+/**
+ * Companion object used by instances of [[SparkError]] to access error class information and
+ * construct error messages.
+ */
+object SparkError {
+ val errorClassesUrl: URL =
+ Utils.getSparkClassLoader.getResource("error/error-classes.json")
+ val errorClassToInfoMap: SortedMap[String, ErrorInfo] = {
+ val mapper: JsonMapper = JsonMapper.builder()
+ .addModule(DefaultScalaModule)
+ .build()
+ mapper.readValue(errorClassesUrl, new TypeReference[SortedMap[String, ErrorInfo]]() {})
+ }
+
+ def getMessage(errorClass: String, messageParameters: Seq[String]): String = {
+ val errorInfo = errorClassToInfoMap.getOrElse(errorClass,
+ throw new IllegalArgumentException(s"Cannot find error class '$errorClass'"))
+ String.format(errorInfo.messageFormat, messageParameters: _*)
+ }
+}
+
+/**
+ * Trait mixed into exceptions thrown from Spark.
+ * - For backwards compatibility, existing exception types can be thrown with an arbitrary error
+ * message with no error class. See [[SparkException]].
+ * - To promote standardization, exceptions should be thrown with an error class and message
+ * parameters to construct an error message with SparkError.getMessage(). New exception types
+ * should not accept arbitrary error messages. See [[SparkArithmeticException]].
+ */
+trait SparkError extends Throwable {
Review comment:
let's think about which APIs are needed by end users, and mark others as `protected` or `private[spark]`
##########
File path: core/src/main/scala/org/apache/spark/SparkError.scala
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import java.net.URL
+
+import scala.collection.immutable.SortedMap
+
+import com.fasterxml.jackson.annotation.JsonIgnore
+import com.fasterxml.jackson.core.`type`.TypeReference
+import com.fasterxml.jackson.databind.json.JsonMapper
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+
+import org.apache.spark.util.Utils
+
+/**
+ * Information associated with an error class.
+ *
+ * @param sqlState SQLSTATE associated with this class.
+ * @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]) {
Review comment:
If this is a public API, it's better to use class as `case class` exposes too many APIs (apply, unapply, copy, etc.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-865321108
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/44625/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-870900389
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/140401/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-865321108
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-870900299
**[Test build #140401 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/140401/testReport)** for PR 32850 at commit [`2a06bb1`](https://github.com/apache/spark/commit/2a06bb11e27524dbdf911bb595bebb63bf19c482).
* This patch **fails to build**.
* This patch merges cleanly.
* This patch adds no public classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r656218949
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -0,0 +1,22 @@
+{
+ "DIVIDE_BY_ZERO_ERROR" : {
+ "sqlState" : "22012",
+ "message" : [ "divide by zero" ]
+ },
+ "DUPLICATE_KEY_ERROR" : {
+ "sqlState" : "23000",
+ "message" : [ "Found duplicate keys '%s'" ]
+ },
+ "MISSING_COLUMN_ERROR" : {
+ "sqlState" : "42000",
+ "message" : [ "cannot resolve '%s' given input columns: [%s]" ]
+ },
+ "SECOND_FUNCTION_ARGUMENT_NOT_INTEGER_ERROR" : {
+ "sqlState" : "22023",
+ "message" : [ "The second argument of '%s' function needs to be an integer." ]
+ },
+ "WRITING_JOB_ABORTED_ERROR" : {
Review comment:
do we always need to put ERROR at the end of the error class?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-867242651
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44754/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-865287328
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-870895984
**[Test build #140401 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/140401/testReport)** for PR 32850 at commit [`2a06bb1`](https://github.com/apache/spark/commit/2a06bb11e27524dbdf911bb595bebb63bf19c482).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-865314047
Kubernetes integration test unable to build dist.
exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44625/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r660338248
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -0,0 +1,22 @@
+{
+ "DIVIDE_BY_ZERO" : {
+ "sqlState" : "22012",
Review comment:
ditto, let's put optional fields at the end.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-867226296
**[Test build #140225 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/140225/testReport)** for PR 32850 at commit [`7cbc288`](https://github.com/apache/spark/commit/7cbc288a214a21551252062de0604bccaa7a14b5).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r659770974
##########
File path: core/src/main/scala/org/apache/spark/SparkException.scala
##########
@@ -50,3 +65,16 @@ private[spark] case class ExecutorDeadException(message: String)
private[spark] class SparkUpgradeException(version: String, message: String, cause: Throwable)
extends RuntimeException("You may get a different result due to the upgrading of Spark" +
s" $version: $message", cause)
+
+/**
+ * Arithmetic exception thrown from Spark with an error class.
+ */
+private[spark] class SparkArithmeticException(
Review comment:
`private[spark]`? Could we make the new standardized error classes public to 3rd-party develoeprs?
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -0,0 +1,22 @@
+{
+ "DIVIDE_BY_ZERO" : {
+ "sqlState" : "22012",
+ "message" : [ "divide by zero" ]
+ },
Review comment:
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.
##########
File path: core/src/main/resources/error/README.md
##########
@@ -0,0 +1,79 @@
+# Guidelines
+
+To throw a standardized exception, developers should use an error class and message parameters
+rather than an arbitrary error message.
+
+## Usage
+
+To throw an exception, do the following.
+
+1. Check if an appropriate error class already exists in `error-class.json`.
Review comment:
Why do we need to load these error states from a json file instead of defining them statically in a companion object?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] karenfeng commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
karenfeng commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r655602907
##########
File path: core/src/main/resources/error/README.md
##########
@@ -0,0 +1,79 @@
+# Guidelines
+
+To throw a standardized exception, developers should use an error class and message parameters
+rather than an arbitrary error message.
+
+## Usage
+
+To throw an exception, do the following.
+
+1. Check if an appropriate error class already exists in `error-class.json`.
+ If true, skip to step 3. Otherwise, continue to step 2.
+2. Add a new class to `error-class.json`; keep in mind the invariants below.
+3. Check if the exception type already extends `SparkError`.
+ If true, skip to step 5. Otherwise, continue to step 4.
+4. Mix `SparkError` into the exception.
+5. Throw the exception with the error class and message parameters.
+
+### Before
+
+Throw exception:
+
+ throw new TestException("Problem A because B")
+
+
+### After
+
+`error-class.json`
+
+ "PROBLEM_BECAUSE": {
+ "sqlState": "XXXXX",
+ "messageFormatLines": ["Problem %s because %s"]
+ }
+
+`SparkException.scala`
+
+ class SparkTestException(
+ val errorClass: String,
+ val messageParameters: Seq[String])
+ extends TestException(SparkError.getMessage(errorClass, messageParameters))
+ with SparkError
+
+Throw exception:
+
+ throw new SparkTestException("PROBLEM_BECAUSE", Seq("A", "B"))
+
+## Access fields
+
+To access error fields, catch exceptions that extend `org.apache.spark.SparkError` and access
+ - Error class with `errorClass`
+ - SQLSTATE with `sqlState`
+
+
+ try {
+ ...
+ } catch {
+ case e: SparkError if e.sqlState.forall(_.startsWith("42")) =>
+ warn("Syntax error")
+ }
+
+## Fields
+
+All fields, excluding error messages, should be consistent across releases.
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-858137717
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44122/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-859584261
cc @viirya @maropu @dongjoon-hyun
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r659770974
##########
File path: core/src/main/scala/org/apache/spark/SparkException.scala
##########
@@ -50,3 +65,16 @@ private[spark] case class ExecutorDeadException(message: String)
private[spark] class SparkUpgradeException(version: String, message: String, cause: Throwable)
extends RuntimeException("You may get a different result due to the upgrading of Spark" +
s" $version: $message", cause)
+
+/**
+ * Arithmetic exception thrown from Spark with an error class.
+ */
+private[spark] class SparkArithmeticException(
Review comment:
`private[spark]`? Could we make the new standardized error classes public to 3rd-party develoeprs?
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -0,0 +1,22 @@
+{
+ "DIVIDE_BY_ZERO" : {
+ "sqlState" : "22012",
+ "message" : [ "divide by zero" ]
+ },
Review comment:
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.
##########
File path: core/src/main/resources/error/README.md
##########
@@ -0,0 +1,79 @@
+# Guidelines
+
+To throw a standardized exception, developers should use an error class and message parameters
+rather than an arbitrary error message.
+
+## Usage
+
+To throw an exception, do the following.
+
+1. Check if an appropriate error class already exists in `error-class.json`.
Review comment:
Why do we need to load these error states from a json file instead of defining them statically in a companion object?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-871058211
**[Test build #140378 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/140378/testReport)** for PR 32850 at commit [`e471e6b`](https://github.com/apache/spark/commit/e471e6b188fed75fe569944dc8c5c614c8fb73e9).
* 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(`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-871070332
**[Test build #140422 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/140422/testReport)** for PR 32850 at commit [`6d8e915`](https://github.com/apache/spark/commit/6d8e915ca62806f511588134a57bbeca99361d86).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-867246509
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/44754/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-870297019
retest this please
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-870825238
**[Test build #140378 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/140378/testReport)** for PR 32850 at commit [`e471e6b`](https://github.com/apache/spark/commit/e471e6b188fed75fe569944dc8c5c614c8fb73e9).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #32850: [SPARK-34920] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-858142553
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/44122/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] karenfeng commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
karenfeng commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r659991893
##########
File path: core/src/main/scala/org/apache/spark/SparkException.scala
##########
@@ -50,3 +65,16 @@ private[spark] case class ExecutorDeadException(message: String)
private[spark] class SparkUpgradeException(version: String, message: String, cause: Throwable)
extends RuntimeException("You may get a different result due to the upgrading of Spark" +
s" $version: $message", cause)
+
+/**
+ * Arithmetic exception thrown from Spark with an error class.
+ */
+private[spark] class SparkArithmeticException(
Review comment:
Good idea. Done - thanks for the review!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-867226296
**[Test build #140225 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/140225/testReport)** for PR 32850 at commit [`7cbc288`](https://github.com/apache/spark/commit/7cbc288a214a21551252062de0604bccaa7a14b5).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangyum commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
wangyum commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r651459603
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -0,0 +1,22 @@
+{
+ "DIVIDE_BY_ZERO_ERROR" : {
+ "sqlState" : "22012",
+ "messageFormatLines" : [ "divide by zero" ]
+ },
+ "DUPLICATE_KEY_ERROR" : {
+ "sqlState" : "23000",
+ "messageFormatLines" : [ "Found duplicate keys '%s'" ]
+ },
Review comment:
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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] karenfeng commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
karenfeng commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r659973018
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -0,0 +1,22 @@
+{
+ "DIVIDE_BY_ZERO" : {
+ "sqlState" : "22012",
+ "message" : [ "divide by zero" ]
+ },
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #32850: [SPARK-34920] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-858189870
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/139596/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r660333239
##########
File path: core/src/main/resources/error/README.md
##########
@@ -0,0 +1,79 @@
+# Guidelines
+
+To throw a standardized exception, developers should use an error class and message parameters
+rather than an arbitrary error message.
+
+## Usage
+
+To throw an exception, do the following.
+
+1. Check if an appropriate error class already exists in `error-class.json`.
Review comment:
I think json file is more general for auditing, doc generation, internationalization (each error message has translations for different languages).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-865361440
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/140097/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r657701584
##########
File path: core/src/main/scala/org/apache/spark/SparkError.scala
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import java.net.URL
+
+import scala.collection.immutable.SortedMap
+
+import com.fasterxml.jackson.annotation.JsonIgnore
+import com.fasterxml.jackson.core.`type`.TypeReference
+import com.fasterxml.jackson.databind.SerializationFeature
+import com.fasterxml.jackson.databind.json.JsonMapper
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+
+import org.apache.spark.util.Utils
+
+/**
+ * Information associated with an error class.
+ *
+ * @param sqlState SQLSTATE associated with this class.
+ * @param messageFormatLines C-style message format compatible with printf.
+ * The error message is constructed by concatenating the lines with
+ * linebreaks.
+ */
+case class ErrorInfo(sqlState: Option[String], messageFormatLines: Seq[String]) {
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-871119589
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44937/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r660339648
##########
File path: core/src/main/scala/org/apache/spark/SparkError.scala
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import java.net.URL
+
+import scala.collection.immutable.SortedMap
+
+import com.fasterxml.jackson.annotation.JsonIgnore
+import com.fasterxml.jackson.core.`type`.TypeReference
+import com.fasterxml.jackson.databind.json.JsonMapper
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+
+import org.apache.spark.util.Utils
+
+/**
+ * Information associated with an error class.
+ *
+ * @param sqlState SQLSTATE associated with this class.
+ * @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]) {
Review comment:
This is a public API, it's better to use class as `case class` exposes too many APIs (apply, unapply, copy, etc.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan closed pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #32850:
URL: https://github.com/apache/spark/pull/32850
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-871070332
**[Test build #140422 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/140422/testReport)** for PR 32850 at commit [`6d8e915`](https://github.com/apache/spark/commit/6d8e915ca62806f511588134a57bbeca99361d86).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-858155908
**[Test build #139595 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/139595/testReport)** for PR 32850 at commit [`4e5e410`](https://github.com/apache/spark/commit/4e5e410e03a10ad74d125c82ed894a64302a8679).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-870900389
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/140401/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-870895984
**[Test build #140401 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/140401/testReport)** for PR 32850 at commit [`2a06bb1`](https://github.com/apache/spark/commit/2a06bb11e27524dbdf911bb595bebb63bf19c482).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-865361440
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/140097/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-858172392
**[Test build #139596 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/139596/testReport)** for PR 32850 at commit [`92e275f`](https://github.com/apache/spark/commit/92e275f501cfbd44245620b34744f23962e4cbde).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-867227892
**[Test build #140225 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/140225/testReport)** for PR 32850 at commit [`7cbc288`](https://github.com/apache/spark/commit/7cbc288a214a21551252062de0604bccaa7a14b5).
* This patch **fails to build**.
* This patch merges cleanly.
* This patch adds no public classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #32850: [SPARK-34920] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-858165316
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r659585545
##########
File path: core/src/main/scala/org/apache/spark/SparkError.scala
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import java.net.URL
+
+import scala.collection.immutable.SortedMap
+
+import com.fasterxml.jackson.annotation.JsonIgnore
+import com.fasterxml.jackson.core.`type`.TypeReference
+import com.fasterxml.jackson.databind.SerializationFeature
+import com.fasterxml.jackson.databind.json.JsonMapper
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+
+import org.apache.spark.util.Utils
+
+/**
+ * Information associated with an error class.
+ *
+ * @param sqlState SQLSTATE associated with this class.
+ * @param messageFormatLines C-style message format compatible with printf.
+ * The error message is constructed by concatenating the lines with
+ * linebreaks.
+ */
+case class ErrorInfo(sqlState: Option[String], messageFormatLines: Seq[String]) {
Review comment:
I prefer `sqlState`, to clearly indicate what it is, since SQL state is a standard.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-867227912
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/140225/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-871098074
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44934/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-865287328
**[Test build #140097 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/140097/testReport)** for PR 32850 at commit [`bfdfc87`](https://github.com/apache/spark/commit/bfdfc87ce696fedbfd7deb5b556e7248972aab46).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-871069872
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/140378/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #32850: [SPARK-34920] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-858083271
**[Test build #139595 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/139595/testReport)** for PR 32850 at commit [`4e5e410`](https://github.com/apache/spark/commit/4e5e410e03a10ad74d125c82ed894a64302a8679).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-867227912
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/140225/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangyum commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
wangyum commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r651481154
##########
File path: 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)
Review comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r650282569
##########
File path: core/src/main/resources/error/README.md
##########
@@ -0,0 +1,79 @@
+# Guidelines
+
+To throw a standardized exception, developers should use an error class and message parameters
+rather than an arbitrary error message.
+
+## Usage
+
+To throw an exception, do the following.
+
+1. Check if an appropriate error class already exists in `error-class.json`.
+ If true, skip to step 3. Otherwise, continue to step 2.
+2. Add a new class to `error-class.json`; keep in mind the invariants below.
+3. Check if the exception type already extends `SparkError`.
+ If true, skip to step 5. Otherwise, continue to step 4.
+4. Mix `SparkError` into the exception.
+5. Throw the exception with the error class and message parameters.
+
+### Before
+
+Throw exception:
+
+ throw new TestException("Problem A because B")
+
+
+### After
+
+`error-class.json`
+
+ "PROBLEM_BECAUSE": {
+ "sqlState": "XXXXX",
+ "messageFormatLines": ["Problem %s because %s"]
+ }
+
+`SparkException.scala`
+
+ class SparkTestException(
+ val errorClass: String,
+ val messageParameters: Seq[String])
+ extends TestException(SparkError.getMessage(errorClass, messageParameters))
+ with SparkError
+
+Throw exception:
+
+ throw new SparkTestException("PROBLEM_BECAUSE", Seq("A", "B"))
+
+## Access fields
+
+To access error fields, catch exceptions that extend `org.apache.spark.SparkError` and access
+ - Error class with `errorClass`
+ - SQLSTATE with `sqlState`
+
+
+ try {
+ ...
+ } catch {
+ case e: SparkError if e.sqlState.forall(_.startsWith("42")) =>
+ warn("Syntax error")
+ }
+
+## Fields
+
+All fields, excluding error messages, should be consistent across releases.
Review comment:
This sounds a pretty strict requirement.
##########
File path: core/src/main/resources/error/README.md
##########
@@ -0,0 +1,79 @@
+# Guidelines
+
+To throw a standardized exception, developers should use an error class and message parameters
+rather than an arbitrary error message.
+
+## Usage
+
+To throw an exception, do the following.
+
+1. Check if an appropriate error class already exists in `error-class.json`.
Review comment:
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.
##########
File path: core/src/main/scala/org/apache/spark/SparkError.scala
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import java.net.URL
+
+import scala.collection.immutable.SortedMap
+
+import com.fasterxml.jackson.annotation.JsonIgnore
+import com.fasterxml.jackson.core.`type`.TypeReference
+import com.fasterxml.jackson.databind.SerializationFeature
+import com.fasterxml.jackson.databind.json.JsonMapper
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+
+import org.apache.spark.util.Utils
+
+/**
+ * Information associated with an error class.
+ *
+ * @param sqlState SQLSTATE associated with this class.
+ * @param messageFormatLines C-style message format compatible with printf.
+ * The error message is constructed by concatenating the lines with
+ * linebreaks.
+ */
+case class ErrorInfo(sqlState: Option[String], messageFormatLines: Seq[String]) {
Review comment:
This is defined in core. But sql state is a SQL concept?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r660340985
##########
File path: core/src/main/scala/org/apache/spark/SparkError.scala
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import java.net.URL
+
+import scala.collection.immutable.SortedMap
+
+import com.fasterxml.jackson.annotation.JsonIgnore
+import com.fasterxml.jackson.core.`type`.TypeReference
+import com.fasterxml.jackson.databind.json.JsonMapper
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+
+import org.apache.spark.util.Utils
+
+/**
+ * Information associated with an error class.
+ *
+ * @param sqlState SQLSTATE associated with this class.
+ * @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]) {
+ // For compatibility with multi-line error messages
+ @JsonIgnore
+ val messageFormat: String = message.mkString("\n")
+}
+
+/**
+ * Companion object used by instances of [[SparkError]] to access error class information and
+ * construct error messages.
+ */
+object SparkError {
+ val errorClassesUrl: URL =
+ Utils.getSparkClassLoader.getResource("error/error-classes.json")
+ val errorClassToInfoMap: SortedMap[String, ErrorInfo] = {
+ val mapper: JsonMapper = JsonMapper.builder()
+ .addModule(DefaultScalaModule)
+ .build()
+ mapper.readValue(errorClassesUrl, new TypeReference[SortedMap[String, ErrorInfo]]() {})
+ }
+
+ def getMessage(errorClass: String, messageParameters: Seq[String]): String = {
+ val errorInfo = errorClassToInfoMap.getOrElse(errorClass,
+ throw new IllegalArgumentException(s"Cannot find error class '$errorClass'"))
+ String.format(errorInfo.messageFormat, messageParameters: _*)
+ }
+}
+
+/**
+ * Trait mixed into exceptions thrown from Spark.
+ * - For backwards compatibility, existing exception types can be thrown with an arbitrary error
+ * message with no error class. See [[SparkException]].
+ * - To promote standardization, exceptions should be thrown with an error class and message
+ * parameters to construct an error message with SparkError.getMessage(). New exception types
+ * should not accept arbitrary error messages. See [[SparkArithmeticException]].
+ */
+trait SparkError extends Throwable {
Review comment:
let's think about which APIs are needed by end users, and mark others as `protected` or `private[spark]`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #32850: [SPARK-34920] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-858142553
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/44122/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-870825238
**[Test build #140378 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/140378/testReport)** for PR 32850 at commit [`e471e6b`](https://github.com/apache/spark/commit/e471e6b188fed75fe569944dc8c5c614c8fb73e9).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r660339927
##########
File path: core/src/main/scala/org/apache/spark/SparkError.scala
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import java.net.URL
+
+import scala.collection.immutable.SortedMap
+
+import com.fasterxml.jackson.annotation.JsonIgnore
+import com.fasterxml.jackson.core.`type`.TypeReference
+import com.fasterxml.jackson.databind.json.JsonMapper
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+
+import org.apache.spark.util.Utils
+
+/**
+ * Information associated with an error class.
+ *
+ * @param sqlState SQLSTATE associated with this class.
+ * @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]) {
+ // For compatibility with multi-line error messages
+ @JsonIgnore
+ val messageFormat: String = message.mkString("\n")
+}
+
+/**
+ * Companion object used by instances of [[SparkError]] to access error class information and
+ * construct error messages.
+ */
+object SparkError {
Review comment:
this can be `private[spark]`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-865287328
**[Test build #140097 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/140097/testReport)** for PR 32850 at commit [`bfdfc87`](https://github.com/apache/spark/commit/bfdfc87ce696fedbfd7deb5b556e7248972aab46).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920] Add SQLSTATE to exceptions thrown from Spark
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-858083271
**[Test build #139595 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/139595/testReport)** for PR 32850 at commit [`4e5e410`](https://github.com/apache/spark/commit/4e5e410e03a10ad74d125c82ed894a64302a8679).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r660339648
##########
File path: core/src/main/scala/org/apache/spark/SparkError.scala
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import java.net.URL
+
+import scala.collection.immutable.SortedMap
+
+import com.fasterxml.jackson.annotation.JsonIgnore
+import com.fasterxml.jackson.core.`type`.TypeReference
+import com.fasterxml.jackson.databind.json.JsonMapper
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+
+import org.apache.spark.util.Utils
+
+/**
+ * Information associated with an error class.
+ *
+ * @param sqlState SQLSTATE associated with this class.
+ * @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]) {
Review comment:
This is a public API, it's better to use class as `case class` exposes to many APIs (apply, unapply, copy, etc.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-871134696
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-871069872
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/140378/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #32850: [SPARK-34920] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-858115489
**[Test build #139596 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/139596/testReport)** for PR 32850 at commit [`92e275f`](https://github.com/apache/spark/commit/92e275f501cfbd44245620b34744f23962e4cbde).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-858115489
**[Test build #139596 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/139596/testReport)** for PR 32850 at commit [`92e275f`](https://github.com/apache/spark/commit/92e275f501cfbd44245620b34744f23962e4cbde).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-871127360
**[Test build #140422 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/140422/testReport)** for PR 32850 at commit [`6d8e915`](https://github.com/apache/spark/commit/6d8e915ca62806f511588134a57bbeca99361d86).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] karenfeng commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
karenfeng commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r657460765
##########
File path: core/src/main/scala/org/apache/spark/SparkError.scala
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import java.net.URL
+
+import scala.collection.immutable.SortedMap
+
+import com.fasterxml.jackson.annotation.JsonIgnore
+import com.fasterxml.jackson.core.`type`.TypeReference
+import com.fasterxml.jackson.databind.SerializationFeature
+import com.fasterxml.jackson.databind.json.JsonMapper
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+
+import org.apache.spark.util.Utils
+
+/**
+ * Information associated with an error class.
+ *
+ * @param sqlState SQLSTATE associated with this class.
+ * @param messageFormatLines C-style message format compatible with printf.
+ * The error message is constructed by concatenating the lines with
+ * linebreaks.
+ */
+case class ErrorInfo(sqlState: Option[String], messageFormatLines: Seq[String]) {
Review comment:
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 `SparkException`s (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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-871085966
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44934/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-871098737
**[Test build #140419 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/140419/testReport)** for PR 32850 at commit [`d73bb83`](https://github.com/apache/spark/commit/d73bb83fa61ad551f634018f6e8cbf3d2b45842c).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r660337990
##########
File path: core/src/main/resources/error/README.md
##########
@@ -0,0 +1,89 @@
+# Guidelines
+
+To throw a standardized user-facing exception, developers should specify the error class and
+message parameters rather than an arbitrary error message.
+
+## Usage
+
+To throw an exception, do the following.
+
+1. Check if an appropriate error class already exists in `error-class.json`.
+ If true, skip to step 3. Otherwise, continue to step 2.
+2. Add a new class to `error-class.json`; keep in mind the invariants below.
+3. Check if the exception type already extends `SparkError`.
+ If true, skip to step 5. Otherwise, continue to step 4.
+4. Mix `SparkError` into the exception.
+5. Throw the exception with the error class and message parameters.
+
+### Before
+
+Throw exception:
+
+ throw new TestException("Problem A because B")
+
+
+### After
+
+`error-class.json`
+
+ "PROBLEM_BECAUSE": {
+ "sqlState": "XXXXX",
+ "message": ["Problem %s because %s"]
+ }
+
+`SparkException.scala`
+
+ class SparkTestException(
+ val errorClass: String,
+ val messageParameters: Seq[String])
+ extends TestException(SparkError.getMessage(errorClass, messageParameters))
+ with SparkError
+
+Throw exception:
+
+ throw new SparkTestException("PROBLEM_BECAUSE", Seq("A", "B"))
+
+## Access fields
+
+To access error fields, catch exceptions that extend `org.apache.spark.SparkError` and access
+ - Error class with `errorClass`
+ - SQLSTATE with `sqlState`
+
+
+ try {
+ ...
+ } catch {
+ case e: SparkError if e.sqlState.forall(_.startsWith("42")) =>
+ warn("Syntax error")
+ }
+
+## Fields
+
+### Error class
+
+Error classes are a succinct, human-readable representation of the error category.
+
+Invariants:
+
+- Unique
+- Consistent across releases
+- Sorted alphabetically
+
+### SQLSTATE
Review comment:
let's put it after `Message`, as it's optional
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-871105704
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44937/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-871108995
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] karenfeng commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
karenfeng commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r658917625
##########
File path: core/src/main/scala/org/apache/spark/SparkError.scala
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import java.net.URL
+
+import scala.collection.immutable.SortedMap
+
+import com.fasterxml.jackson.annotation.JsonIgnore
+import com.fasterxml.jackson.core.`type`.TypeReference
+import com.fasterxml.jackson.databind.SerializationFeature
+import com.fasterxml.jackson.databind.json.JsonMapper
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+
+import org.apache.spark.util.Utils
+
+/**
+ * Information associated with an error class.
+ *
+ * @param sqlState SQLSTATE associated with this class.
+ * @param messageFormatLines C-style message format compatible with printf.
+ * The error message is constructed by concatenating the lines with
+ * linebreaks.
+ */
+case class ErrorInfo(sqlState: Option[String], messageFormatLines: Seq[String]) {
Review comment:
Postgres calls their SQLSTATEs [error codes](https://www.postgresql.org/docs/13/errcodes-appendix.html), 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-867246501
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44754/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] karenfeng commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
karenfeng commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r655597628
##########
File path: 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)
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] karenfeng commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
karenfeng commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r657460765
##########
File path: core/src/main/scala/org/apache/spark/SparkError.scala
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import java.net.URL
+
+import scala.collection.immutable.SortedMap
+
+import com.fasterxml.jackson.annotation.JsonIgnore
+import com.fasterxml.jackson.core.`type`.TypeReference
+import com.fasterxml.jackson.databind.SerializationFeature
+import com.fasterxml.jackson.databind.json.JsonMapper
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+
+import org.apache.spark.util.Utils
+
+/**
+ * Information associated with an error class.
+ *
+ * @param sqlState SQLSTATE associated with this class.
+ * @param messageFormatLines C-style message format compatible with printf.
+ * The error message is constructed by concatenating the lines with
+ * linebreaks.
+ */
+case class ErrorInfo(sqlState: Option[String], messageFormatLines: Seq[String]) {
Review comment:
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 `SparkException`s (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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] karenfeng commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
karenfeng commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r655600869
##########
File path: core/src/main/scala/org/apache/spark/SparkError.scala
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import java.net.URL
+
+import scala.collection.immutable.SortedMap
+
+import com.fasterxml.jackson.annotation.JsonIgnore
+import com.fasterxml.jackson.core.`type`.TypeReference
+import com.fasterxml.jackson.databind.SerializationFeature
+import com.fasterxml.jackson.databind.json.JsonMapper
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+
+import org.apache.spark.util.Utils
+
+/**
+ * Information associated with an error class.
+ *
+ * @param sqlState SQLSTATE associated with this class.
+ * @param messageFormatLines C-style message format compatible with printf.
+ * The error message is constructed by concatenating the lines with
+ * linebreaks.
+ */
+case class ErrorInfo(sqlState: Option[String], messageFormatLines: Seq[String]) {
Review comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-871108997
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-858140680
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44123/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] karenfeng commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
karenfeng commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r659973018
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -0,0 +1,22 @@
+{
+ "DIVIDE_BY_ZERO" : {
+ "sqlState" : "22012",
+ "message" : [ "divide by zero" ]
+ },
Review comment:
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.
##########
File path: core/src/main/scala/org/apache/spark/SparkException.scala
##########
@@ -50,3 +65,16 @@ private[spark] case class ExecutorDeadException(message: String)
private[spark] class SparkUpgradeException(version: String, message: String, cause: Throwable)
extends RuntimeException("You may get a different result due to the upgrading of Spark" +
s" $version: $message", cause)
+
+/**
+ * Arithmetic exception thrown from Spark with an error class.
+ */
+private[spark] class SparkArithmeticException(
Review comment:
Good idea. Done - thanks for the review!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] xuanyuanking commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
xuanyuanking commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r658434031
##########
File path: core/src/main/scala/org/apache/spark/SparkError.scala
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import java.net.URL
+
+import scala.collection.immutable.SortedMap
+
+import com.fasterxml.jackson.annotation.JsonIgnore
+import com.fasterxml.jackson.core.`type`.TypeReference
+import com.fasterxml.jackson.databind.SerializationFeature
+import com.fasterxml.jackson.databind.json.JsonMapper
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+
+import org.apache.spark.util.Utils
+
+/**
+ * Information associated with an error class.
+ *
+ * @param sqlState SQLSTATE associated with this class.
+ * @param messageFormatLines C-style message format compatible with printf.
+ * The error message is constructed by concatenating the lines with
+ * linebreaks.
+ */
+case class ErrorInfo(sqlState: Option[String], messageFormatLines: Seq[String]) {
Review comment:
Yup, maybe we can consider giving a general name for `sqlState` (e.g. errorState, errorStateNumber, or etc).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r656221248
##########
File path: core/src/main/scala/org/apache/spark/SparkError.scala
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import java.net.URL
+
+import scala.collection.immutable.SortedMap
+
+import com.fasterxml.jackson.annotation.JsonIgnore
+import com.fasterxml.jackson.core.`type`.TypeReference
+import com.fasterxml.jackson.databind.SerializationFeature
+import com.fasterxml.jackson.databind.json.JsonMapper
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+
+import org.apache.spark.util.Utils
+
+/**
+ * Information associated with an error class.
+ *
+ * @param sqlState SQLSTATE associated with this class.
+ * @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]) {
+ // For compatibility with multi-line error messages
+ @JsonIgnore
+ val messageFormat: String = message.mkString("\n")
+}
+
+/**
+ * Companion object used by instances of [[SparkError]] to access error class information and
+ * construct error messages.
+ */
+object SparkError {
+ val errorClassesUrl: URL =
+ Utils.getSparkClassLoader.getResource("error/error-classes.json")
+ val mapper: JsonMapper = JsonMapper.builder()
Review comment:
nit: if `mapper` is only an intermediate value, let's not expose it and allow it to be GCed
```
val errorClassToInfoMap: SortedMap[String, ErrorInfo] = {
val mapper = ...
...
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r656226073
##########
File path: sql/core/src/test/resources/sql-tests/results/ansi/interval.sql.out
##########
@@ -67,7 +67,7 @@ select interval '2 seconds' / 0
-- !query schema
struct<>
-- !query output
-java.lang.ArithmeticException
+org.apache.spark.SparkArithmeticException
Review comment:
So we allow changing the exception type. Then can we make `SparkError` a concrete exception class and eventually throw `SparkError` everywhere?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #32850: [SPARK-34920] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-858165319
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r649969427
##########
File path: core/src/main/resources/error/README.md
##########
@@ -0,0 +1,79 @@
+# Guidelines
+
+To throw a standardized exception, developers should use an error class and message parameters
Review comment:
Shall we highlight "user-facing exception"? I think for assert-like errors we can still keep them unchanged in the Spark codebase.
##########
File path: core/src/main/resources/error/README.md
##########
@@ -0,0 +1,79 @@
+# Guidelines
+
+To throw a standardized exception, developers should use an error class and message parameters
+rather than an arbitrary error message.
+
+## Usage
+
+To throw an exception, do the following.
+
+1. Check if an appropriate error class already exists in `error-class.json`.
+ If true, skip to step 3. Otherwise, continue to step 2.
+2. Add a new class to `error-class.json`; keep in mind the invariants below.
+3. Check if the exception type already extends `SparkError`.
+ If true, skip to step 5. Otherwise, continue to step 4.
+4. Mix `SparkError` into the exception.
+5. Throw the exception with the error class and message parameters.
+
+### Before
+
+Throw exception:
+
+ throw new TestException("Problem A because B")
+
+
+### After
+
+`error-class.json`
+
+ "PROBLEM_BECAUSE": {
+ "sqlState": "XXXXX",
+ "messageFormatLines": ["Problem %s because %s"]
+ }
+
+`SparkException.scala`
+
+ class SparkTestException(
+ val errorClass: String,
+ val messageParameters: Seq[String])
+ extends TestException(SparkError.getMessage(errorClass, messageParameters))
+ with SparkError
+
+Throw exception:
+
+ throw new SparkTestException("PROBLEM_BECAUSE", Seq("A", "B"))
+
+## Access fields
+
+To access error fields, catch exceptions that extend `org.apache.spark.SparkError` and access
+ - Error class with `errorClass`
+ - SQLSTATE with `sqlState`
+
+
+ try {
+ ...
+ } catch {
+ case e: SparkError if e.sqlState.forall(_.startsWith("42")) =>
+ warn("Syntax error")
+ }
+
+## Fields
+
+All fields, excluding error messages, should be consistent across releases.
Review comment:
what do you mean by "consistent"? we can't change the error message in a new release?
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -0,0 +1,22 @@
+{
+ "DIVIDE_BY_ZERO_ERROR" : {
+ "sqlState" : "22012",
+ "messageFormatLines" : [ "divide by zero" ]
Review comment:
how about just naming it "message"?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-865360352
**[Test build #140097 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/140097/testReport)** for PR 32850 at commit [`bfdfc87`](https://github.com/apache/spark/commit/bfdfc87ce696fedbfd7deb5b556e7248972aab46).
* This patch **fails PySpark unit tests**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `case class ErrorInfo(sqlState: Option[String], message: Seq[String]) `
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangyum commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
wangyum commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-871125876
Last question. How do other common modules use these errors? For example: spark-unsafe, spark-network-common.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] karenfeng commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
karenfeng commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r660908892
##########
File path: core/src/main/scala/org/apache/spark/SparkError.scala
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import java.net.URL
+
+import scala.collection.immutable.SortedMap
+
+import com.fasterxml.jackson.annotation.JsonIgnore
+import com.fasterxml.jackson.core.`type`.TypeReference
+import com.fasterxml.jackson.databind.json.JsonMapper
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+
+import org.apache.spark.util.Utils
+
+/**
+ * Information associated with an error class.
+ *
+ * @param sqlState SQLSTATE associated with this class.
+ * @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]) {
Review comment:
Done; I think we can make `ErrorInfo` a private API for now as well. These fields should be accessed from the exception type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-867246509
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/44754/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-871134692
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] karenfeng commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
karenfeng commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r656403899
##########
File path: sql/core/src/test/resources/sql-tests/results/ansi/interval.sql.out
##########
@@ -67,7 +67,7 @@ select interval '2 seconds' / 0
-- !query schema
struct<>
-- !query output
-java.lang.ArithmeticException
+org.apache.spark.SparkArithmeticException
Review comment:
My goal here was to create a single trait that exposes the fields we're looking for, such as `sqlState`. By mixing it with base exception types like `java.lang.ArithmeticException` rather than creating a concrete exception class, I'm hoping that we can minimize backwards compatibility issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #32850: [SPARK-34920] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-858189870
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/139596/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-871238577
thanks, merging to master!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-865321108
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] karenfeng commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
karenfeng commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r655594657
##########
File path: core/src/main/resources/error/README.md
##########
@@ -0,0 +1,79 @@
+# Guidelines
+
+To throw a standardized exception, developers should use an error class and message parameters
+rather than an arbitrary error message.
+
+## Usage
+
+To throw an exception, do the following.
+
+1. Check if an appropriate error class already exists in `error-class.json`.
Review comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-870297019
retest this please
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-858154833
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44123/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r660339648
##########
File path: core/src/main/scala/org/apache/spark/SparkError.scala
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import java.net.URL
+
+import scala.collection.immutable.SortedMap
+
+import com.fasterxml.jackson.annotation.JsonIgnore
+import com.fasterxml.jackson.core.`type`.TypeReference
+import com.fasterxml.jackson.databind.json.JsonMapper
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+
+import org.apache.spark.util.Utils
+
+/**
+ * Information associated with an error class.
+ *
+ * @param sqlState SQLSTATE associated with this class.
+ * @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]) {
Review comment:
If this is a public API, it's better to use class as `case class` exposes too many APIs (apply, unapply, copy, etc.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r660337288
##########
File path: core/src/main/resources/error/README.md
##########
@@ -0,0 +1,89 @@
+# Guidelines
+
+To throw a standardized user-facing exception, developers should specify the error class and
+message parameters rather than an arbitrary error message.
+
+## Usage
+
+To throw an exception, do the following.
+
+1. Check if an appropriate error class already exists in `error-class.json`.
+ If true, skip to step 3. Otherwise, continue to step 2.
+2. Add a new class to `error-class.json`; keep in mind the invariants below.
+3. Check if the exception type already extends `SparkError`.
+ If true, skip to step 5. Otherwise, continue to step 4.
+4. Mix `SparkError` into the exception.
+5. Throw the exception with the error class and message parameters.
+
+### Before
+
+Throw exception:
+
+ throw new TestException("Problem A because B")
+
+
+### After
+
+`error-class.json`
+
+ "PROBLEM_BECAUSE": {
+ "sqlState": "XXXXX",
+ "message": ["Problem %s because %s"]
Review comment:
nit:
```
"PROBLEM_BECAUSE": {
"message": ["Problem %s because %s"],
"sqlState": "XXXXX" (this is optional)
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] karenfeng commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
karenfeng commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r660908892
##########
File path: core/src/main/scala/org/apache/spark/SparkError.scala
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark
+
+import java.net.URL
+
+import scala.collection.immutable.SortedMap
+
+import com.fasterxml.jackson.annotation.JsonIgnore
+import com.fasterxml.jackson.core.`type`.TypeReference
+import com.fasterxml.jackson.databind.json.JsonMapper
+import com.fasterxml.jackson.module.scala.DefaultScalaModule
+
+import org.apache.spark.util.Utils
+
+/**
+ * Information associated with an error class.
+ *
+ * @param sqlState SQLSTATE associated with this class.
+ * @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]) {
Review comment:
I think we can make `ErrorInfo` a private API. These fields should be accessed from the exception type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] karenfeng commented on a change in pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
karenfeng commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r655592044
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -0,0 +1,22 @@
+{
+ "DIVIDE_BY_ZERO_ERROR" : {
+ "sqlState" : "22012",
+ "messageFormatLines" : [ "divide by zero" ]
+ },
+ "DUPLICATE_KEY_ERROR" : {
+ "sqlState" : "23000",
+ "messageFormatLines" : [ "Found duplicate keys '%s'" ]
+ },
Review comment:
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](https://issues.apache.org/jira/browse/HIVE-3001), given that they are thrown within a `HiveSQLException`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] karenfeng commented on pull request #32850: [SPARK-34920] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
karenfeng commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-858161621
@wangyum, this is similar to your work in https://github.com/apache/spark/pull/32013. Can you take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] karenfeng commented on a change in pull request #32850: [SPARK-34920] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
karenfeng commented on a change in pull request #32850:
URL: https://github.com/apache/spark/pull/32850#discussion_r649275213
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -0,0 +1,22 @@
+{
+ "DIVIDE_BY_ZERO_ERROR" : {
+ "sqlState" : "22012",
+ "messageFormatLines" : [ "divide by zero" ]
+ },
+ "DUPLICATE_KEY_ERROR" : {
+ "sqlState" : "23000",
+ "messageFormatLines" : [ "Found duplicate keys '%s'" ]
+ },
+ "MISSING_COLUMN_ERROR" : {
+ "sqlState" : "42000",
+ "messageFormatLines" : [ "cannot resolve '%s' given input columns: [%s]" ]
+ },
+ "SECOND_FUNCTION_ARGUMENT_NOT_INTEGER_ERROR" : {
+ "sqlState" : "22023",
+ "messageFormatLines" : [ "The second argument of '%s' function needs to be an integer." ]
+ },
+ "WRITING_JOB_ABORTED_ERROR" : {
+ "sqlState" : "40000",
+ "messageFormatLines" : [ "Writing job aborted" ]
+ }
+}
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-858120270
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44122/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-871051277
**[Test build #140419 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/140419/testReport)** for PR 32850 at commit [`d73bb83`](https://github.com/apache/spark/commit/d73bb83fa61ad551f634018f6e8cbf3d2b45842c).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #32850: [SPARK-34920][CORE][SQL] Add error classes with SQLSTATE
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #32850:
URL: https://github.com/apache/spark/pull/32850#issuecomment-871051277
**[Test build #140419 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/140419/testReport)** for PR 32850 at commit [`d73bb83`](https://github.com/apache/spark/commit/d73bb83fa61ad551f634018f6e8cbf3d2b45842c).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org