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/10/16 11:16:33 UTC
[GitHub] [spark] khalidmammadov opened a new pull request, #38273: [SPARK-37945][SQL][CATALYST] Use error classes in the execution errors of arithmetic ops
khalidmammadov opened a new pull request, #38273:
URL: https://github.com/apache/spark/pull/38273
### What changes were proposed in this pull request?
Migrate the following errors in QueryExecutionErrors onto use error classes:
unscaledValueTooLargeForPrecisionError -> UNSCALED_VALUE_TOO_LARGE_FOR_PRECISION
decimalPrecisionExceedsMaxPrecisionError -> DECIMAL_PRECISION_EXCEEDS_MAX_PRECISION
integerOverflowError -> INTEGER_OVERFLOW
outOfDecimalTypeRangeError -> OUT_OF_DECIMAL_TYPE_RANGE
### Why are the changes needed?
Porting ArithmeticExceptions to the new error framework
### Does this PR introduce _any_ user-facing change?
Yes, errors will indicate that it's controlled Spark exception
### How was this patch tested?
./build/sbt "catalyst/testOnly org.apache.spark.sql.types.DecimalSuite"
./build/sbt "sql/testOnly org.apache.spark.sql.execution.streaming.sources.RateStreamProviderSuite"
./build/sbt "core/testOnly testOnly org.apache.spark.SparkThrowableSuite"
--
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] MaxGekk commented on a diff in pull request #38273: [SPARK-37945][SQL][CORE] Use error classes in the execution errors of arithmetic ops
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #38273:
URL: https://github.com/apache/spark/pull/38273#discussion_r996481213
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -256,6 +256,11 @@
],
"sqlState" : "22008"
},
+ "DECIMAL_PRECISION_EXCEEDS_MAX_PRECISION" : {
Review Comment:
> I can work on changing those cases to assert additionally the class name/msg using checkError ...
Please, do that. The purpose is to make the tests independent from error messages (only valuable message parameters), so, in this way tech editors could edit error message in error-classes.json and don't worry about internal Spark tests.
--
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] amaliujia commented on a diff in pull request #38273: [SPARK-37945][SQL][CORE] Use error classes in the execution errors of arithmetic ops
Posted by GitBox <gi...@apache.org>.
amaliujia commented on code in PR #38273:
URL: https://github.com/apache/spark/pull/38273#discussion_r996476109
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -256,6 +256,11 @@
],
"sqlState" : "22008"
},
+ "DECIMAL_PRECISION_EXCEEDS_MAX_PRECISION" : {
Review Comment:
I am pretty sure there is one already: _LEGACY_ERROR_TEMP_2118.
Please double check before adding more new error messages.
--
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 #38273: [SPARK-37945][SQL][CORE] Use error classes in the execution errors of arithmetic ops
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #38273:
URL: https://github.com/apache/spark/pull/38273#issuecomment-1280478206
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] MaxGekk closed pull request #38273: [SPARK-37945][SQL][CORE] Use error classes in the execution errors of arithmetic ops
Posted by GitBox <gi...@apache.org>.
MaxGekk closed pull request #38273: [SPARK-37945][SQL][CORE] Use error classes in the execution errors of arithmetic ops
URL: https://github.com/apache/spark/pull/38273
--
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] MaxGekk commented on pull request #38273: [SPARK-37945][SQL][CORE] Use error classes in the execution errors of arithmetic ops
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on PR #38273:
URL: https://github.com/apache/spark/pull/38273#issuecomment-1287797132
@khalidmammadov Could you re-trigger tests/builds by merging the recent master, 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] MaxGekk commented on pull request #38273: [SPARK-37945][SQL][CORE] Use error classes in the execution errors of arithmetic ops
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on PR #38273:
URL: https://github.com/apache/spark/pull/38273#issuecomment-1288026359
+1, LGTM. Merging to master.
Thank you, @khalidmammadov and @amaliujia for 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] MaxGekk commented on a diff in pull request #38273: [SPARK-37945][SQL][CORE] Use error classes in the execution errors of arithmetic ops
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #38273:
URL: https://github.com/apache/spark/pull/38273#discussion_r998048906
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -551,6 +561,11 @@
],
"sqlState" : "22005"
},
+ "OUT_OF_DECIMAL_TYPE_RANGE" : {
+ "message" : [
+ "out of decimal type range: <value>"
Review Comment:
nit: Please, upper case the first letter to be consistent w/ other error messages.
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -686,6 +701,11 @@
},
"sqlState" : "42000"
},
+ "UNSCALED_VALUE_TOO_LARGE_FOR_PRECISION" : {
+ "message" : [
+ "Unscaled value too large for precision. If necessary set <ANSIEnabled> to false to bypass this error."
Review Comment:
ANSIEnabled -> ansiConfig, see other error classes.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala:
##########
@@ -2307,8 +2310,14 @@ private[sql] object QueryExecutionErrors extends QueryErrorsBase {
new SparkException("Foreach writer has been aborted due to a task failure")
}
- def integerOverflowError(message: String): Throwable = {
- new ArithmeticException(s"Integer overflow. $message")
+ def integerOverflowError(message: String): SparkArithmeticException = {
Review Comment:
The method `integerOverflowError` is invoked from only 2 places. Let's introduce specific error classes for both cases, and don't pass arbitrary message.
Could you image that we will output errors according to locale in a local language. In that case, we will translate entire `error-classes.json` to the language but some text will pass in English from source code. That looks inconsistent.
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -396,6 +401,11 @@
}
}
},
+ "INTEGER_OVERFLOW" : {
Review Comment:
What's the difference with `ARITHMETIC_OVERFLOW`? Can't you re-use the last one?
--
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] khalidmammadov commented on pull request #38273: [SPARK-37945][SQL][CORE] Use error classes in the execution errors of arithmetic ops
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on PR #38273:
URL: https://github.com/apache/spark/pull/38273#issuecomment-1288150124
@MaxGekk thanks for reviews and merge!
--
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] khalidmammadov commented on a diff in pull request #38273: [SPARK-37945][SQL][CORE] Use error classes in the execution errors of arithmetic ops
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on code in PR #38273:
URL: https://github.com/apache/spark/pull/38273#discussion_r996482360
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -256,6 +256,11 @@
],
"sqlState" : "22008"
},
+ "DECIMAL_PRECISION_EXCEEDS_MAX_PRECISION" : {
Review Comment:
sure, working on it
--
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] MaxGekk commented on a diff in pull request #38273: [SPARK-37945][SQL][CORE] Use error classes in the execution errors of arithmetic ops
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #38273:
URL: https://github.com/apache/spark/pull/38273#discussion_r996473498
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -256,6 +256,11 @@
],
"sqlState" : "22008"
},
+ "DECIMAL_PRECISION_EXCEEDS_MAX_PRECISION" : {
Review Comment:
Does the error class have at least one test? If not, please add one. The same question about other new error 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] khalidmammadov commented on a diff in pull request #38273: [SPARK-37945][SQL][CORE] Use error classes in the execution errors of arithmetic ops
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on code in PR #38273:
URL: https://github.com/apache/spark/pull/38273#discussion_r996913120
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -256,6 +256,11 @@
],
"sqlState" : "22008"
},
+ "DECIMAL_PRECISION_EXCEEDS_MAX_PRECISION" : {
Review Comment:
@MaxGekk Can you please check if all good or not?
--
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] khalidmammadov commented on pull request #38273: [SPARK-37945][SQL][CORE] Use error classes in the execution errors of arithmetic ops
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on PR #38273:
URL: https://github.com/apache/spark/pull/38273#issuecomment-1279949399
@MaxGekk Please 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] khalidmammadov commented on a diff in pull request #38273: [SPARK-37945][SQL][CORE] Use error classes in the execution errors of arithmetic ops
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on code in PR #38273:
URL: https://github.com/apache/spark/pull/38273#discussion_r996479467
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -256,6 +256,11 @@
],
"sqlState" : "22008"
},
+ "DECIMAL_PRECISION_EXCEEDS_MAX_PRECISION" : {
Review Comment:
@amaliujia apart from INTEGER_OVERFLOW rest of the changes are merely replacing name for the existing class metaname
@MaxGekk all of these are already unit tested within relevant use cases via **intercept[]** but I can work on changing those cases to assert additionally the class name/msg using **checkError( ... errorClass="CLASS_NAME")**
--
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] MaxGekk commented on a diff in pull request #38273: [SPARK-37945][SQL][CORE] Use error classes in the execution errors of arithmetic ops
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #38273:
URL: https://github.com/apache/spark/pull/38273#discussion_r1001374815
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -276,6 +276,11 @@
],
"sqlState" : "22008"
},
+ "DECIMAL_PRECISION_EXCEEDS_MAX_PRECISION" : {
+ "message" : [
+ "Decimal precision <precision> exceeds max precision <maxPrecision>"
Review Comment:
minor but for consistency:
```suggestion
"Decimal precision <precision> exceeds max precision <maxPrecision>."
```
##########
core/src/main/scala/org/apache/spark/SparkException.scala:
##########
@@ -348,3 +348,19 @@ private[spark] class SparkSQLFeatureNotSupportedException(
override def getErrorClass: String = errorClass
}
+
+/**
+ * Exception thrown from Spark Streaming framework.
+ */
+private[spark] class SparkStreamingException(
Review Comment:
Could you re-use the existing exception: `SparkRuntimeException`, please. We don't need to introduce additional things as we already have error classes that users can use to distinguish errors.
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -605,6 +620,11 @@
],
"sqlState" : "22005"
},
+ "OUT_OF_DECIMAL_TYPE_RANGE" : {
+ "message" : [
+ "Out of decimal type range: <value>"
Review Comment:
```suggestion
"Out of decimal type range: <value>."
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala:
##########
@@ -2384,8 +2387,28 @@ private[sql] object QueryExecutionErrors extends QueryErrorsBase {
new SparkException("Foreach writer has been aborted due to a task failure")
}
- def integerOverflowError(message: String): Throwable = {
- new ArithmeticException(s"Integer overflow. $message")
+ def incorrectRumpUpRate(rowsPerSecond: Long,
+ maxSeconds: Long,
+ rampUpTimeSeconds: Long): Throwable = {
Review Comment:
Could you fix indentation here. See https://github.com/databricks/scala-style-guide#spacing-and-indentation
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala:
##########
@@ -1260,27 +1260,30 @@ private[sql] object QueryExecutionErrors extends QueryErrorsBase {
def unscaledValueTooLargeForPrecisionError(): SparkArithmeticException = {
new SparkArithmeticException(
- errorClass = "_LEGACY_ERROR_TEMP_2117",
- messageParameters = Map("ansiConfig" -> toSQLConf(SQLConf.ANSI_ENABLED.key)),
+ errorClass = "UNSCALED_VALUE_TOO_LARGE_FOR_PRECISION",
+ messageParameters = Map(
+ "ansiConfig" -> SQLConf.ANSI_ENABLED.key),
Review Comment:
Wrap the config by `toSQLConf()`, see example in the file.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala:
##########
@@ -2384,8 +2387,28 @@ private[sql] object QueryExecutionErrors extends QueryErrorsBase {
new SparkException("Foreach writer has been aborted due to a task failure")
}
- def integerOverflowError(message: String): Throwable = {
- new ArithmeticException(s"Integer overflow. $message")
+ def incorrectRumpUpRate(rowsPerSecond: Long,
+ maxSeconds: Long,
+ rampUpTimeSeconds: Long): Throwable = {
+ new SparkStreamingException(
+ errorClass = "INCORRECT_RUMP_UP_RATE",
+ messageParameters = Map(
+ "rowsPerSecond" -> rowsPerSecond.toString,
+ "maxSeconds" -> maxSeconds.toString,
+ "rampUpTimeSeconds" -> rampUpTimeSeconds.toString
+ ))
+ }
+
+ def incorrectEndOffset(rowsPerSecond: Long,
+ maxSeconds: Long,
+ endSeconds: Long): Throwable = {
Review Comment:
Please, fix indentation.
--
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