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 2022/05/26 23:21:23 UTC
[GitHub] [spark] srielau opened a new pull request, #36693: CheckError
srielau opened a new pull request, #36693:
URL: https://github.com/apache/spark/pull/36693
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
--
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] gengliangwang commented on a diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r890443211
##########
core/src/test/scala/org/apache/spark/SparkFunSuite.scala:
##########
@@ -264,6 +264,81 @@ abstract class SparkFunSuite
}
}
+ /**
+ * Checks an exception with an error class against expected results.
+ * @param exception The exception to check
+ * @param errorClass The expected error class identifying the error
+ * @param errorSubClass Optional the expected subclass, None if not given
+ * @param sqlState Optional the expected SQLSTATE, not verified if not supplied
+ * @param parameters A map of parameter names and values. The names are as defined
+ * in the error-classes file.
+ */
+ protected def checkError(exception: Exception with SparkThrowable,
+ errorClass: String,
+ errorSubClass: Option[String],
+ sqlState: Option[String],
+ parameters: Map[String, String],
+ matchPVals: Boolean = false): Unit = {
Review Comment:
```suggestion
protected def checkError(
exception: Exception with SparkThrowable,
errorClass: String,
errorSubClass: Option[String],
sqlState: Option[String],
parameters: Map[String, String],
matchPVals: Boolean = false): Unit = {
```
--
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] srielau commented on a diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
srielau commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r891404260
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/AnalysisException.scala:
##########
@@ -36,13 +36,31 @@ class AnalysisException protected[sql] (
@transient val plan: Option[LogicalPlan] = None,
val cause: Option[Throwable] = None,
val errorClass: Option[String] = None,
+ val errorSubClass: Option[String] = None,
val messageParameters: Array[String] = Array.empty)
extends Exception(message, cause.orNull) with SparkThrowable with Serializable {
+ protected[sql] def this(message: String,
Review Comment:
You are correct
--
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] srielau commented on a diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
srielau commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r892641206
##########
core/src/main/scala/org/apache/spark/ErrorInfo.scala:
##########
@@ -73,18 +73,20 @@ private[spark] object SparkThrowableHelper {
def getMessage(
errorClass: String,
+ errorSubClass: Option[String],
Review Comment:
OK, testing...
--
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] HyukjinKwon commented on a diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r888508847
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/AnalysisException.scala:
##########
@@ -54,10 +72,34 @@ class AnalysisException protected[sql] (
messageParameters: Array[String],
origin: Origin) =
this(
- SparkThrowableHelper.getMessage(errorClass, messageParameters),
+ SparkThrowableHelper.getMessage(errorClass, None, messageParameters),
+ line = origin.line,
+ startPosition = origin.startPosition,
+ errorClass = Some(errorClass),
+ errorSubClass = None,
+ messageParameters = messageParameters)
+
+ def this(
+ errorClass: String,
+ errorSubClass: String,
+ messageParameters: Array[String]) =
Review Comment:
Just dropping a comment for a nit. maybe would have to be four spaces per https://github.com/databricks/scala-style-guide#spacing-and-indentation:
```scala
def this(
errorClass: String,
errorSubClass: String,
messageParameters: Array[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.
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 diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r891301726
##########
core/src/main/scala/org/apache/spark/ErrorInfo.scala:
##########
@@ -73,18 +73,20 @@ private[spark] object SparkThrowableHelper {
def getMessage(
errorClass: String,
+ errorSubClass: Option[String],
Review Comment:
since the error class name will never contain dots, how about we simplify the API and ask the caller side to pass `ERROR_CLASS.SUB_CLASS` as the `errorClass` parameter? e.g.
```
val errorClassArray = errorClass.split("\.")
... if (errorInfo.subClass.isEmpty) {
assert(errorClassArray.length == 1)
(errorClassArray.head, ...)
} else {
assert(errorClassArray.length == 2)
...
}
```
--
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] srielau commented on a diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
srielau commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r891395982
##########
core/src/test/scala/org/apache/spark/SparkFunSuite.scala:
##########
@@ -264,6 +264,87 @@ abstract class SparkFunSuite
}
}
+ /**
+ * Checks an exception with an error class against expected results.
+ * @param exception The exception to check
+ * @param errorClass The expected error class identifying the error
+ * @param errorSubClass Optional the expected subclass, None if not given
+ * @param sqlState Optional the expected SQLSTATE, not verified if not supplied
+ * @param parameters A map of parameter names and values. The names are as defined
+ * in the error-classes file.
Review Comment:
Done
--
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 diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r891301726
##########
core/src/main/scala/org/apache/spark/ErrorInfo.scala:
##########
@@ -73,18 +73,20 @@ private[spark] object SparkThrowableHelper {
def getMessage(
errorClass: String,
+ errorSubClass: Option[String],
Review Comment:
since the error class name will never contain dots, how about we simplify the API and ask the caller side to pass `ERROR_CLASS.SUB_CLASS` as the `errorClass` parameter? e.g.
```
val errorClassArray = errorClass.split("\.")
... if (errorInfo.subClass.isEmpty) {
assert(errorClassArray.length == 1)
(errorClassArray.head, ...)
} else {
assert(errorClassArray.length == 2)
...
}
```
--
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 diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r891320687
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/AnalysisException.scala:
##########
@@ -36,13 +36,31 @@ class AnalysisException protected[sql] (
@transient val plan: Option[LogicalPlan] = None,
val cause: Option[Throwable] = None,
val errorClass: Option[String] = None,
+ val errorSubClass: Option[String] = None,
val messageParameters: Array[String] = Array.empty)
extends Exception(message, cause.orNull) with SparkThrowable with Serializable {
+ protected[sql] def this(message: String,
Review Comment:
do we need this constructor? The main constructor is sufficient as `val errorSubClass: Option[String] = None` 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] gengliangwang commented on a diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r890373656
##########
core/src/main/java/org/apache/spark/memory/SparkOutOfMemoryError.java:
##########
@@ -28,6 +28,7 @@
@Private
public final class SparkOutOfMemoryError extends OutOfMemoryError implements SparkThrowable {
String errorClass;
+ String errorSubClass;
Review Comment:
It seems that the `errorSubClass` here is always null if there is no constructor method setting it.
##########
core/src/main/java/org/apache/spark/SparkThrowable.java:
##########
@@ -35,6 +35,9 @@ public interface SparkThrowable {
// Succinct, human-readable, unique, and consistent representation of the error category
// If null, error class is not set
String getErrorClass();
+ default String getErrorSubClass() {
Review Comment:
```suggestion
default String getErrorSubClass() {
```
##########
core/src/main/java/org/apache/spark/SparkThrowable.java:
##########
@@ -46,4 +49,13 @@ default String getSqlState() {
default boolean isInternalError() {
return SparkThrowableHelper.isInternalError(this.getErrorClass());
}
+
+ default String[] getMessageParameters() {
+ return new String[]{};
+ }
+
+ // True if this error is an internal error.
Review Comment:
We need to update the comment since it returns an array
##########
core/src/main/java/org/apache/spark/SparkThrowable.java:
##########
@@ -35,6 +35,9 @@ public interface SparkThrowable {
// Succinct, human-readable, unique, and consistent representation of the error category
// If null, error class is not set
String getErrorClass();
+ default String getErrorSubClass() {
Review Comment:
nit: Usually there needs a blank line between methods.
##########
core/src/main/java/org/apache/spark/SparkThrowable.java:
##########
@@ -35,6 +35,9 @@ public interface SparkThrowable {
// Succinct, human-readable, unique, and consistent representation of the error category
// If null, error class is not set
String getErrorClass();
+ default String getErrorSubClass() {
+ return null;
Review Comment:
Do we consider making the default value as empty string to avoid nullpointerexception?
--
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 diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r891292604
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -157,8 +157,7 @@
"See more details in SPARK-31404. You can set the SQL config <config> or",
"the datasource option <option> to \"LEGACY\" to rebase the datetime values",
"w.r.t. the calendar difference during reading. To read the datetime values",
- "as it is, set the SQL config <config> or the datasource option <option>",
- "to \"CORRECTED\"."
+ "as it is, set the SQL config or the datasource option t \"CORRECTED\"."
Review Comment:
```suggestion
"as it is, set the SQL config or the datasource option to \"CORRECTED\"."
```
--
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] srielau commented on pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
srielau commented on PR #36693:
URL: https://github.com/apache/spark/pull/36693#issuecomment-1147543765
@gengliangwang @cloud-fan Hoping for a 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] gengliangwang commented on a diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r890432585
##########
core/src/main/java/org/apache/spark/memory/SparkOutOfMemoryError.java:
##########
@@ -28,6 +28,7 @@
@Private
public final class SparkOutOfMemoryError extends OutOfMemoryError implements SparkThrowable {
String errorClass;
+ String errorSubClass;
Review Comment:
I mean we can skip adding `errorSubClass` and implementing `getErrorSubClass` in this PR. We can have it when there is sub 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] srielau commented on a diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
srielau commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r890411559
##########
core/src/main/java/org/apache/spark/SparkThrowable.java:
##########
@@ -35,6 +35,9 @@ public interface SparkThrowable {
// Succinct, human-readable, unique, and consistent representation of the error category
// If null, error class is not set
String getErrorClass();
+ default String getErrorSubClass() {
+ return null;
Review Comment:
I think this is mapped to an Option in scala... But you tell me... I'm all thumbs
--
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 pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on PR #36693:
URL: https://github.com/apache/spark/pull/36693#issuecomment-1150579622
The python doc issue is being fixed by https://github.com/apache/spark/pull/36813
--
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] srielau commented on a diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
srielau commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r891355010
##########
core/src/test/scala/org/apache/spark/SparkFunSuite.scala:
##########
@@ -264,6 +264,87 @@ abstract class SparkFunSuite
}
}
+ /**
+ * Checks an exception with an error class against expected results.
+ * @param exception The exception to check
+ * @param errorClass The expected error class identifying the error
+ * @param errorSubClass Optional the expected subclass, None if not given
+ * @param sqlState Optional the expected SQLSTATE, not verified if not supplied
+ * @param parameters A map of parameter names and values. The names are as defined
+ * in the error-classes file.
+ */
+ protected def checkError(
+ exception: Exception with SparkThrowable,
+ errorClass: String,
+ errorSubClass: Option[String],
+ sqlState: Option[String],
+ parameters: Map[String, String],
+ matchPVals: Boolean = false): Unit = {
+ assert(exception.getErrorClass === errorClass)
+ if (exception.getErrorSubClass != null) { assert(errorSubClass.isDefined) }
+ errorSubClass.foreach(subClass => assert(exception.getErrorSubClass === subClass))
+ sqlState.foreach(state => assert(exception.getSqlState === state))
+ val expectedParameters = (exception.getParameterNames zip exception.getMessageParameters).toMap
+ if (matchPVals == true) {
+ assert(expectedParameters.size === parameters.size)
+ expectedParameters.foreach(
+ exp => {
+ val parm = parameters.getOrElse(exp._1,
+ throw new IllegalArgumentException("Missing parameter" + exp._1))
+ if (!exp._2.matches(parm)) {
+ throw new IllegalArgumentException("(" + exp._1 + ", " + exp._2 +
+ ") does not match: " + parm)
+ }
+ }
+ )
+ } else {
+ assert(expectedParameters === parameters)
+ }
+ }
+
+ protected def checkError(
+ exception: Exception with SparkThrowable,
+ errorClass: String,
+ errorSubClass: String,
+ sqlState: String,
+ parameters: Map[String, String]): Unit =
+ checkError(exception, errorClass, Some(errorSubClass), Some(sqlState), parameters)
+
+ protected def checkError(
+ exception: Exception with SparkThrowable,
+ errorClass: String,
+ sqlState: String,
+ parameters: Map[String, String]): Unit =
+ checkError(exception, errorClass, None, Some(sqlState), parameters)
+
+ protected def checkError(
+ exception: Exception with SparkThrowable,
+ errorClass: String,
+ parameters: Map[String, String]): Unit =
+ checkError(exception, errorClass, None, None, parameters)
+
+ /**
+ * Checks an exception with an error class against expected results.
+ * @param exception The exception to check
+ * @param errorClass The expected error class identifying the error
+ * @param sqlState Optional the expected SQLSTATE, not verified if not supplied
+ * @param parameters An array of values. This does not verify the right name association.
+ */
+ protected def checkError(
+ exception: Exception with SparkThrowable,
+ errorClass: String,
+ sqlState: String,
+ parameters: Array[String]): Unit =
Review Comment:
I haven't thought about it either way. Do you want me to reduce the number of overloads?
--
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] gengliangwang commented on a diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r890391580
##########
core/src/main/scala/org/apache/spark/SparkException.scala:
##########
@@ -28,23 +28,47 @@ class SparkException(
message: String,
cause: Throwable,
errorClass: Option[String],
+ errorSubClass: Option[String],
messageParameters: Array[String])
extends Exception(message, cause) with SparkThrowable {
+ def this(message: String,
+ cause: Throwable,
+ errorClass: Option[String],
+ messageParameters: Array[String]) =
Review Comment:
```suggestion
def this(
message: String,
cause: Throwable,
errorClass: Option[String],
messageParameters: Array[String]) =
```
##########
core/src/main/scala/org/apache/spark/SparkException.scala:
##########
@@ -28,23 +28,47 @@ class SparkException(
message: String,
cause: Throwable,
errorClass: Option[String],
+ errorSubClass: Option[String],
messageParameters: Array[String])
extends Exception(message, cause) with SparkThrowable {
+ def this(message: String,
+ cause: Throwable,
+ errorClass: Option[String],
+ messageParameters: Array[String]) =
Review Comment:
nit comment for scala style
--
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 diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r891312375
##########
core/src/test/scala/org/apache/spark/SparkFunSuite.scala:
##########
@@ -264,6 +264,87 @@ abstract class SparkFunSuite
}
}
+ /**
+ * Checks an exception with an error class against expected results.
+ * @param exception The exception to check
+ * @param errorClass The expected error class identifying the error
+ * @param errorSubClass Optional the expected subclass, None if not given
+ * @param sqlState Optional the expected SQLSTATE, not verified if not supplied
+ * @param parameters A map of parameter names and values. The names are as defined
+ * in the error-classes file.
Review Comment:
can we add doc for `matchPVals` as well?
--
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] srielau commented on a diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
srielau commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r891425678
##########
core/src/main/java/org/apache/spark/memory/SparkOutOfMemoryError.java:
##########
@@ -39,11 +39,17 @@ public SparkOutOfMemoryError(OutOfMemoryError e) {
}
public SparkOutOfMemoryError(String errorClass, String[] messageParameters) {
- super(SparkThrowableHelper.getMessage(errorClass, messageParameters, ""));
+ super(SparkThrowableHelper.getMessage(errorClass, null,
Review Comment:
Declined, it's java
--
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] srielau commented on a diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
srielau commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r891373947
##########
core/src/main/java/org/apache/spark/memory/SparkOutOfMemoryError.java:
##########
@@ -39,11 +39,17 @@ public SparkOutOfMemoryError(OutOfMemoryError e) {
}
public SparkOutOfMemoryError(String errorClass, String[] messageParameters) {
- super(SparkThrowableHelper.getMessage(errorClass, messageParameters, ""));
+ super(SparkThrowableHelper.getMessage(errorClass, null,
Review Comment:
@cloud-fan This is Java, does it have None?
--
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 diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r892535272
##########
core/src/main/scala/org/apache/spark/ErrorInfo.scala:
##########
@@ -73,18 +73,20 @@ private[spark] object SparkThrowableHelper {
def getMessage(
errorClass: String,
+ errorSubClass: Option[String],
Review Comment:
since this method can be called in Java, using nullable `String` is better here? e.g. https://github.com/apache/spark/pull/36693/files#diff-82cc1e88d98fb17768fdc4a36035aac5e4b4b2aa797ec5c4e14860e3be7be4ccR42
--
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] srielau commented on a diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
srielau commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r890410763
##########
core/src/main/java/org/apache/spark/memory/SparkOutOfMemoryError.java:
##########
@@ -28,6 +28,7 @@
@Private
public final class SparkOutOfMemoryError extends OutOfMemoryError implements SparkThrowable {
String errorClass;
+ String errorSubClass;
Review Comment:
I think that's correct. At this point Out Of Memory has no subclass.
--
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] srielau commented on a diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
srielau commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r890413958
##########
core/src/main/java/org/apache/spark/SparkThrowable.java:
##########
@@ -46,4 +49,13 @@ default String getSqlState() {
default boolean isInternalError() {
return SparkThrowableHelper.isInternalError(this.getErrorClass());
}
+
+ default String[] getMessageParameters() {
+ return new String[]{};
+ }
+
+ // True if this error is an internal error.
Review Comment:
```suggestion
// Returns a string array of all parameters that need to be passed to this error message
```
##########
core/src/main/java/org/apache/spark/SparkThrowable.java:
##########
@@ -46,4 +49,13 @@ default String getSqlState() {
default boolean isInternalError() {
return SparkThrowableHelper.isInternalError(this.getErrorClass());
}
+
+ default String[] getMessageParameters() {
+ return new String[]{};
+ }
+
+ // True if this error is an internal error.
Review Comment:
```suggestion
// Returns a string array of all parameters that need to be passed to this error 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.
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] srielau commented on a diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
srielau commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r890413958
##########
core/src/main/java/org/apache/spark/SparkThrowable.java:
##########
@@ -46,4 +49,13 @@ default String getSqlState() {
default boolean isInternalError() {
return SparkThrowableHelper.isInternalError(this.getErrorClass());
}
+
+ default String[] getMessageParameters() {
+ return new String[]{};
+ }
+
+ // True if this error is an internal error.
Review Comment:
```suggestion
// Returns a string array of all parameters that need to be passed to this error 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.
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 diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r891297431
##########
core/src/main/java/org/apache/spark/memory/SparkOutOfMemoryError.java:
##########
@@ -39,11 +39,17 @@ public SparkOutOfMemoryError(OutOfMemoryError e) {
}
public SparkOutOfMemoryError(String errorClass, String[] messageParameters) {
- super(SparkThrowableHelper.getMessage(errorClass, messageParameters, ""));
+ super(SparkThrowableHelper.getMessage(errorClass, null,
Review Comment:
```suggestion
super(SparkThrowableHelper.getMessage(errorClass, None,
```
--
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] srielau commented on a diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
srielau commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r891349846
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -333,7 +332,7 @@
},
"SECOND_FUNCTION_ARGUMENT_NOT_INTEGER" : {
"message" : [
- "The second argument of '<functionName>' function needs to be an integer."
+ "The second argument of <functionName> function needs to be an integer."
Review Comment:
It should be back-ticked. But @MaxGekk is doing that as part of the parameter value.
If I leave the tick it will be: '`default`.`foo`'
--
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 diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r891309492
##########
core/src/test/scala/org/apache/spark/SparkFunSuite.scala:
##########
@@ -264,6 +264,87 @@ abstract class SparkFunSuite
}
}
+ /**
+ * Checks an exception with an error class against expected results.
+ * @param exception The exception to check
+ * @param errorClass The expected error class identifying the error
+ * @param errorSubClass Optional the expected subclass, None if not given
+ * @param sqlState Optional the expected SQLSTATE, not verified if not supplied
+ * @param parameters A map of parameter names and values. The names are as defined
+ * in the error-classes file.
+ */
+ protected def checkError(
+ exception: Exception with SparkThrowable,
Review Comment:
```suggestion
exception: SparkThrowable,
```
--
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] srielau commented on a diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
srielau commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r891389538
##########
core/src/main/scala/org/apache/spark/ErrorInfo.scala:
##########
@@ -98,6 +100,29 @@ private[spark] object SparkThrowableHelper {
s"[$displayClass] $displayMessage$displayQueryContext"
}
+ def getParameterNames(errorClass: String, errorSubCLass: String): Array[String] = {
Review Comment:
Java...
--
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 diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r891314795
##########
core/src/test/scala/org/apache/spark/SparkFunSuite.scala:
##########
@@ -264,6 +264,87 @@ abstract class SparkFunSuite
}
}
+ /**
+ * Checks an exception with an error class against expected results.
+ * @param exception The exception to check
+ * @param errorClass The expected error class identifying the error
+ * @param errorSubClass Optional the expected subclass, None if not given
+ * @param sqlState Optional the expected SQLSTATE, not verified if not supplied
+ * @param parameters A map of parameter names and values. The names are as defined
+ * in the error-classes file.
+ */
+ protected def checkError(
+ exception: Exception with SparkThrowable,
+ errorClass: String,
+ errorSubClass: Option[String],
+ sqlState: Option[String],
+ parameters: Map[String, String],
+ matchPVals: Boolean = false): Unit = {
+ assert(exception.getErrorClass === errorClass)
+ if (exception.getErrorSubClass != null) { assert(errorSubClass.isDefined) }
+ errorSubClass.foreach(subClass => assert(exception.getErrorSubClass === subClass))
+ sqlState.foreach(state => assert(exception.getSqlState === state))
+ val expectedParameters = (exception.getParameterNames zip exception.getMessageParameters).toMap
+ if (matchPVals == true) {
+ assert(expectedParameters.size === parameters.size)
+ expectedParameters.foreach(
+ exp => {
+ val parm = parameters.getOrElse(exp._1,
+ throw new IllegalArgumentException("Missing parameter" + exp._1))
+ if (!exp._2.matches(parm)) {
+ throw new IllegalArgumentException("(" + exp._1 + ", " + exp._2 +
+ ") does not match: " + parm)
+ }
+ }
+ )
+ } else {
+ assert(expectedParameters === parameters)
+ }
+ }
+
+ protected def checkError(
+ exception: Exception with SparkThrowable,
+ errorClass: String,
+ errorSubClass: String,
+ sqlState: String,
+ parameters: Map[String, String]): Unit =
+ checkError(exception, errorClass, Some(errorSubClass), Some(sqlState), parameters)
+
+ protected def checkError(
+ exception: Exception with SparkThrowable,
+ errorClass: String,
+ sqlState: String,
+ parameters: Map[String, String]): Unit =
+ checkError(exception, errorClass, None, Some(sqlState), parameters)
+
+ protected def checkError(
+ exception: Exception with SparkThrowable,
+ errorClass: String,
+ parameters: Map[String, String]): Unit =
+ checkError(exception, errorClass, None, None, parameters)
+
+ /**
+ * Checks an exception with an error class against expected results.
+ * @param exception The exception to check
+ * @param errorClass The expected error class identifying the error
+ * @param sqlState Optional the expected SQLSTATE, not verified if not supplied
+ * @param parameters An array of values. This does not verify the right name association.
+ */
+ protected def checkError(
+ exception: Exception with SparkThrowable,
+ errorClass: String,
+ sqlState: String,
+ parameters: Array[String]): Unit =
Review Comment:
is it temporary and will be removed in the future?
--
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] gengliangwang commented on a diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r890392259
##########
core/src/main/scala/org/apache/spark/SparkException.scala:
##########
@@ -28,23 +28,47 @@ class SparkException(
message: String,
cause: Throwable,
errorClass: Option[String],
+ errorSubClass: Option[String],
messageParameters: Array[String])
extends Exception(message, cause) with SparkThrowable {
+ def this(message: String,
+ cause: Throwable,
+ errorClass: Option[String],
+ messageParameters: Array[String]) =
+ this(message = message,
+ cause = cause,
+ errorClass = errorClass,
+ errorSubClass = None,
+ messageParameters = messageParameters)
+
def this(message: String, cause: Throwable) =
- this(message = message, cause = cause, errorClass = None, messageParameters = Array.empty)
+ this(message = message, cause = cause, errorClass = None, errorSubClass = None,
+ messageParameters = Array.empty)
def this(message: String) =
this(message = message, cause = null)
def this(errorClass: String, messageParameters: Array[String], cause: Throwable) =
this(
- message = SparkThrowableHelper.getMessage(errorClass, messageParameters),
+ message = SparkThrowableHelper.getMessage(errorClass, None, messageParameters),
+ cause = cause,
+ errorClass = Some(errorClass),
+ errorSubClass = None,
+ messageParameters = messageParameters)
+
+ def this(errorClass: String, errorSubClass: String,
+ messageParameters: Array[String], cause: Throwable) =
Review Comment:
```suggestion
def this(
errorClass: String,
errorSubClass: String,
messageParameters: Array[String],
cause: Throwable) =
```
--
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 diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r891290994
##########
core/src/main/java/org/apache/spark/SparkThrowable.java:
##########
@@ -36,6 +36,10 @@ public interface SparkThrowable {
// If null, error class is not set
String getErrorClass();
+ default String getErrorSubClass() {
Review Comment:
```suggestion
default String getErrorSubClass() {
```
--
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 diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r892541582
##########
core/src/main/scala/org/apache/spark/SparkException.scala:
##########
@@ -73,236 +101,340 @@ private[spark] case class ExecutorDeadException(message: String)
*/
private[spark] class SparkUpgradeException(
errorClass: String,
+ errorSubClass: Option[String] = None,
messageParameters: Array[String],
cause: Throwable)
- extends RuntimeException(SparkThrowableHelper.getMessage(errorClass, messageParameters), cause)
+ extends RuntimeException(SparkThrowableHelper.getMessage(errorClass, errorSubClass,
+ messageParameters), cause)
with SparkThrowable {
+ override def getMessageParameters: Array[String] = messageParameters
override def getErrorClass: String = errorClass
-}
+ override def getErrorSubClass: String = errorSubClass.orNull}
/**
* Arithmetic exception thrown from Spark with an error class.
*/
private[spark] class SparkArithmeticException(
errorClass: String,
+ errorSubClass: Option[String] = None,
messageParameters: Array[String],
queryContext: String = "")
extends ArithmeticException(
- SparkThrowableHelper.getMessage(errorClass, messageParameters, queryContext))
+ SparkThrowableHelper.getMessage(errorClass, errorSubClass, messageParameters, queryContext))
with SparkThrowable {
+ override def getMessageParameters: Array[String] = messageParameters
override def getErrorClass: String = errorClass
-}
+ override def getErrorSubClass: String = errorSubClass.orNull}
/**
* Unsupported operation exception thrown from Spark with an error class.
*/
private[spark] class SparkUnsupportedOperationException(
errorClass: String,
+ errorSubClass: Option[String] = None,
messageParameters: Array[String])
extends UnsupportedOperationException(
- SparkThrowableHelper.getMessage(errorClass, messageParameters)) with SparkThrowable {
+ SparkThrowableHelper.getMessage(errorClass, errorSubClass, messageParameters))
+ with SparkThrowable {
+ def this(
+ errorClass: String,
+ errorSubClass: String,
+ messageParameters: Array[String]) =
Review Comment:
```suggestion
def this(
errorClass: String,
errorSubClass: String,
messageParameters: Array[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.
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] srielau commented on a diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
srielau commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r891606582
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/AnalysisException.scala:
##########
@@ -36,13 +36,31 @@ class AnalysisException protected[sql] (
@transient val plan: Option[LogicalPlan] = None,
val cause: Option[Throwable] = None,
val errorClass: Option[String] = None,
+ val errorSubClass: Option[String] = None,
val messageParameters: Array[String] = Array.empty)
extends Exception(message, cause.orNull) with SparkThrowable with Serializable {
+ protected[sql] def this(message: String,
Review Comment:
It's need for binary 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.
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 diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r891302240
##########
core/src/main/scala/org/apache/spark/ErrorInfo.scala:
##########
@@ -98,6 +100,29 @@ private[spark] object SparkThrowableHelper {
s"[$displayClass] $displayMessage$displayQueryContext"
}
+ def getParameterNames(errorClass: String, errorSubCLass: String): Array[String] = {
Review Comment:
ditto
--
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 diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r891302240
##########
core/src/main/scala/org/apache/spark/ErrorInfo.scala:
##########
@@ -98,6 +100,29 @@ private[spark] object SparkThrowableHelper {
s"[$displayClass] $displayMessage$displayQueryContext"
}
+ def getParameterNames(errorClass: String, errorSubCLass: String): Array[String] = {
Review Comment:
should `errorSubClass` be `Option[String]` to be consistent with `getMessage`?
--
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 pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on PR #36693:
URL: https://github.com/apache/spark/pull/36693#issuecomment-1150579697
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] gengliangwang commented on a diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r890433533
##########
core/src/main/java/org/apache/spark/SparkThrowable.java:
##########
@@ -35,6 +35,9 @@ public interface SparkThrowable {
// Succinct, human-readable, unique, and consistent representation of the error category
// If null, error class is not set
String getErrorClass();
+ default String getErrorSubClass() {
+ return null;
Review Comment:
Oh I am fine with either "null" or empty string. I was just wondering which is better.
--
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] srielau commented on a diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
srielau commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r890414735
##########
core/src/main/java/org/apache/spark/SparkThrowable.java:
##########
@@ -46,4 +49,13 @@ default String getSqlState() {
default boolean isInternalError() {
return SparkThrowableHelper.isInternalError(this.getErrorClass());
}
+
+ default String[] getMessageParameters() {
+ return new String[]{};
+ }
+
+ // True if this error is an internal error.
Review Comment:
Done
--
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 #36693: CheckError
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #36693:
URL: https://github.com/apache/spark/pull/36693#issuecomment-1139407990
Can one of the admins verify this patch?
--
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] gengliangwang commented on a diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
gengliangwang commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r890447000
##########
core/src/test/scala/org/apache/spark/SparkFunSuite.scala:
##########
@@ -264,6 +264,81 @@ abstract class SparkFunSuite
}
}
+ /**
+ * Checks an exception with an error class against expected results.
+ * @param exception The exception to check
+ * @param errorClass The expected error class identifying the error
+ * @param errorSubClass Optional the expected subclass, None if not given
+ * @param sqlState Optional the expected SQLSTATE, not verified if not supplied
+ * @param parameters A map of parameter names and values. The names are as defined
+ * in the error-classes file.
+ */
+ protected def checkError(exception: Exception with SparkThrowable,
+ errorClass: String,
+ errorSubClass: Option[String],
+ sqlState: Option[String],
+ parameters: Map[String, String],
+ matchPVals: Boolean = false): Unit = {
+ assert(exception.getErrorClass === errorClass)
+ if (exception.getErrorSubClass != null) { assert(errorSubClass.isDefined) }
+ errorSubClass.foreach(subClass => assert(exception.getErrorSubClass === subClass))
+ sqlState.foreach(state => assert(exception.getSqlState === state))
+ val expectedParameters = (exception.getParameterNames zip exception.getMessageParameters).toMap
+ if (matchPVals == true) {
+ assert(expectedParameters.size === parameters.size)
+ expectedParameters.foreach(
+ exp => {
+ val parm = parameters.getOrElse(exp._1,
+ throw new IllegalArgumentException("Missing parameter" + exp._1))
+ if (!exp._2.matches(parm)) {
+ throw new IllegalArgumentException("(" + exp._1 + ", " + exp._2 +
+ ") does not match: " + parm)
+ }
+ }
+ )
+ } else {
+ assert(expectedParameters === parameters)
+ }
+ }
+
+ protected def checkError(exception: Exception with SparkThrowable,
+ errorClass: String,
+ errorSubClass: String,
+ sqlState: String,
+ parameters: Map[String, String]): Unit =
+ checkError(exception, errorClass, Some(errorSubClass), Some(sqlState), parameters)
+
+ protected def checkError(exception: Exception with SparkThrowable,
+ errorClass: String,
+ sqlState: String,
+ parameters: Map[String, String]): Unit =
+ checkError(exception, errorClass, None, Some(sqlState), parameters)
+
+ protected def checkError(exception: Exception with SparkThrowable,
+ errorClass: String,
+ parameters: Map[String, String]): Unit =
+ checkError(exception, errorClass, None, None, parameters)
+
+ /**
+ * Checks an exception with an error class against expected results.
+ * @param exception The exception to check
+ * @param errorClass The expected error class identifying the error
+ * @param sqlState Optional the expected SQLSTATE, not verified if not supplied
+ * @param parameters An array of values. This does not verify the right name association.
+ */
+ protected def checkError(exception: Exception with SparkThrowable,
+ errorClass: String,
+ sqlState: String,
+ parameters: Array[String]): Unit =
+ checkError(exception, errorClass, None, Some(sqlState),
+ (exception.getParameterNames zip parameters).toMap)
+
+ protected def checkError(exception: Exception with SparkThrowable,
+ errorClass: String,
+ parameters: Array[String]): Unit =
Review Comment:
```suggestion
protected def checkError(
exception: Exception with SparkThrowable,
errorClass: String,
errorSubClass: String,
sqlState: String,
parameters: Map[String, String]): Unit =
checkError(exception, errorClass, Some(errorSubClass), Some(sqlState), parameters)
protected def checkError(
exception: Exception with SparkThrowable,
errorClass: String,
sqlState: String,
parameters: Map[String, String]): Unit =
checkError(exception, errorClass, None, Some(sqlState), parameters)
protected def checkError(
exception: Exception with SparkThrowable,
errorClass: String,
parameters: Map[String, String]): Unit =
checkError(exception, errorClass, None, None, parameters)
/**
* Checks an exception with an error class against expected results.
* @param exception The exception to check
* @param errorClass The expected error class identifying the error
* @param sqlState Optional the expected SQLSTATE, not verified if not supplied
* @param parameters An array of values. This does not verify the right name association.
*/
protected def checkError(
exception: Exception with SparkThrowable,
errorClass: String,
sqlState: String,
parameters: Array[String]): Unit =
checkError(exception, errorClass, None, Some(sqlState),
(exception.getParameterNames zip parameters).toMap)
protected def checkError(
exception: Exception with SparkThrowable,
errorClass: String,
parameters: Array[String]): Unit =
```
--
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] srielau commented on a diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
srielau commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r891425369
##########
core/src/test/scala/org/apache/spark/SparkFunSuite.scala:
##########
@@ -264,6 +264,87 @@ abstract class SparkFunSuite
}
}
+ /**
+ * Checks an exception with an error class against expected results.
+ * @param exception The exception to check
+ * @param errorClass The expected error class identifying the error
+ * @param errorSubClass Optional the expected subclass, None if not given
+ * @param sqlState Optional the expected SQLSTATE, not verified if not supplied
+ * @param parameters A map of parameter names and values. The names are as defined
+ * in the error-classes file.
+ */
+ protected def checkError(
+ exception: Exception with SparkThrowable,
+ errorClass: String,
+ errorSubClass: Option[String],
+ sqlState: Option[String],
+ parameters: Map[String, String],
+ matchPVals: Boolean = false): Unit = {
+ assert(exception.getErrorClass === errorClass)
+ if (exception.getErrorSubClass != null) { assert(errorSubClass.isDefined) }
+ errorSubClass.foreach(subClass => assert(exception.getErrorSubClass === subClass))
+ sqlState.foreach(state => assert(exception.getSqlState === state))
+ val expectedParameters = (exception.getParameterNames zip exception.getMessageParameters).toMap
+ if (matchPVals == true) {
+ assert(expectedParameters.size === parameters.size)
+ expectedParameters.foreach(
+ exp => {
+ val parm = parameters.getOrElse(exp._1,
+ throw new IllegalArgumentException("Missing parameter" + exp._1))
+ if (!exp._2.matches(parm)) {
+ throw new IllegalArgumentException("(" + exp._1 + ", " + exp._2 +
+ ") does not match: " + parm)
+ }
+ }
+ )
+ } else {
+ assert(expectedParameters === parameters)
+ }
+ }
+
+ protected def checkError(
+ exception: Exception with SparkThrowable,
+ errorClass: String,
+ errorSubClass: String,
+ sqlState: String,
+ parameters: Map[String, String]): Unit =
+ checkError(exception, errorClass, Some(errorSubClass), Some(sqlState), parameters)
+
+ protected def checkError(
+ exception: Exception with SparkThrowable,
+ errorClass: String,
+ sqlState: String,
+ parameters: Map[String, String]): Unit =
+ checkError(exception, errorClass, None, Some(sqlState), parameters)
+
+ protected def checkError(
+ exception: Exception with SparkThrowable,
+ errorClass: String,
+ parameters: Map[String, String]): Unit =
+ checkError(exception, errorClass, None, None, parameters)
+
+ /**
+ * Checks an exception with an error class against expected results.
+ * @param exception The exception to check
+ * @param errorClass The expected error class identifying the error
+ * @param sqlState Optional the expected SQLSTATE, not verified if not supplied
+ * @param parameters An array of values. This does not verify the right name association.
+ */
+ protected def checkError(
+ exception: Exception with SparkThrowable,
+ errorClass: String,
+ sqlState: String,
+ parameters: Array[String]): Unit =
Review Comment:
Removed
--
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 diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r891293373
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -333,7 +332,7 @@
},
"SECOND_FUNCTION_ARGUMENT_NOT_INTEGER" : {
"message" : [
- "The second argument of '<functionName>' function needs to be an integer."
+ "The second argument of <functionName> function needs to be an integer."
Review Comment:
hmm, is it an identifier and should be quoted?
--
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 diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r891311339
##########
core/src/test/scala/org/apache/spark/SparkFunSuite.scala:
##########
@@ -264,6 +264,87 @@ abstract class SparkFunSuite
}
}
+ /**
+ * Checks an exception with an error class against expected results.
+ * @param exception The exception to check
+ * @param errorClass The expected error class identifying the error
+ * @param errorSubClass Optional the expected subclass, None if not given
+ * @param sqlState Optional the expected SQLSTATE, not verified if not supplied
+ * @param parameters A map of parameter names and values. The names are as defined
+ * in the error-classes file.
+ */
+ protected def checkError(
+ exception: Exception with SparkThrowable,
+ errorClass: String,
+ errorSubClass: Option[String],
+ sqlState: Option[String],
+ parameters: Map[String, String],
+ matchPVals: Boolean = false): Unit = {
+ assert(exception.getErrorClass === errorClass)
+ if (exception.getErrorSubClass != null) { assert(errorSubClass.isDefined) }
+ errorSubClass.foreach(subClass => assert(exception.getErrorSubClass === subClass))
Review Comment:
```suggestion
if (exception.getErrorSubClass != null) {
assert(errorSubClass.isDefined)
assert(exception.getErrorSubClass === errorSubClass.get)
}
errorSubClass.foreach(subClass => assert(exception.getErrorSubClass === subClass))
```
##########
core/src/test/scala/org/apache/spark/SparkFunSuite.scala:
##########
@@ -264,6 +264,87 @@ abstract class SparkFunSuite
}
}
+ /**
+ * Checks an exception with an error class against expected results.
+ * @param exception The exception to check
+ * @param errorClass The expected error class identifying the error
+ * @param errorSubClass Optional the expected subclass, None if not given
+ * @param sqlState Optional the expected SQLSTATE, not verified if not supplied
+ * @param parameters A map of parameter names and values. The names are as defined
+ * in the error-classes file.
+ */
+ protected def checkError(
+ exception: Exception with SparkThrowable,
+ errorClass: String,
+ errorSubClass: Option[String],
+ sqlState: Option[String],
+ parameters: Map[String, String],
+ matchPVals: Boolean = false): Unit = {
+ assert(exception.getErrorClass === errorClass)
+ if (exception.getErrorSubClass != null) { assert(errorSubClass.isDefined) }
+ errorSubClass.foreach(subClass => assert(exception.getErrorSubClass === subClass))
Review Comment:
```suggestion
if (exception.getErrorSubClass != null) {
assert(errorSubClass.isDefined)
assert(exception.getErrorSubClass === errorSubClass.get)
}
```
--
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 diff in pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36693:
URL: https://github.com/apache/spark/pull/36693#discussion_r891304223
##########
core/src/main/scala/org/apache/spark/ErrorInfo.scala:
##########
@@ -73,18 +73,20 @@ private[spark] object SparkThrowableHelper {
def getMessage(
errorClass: String,
+ errorSubClass: Option[String],
Review Comment:
By doing this, I think we can avoid many code changes in this PR.
--
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 #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #36693: [SPARK-39349] Add a centralized CheckError method for QA of error path
URL: https://github.com/apache/spark/pull/36693
--
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