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/01/21 11:50:08 UTC
[GitHub] [spark] MaxGekk opened a new pull request #35272: [SPARK-37979][SQL] Switch to more generic error classes in AES functions
MaxGekk opened a new pull request #35272:
URL: https://github.com/apache/spark/pull/35272
### What changes were proposed in this pull request?
In the PR, I propose to switch from specific error classes in the AES functions: `aes_encrypt()/aes_decrypt()` to more generic:
- AES_CRYPTO_ERROR -> INVALID_PARAMETER_VALUE
- INVALID_AES_KEY_LENGTH -> INVALID_PARAMETER_VALUE
- UNSUPPORTED_AES_MODE -> UNSUPPORTED_MODE
The new error classes `INVALID_PARAMETER_VALUE` and `UNSUPPORTED_MODE` are made to be re-used from other functions but not only in the AES functions.
### Why are the changes needed?
1. To prevent unlimited inflation of the set of error classes and as a consequence of that inflation of `error-classes.json`.
2. To establish rules for other sub-tasks of SPARK-37935
### Does this PR introduce _any_ user-facing change?
Yes, but the AES functions `aes_encrypt()`/`aes_decrypt()` haven't released yet.
### How was this patch tested?
By running the affected test suites:
```
$ build/sbt "test:testOnly *SparkThrowableSuite"
$ build/sbt "test:testOnly *QueryExecutionErrorsSuite"
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #35272: [SPARK-37979][SQL] Switch to more generic error classes in AES functions
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35272:
URL: https://github.com/apache/spark/pull/35272#discussion_r789615081
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -160,6 +153,10 @@
"message" : [ "Unsupported literal type %s %s" ],
"sqlState" : "0A000"
},
+ "UNSUPPORTED_MODE" : {
Review comment:
`mode` is also a parameter of the aes function, shall we reuse `INVALID_PARAMETER_VALUE`
--
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 change in pull request #35272: [SPARK-37979][SQL] Switch to more generic error classes in AES functions
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #35272:
URL: https://github.com/apache/spark/pull/35272#discussion_r789747323
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -160,6 +153,10 @@
"message" : [ "Unsupported literal type %s %s" ],
"sqlState" : "0A000"
},
+ "UNSUPPORTED_MODE" : {
Review comment:
@cloud-fan What is the use case for the sqlState = 0A000 (feature not supported, see https://github.com/apache/spark/tree/master/core/src/main/resources/error) from your point of view?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #35272: [SPARK-37979][SQL] Switch to more generic error classes in AES functions
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35272:
URL: https://github.com/apache/spark/pull/35272#discussion_r790622028
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala
##########
@@ -1905,15 +1905,27 @@ object QueryExecutionErrors {
}
def invalidAesKeyLengthError(actualLength: Int): RuntimeException = {
- new SparkRuntimeException("INVALID_AES_KEY_LENGTH", Array(actualLength.toString))
+ new SparkRuntimeException(
+ errorClass = "INVALID_PARAMETER_VALUE",
+ messageParameters = Array(
+ "key",
+ "aes_encrypt/aes_decrypt",
+ s"expects a binary value with 16, 24 or 32 bytes, but got ${actualLength.toString} bytes."))
}
def aesModeUnsupportedError(mode: String, padding: String): RuntimeException = {
- new SparkRuntimeException("UNSUPPORTED_AES_MODE", Array(mode, padding))
+ new SparkRuntimeException(
+ errorClass = "UNSUPPORTED_FEATURE",
+ messageParameters = Array(s"AES-$mode with the padding $padding"))
}
def aesCryptoError(detailMessage: String): RuntimeException = {
- new SparkRuntimeException("AES_CRYPTO_ERROR", Array(detailMessage))
+ new SparkRuntimeException(
+ errorClass = "INVALID_PARAMETER_VALUE",
+ messageParameters = Array(
+ "expr, key",
+ "aes_encrypt/aes_decrypt",
Review comment:
```suggestion
"function aes_encrypt/aes_decrypt",
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #35272: [SPARK-37979][SQL] Switch to more generic error classes in AES functions
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35272:
URL: https://github.com/apache/spark/pull/35272#discussion_r790621400
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala
##########
@@ -1905,15 +1905,27 @@ object QueryExecutionErrors {
}
def invalidAesKeyLengthError(actualLength: Int): RuntimeException = {
- new SparkRuntimeException("INVALID_AES_KEY_LENGTH", Array(actualLength.toString))
+ new SparkRuntimeException(
+ errorClass = "INVALID_PARAMETER_VALUE",
+ messageParameters = Array(
+ "key",
+ "aes_encrypt/aes_decrypt",
Review comment:
```suggestion
"function aes_encrypt/aes_decrypt",
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #35272: [SPARK-37979][SQL] Switch to more generic error classes in AES functions
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35272:
URL: https://github.com/apache/spark/pull/35272#discussion_r790621888
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala
##########
@@ -1905,15 +1905,27 @@ object QueryExecutionErrors {
}
def invalidAesKeyLengthError(actualLength: Int): RuntimeException = {
- new SparkRuntimeException("INVALID_AES_KEY_LENGTH", Array(actualLength.toString))
+ new SparkRuntimeException(
+ errorClass = "INVALID_PARAMETER_VALUE",
+ messageParameters = Array(
+ "key",
+ "aes_encrypt/aes_decrypt",
+ s"expects a binary value with 16, 24 or 32 bytes, but got ${actualLength.toString} bytes."))
}
def aesModeUnsupportedError(mode: String, padding: String): RuntimeException = {
- new SparkRuntimeException("UNSUPPORTED_AES_MODE", Array(mode, padding))
+ new SparkRuntimeException(
+ errorClass = "UNSUPPORTED_FEATURE",
+ messageParameters = Array(s"AES-$mode with the padding $padding"))
Review comment:
```suggestion
messageParameters = Array(s"AES-$mode with the padding $padding in function aes_encrypt/aes_decrypt"))
```
--
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 change in pull request #35272: [SPARK-37979][SQL] Switch to more generic error classes in AES functions
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #35272:
URL: https://github.com/apache/spark/pull/35272#discussion_r789670170
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -160,6 +153,10 @@
"message" : [ "Unsupported literal type %s %s" ],
"sqlState" : "0A000"
},
+ "UNSUPPORTED_MODE" : {
Review comment:
Actually, any failure/error can be considered as a result of an invalid parameter. We could consider the entire universe as the input to Spark's code.
--
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 #35272: [SPARK-37979][SQL] Switch to more generic error classes in AES functions
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on pull request #35272:
URL: https://github.com/apache/spark/pull/35272#issuecomment-1020124164
Merging to master. Thank you, @cloud-fan .
--
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 #35272: [SPARK-37979][SQL] Switch to more generic error classes in AES functions
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on pull request #35272:
URL: https://github.com/apache/spark/pull/35272#issuecomment-1018440203
@cloud-fan @sarutak @srielau @entong Could you review this PR, 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 closed pull request #35272: [SPARK-37979][SQL] Switch to more generic error classes in AES functions
Posted by GitBox <gi...@apache.org>.
MaxGekk closed pull request #35272:
URL: https://github.com/apache/spark/pull/35272
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #35272: [SPARK-37979][SQL] Switch to more generic error classes in AES functions
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35272:
URL: https://github.com/apache/spark/pull/35272#discussion_r789618372
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -99,6 +92,10 @@
"INVALID_JSON_SCHEMA_MAPTYPE" : {
"message" : [ "Input schema %s can only contain StringType as a key type for a MapType." ]
},
+ "INVALID_PARAMETER_VALUE" : {
+ "message" : [ "Invalid parameter value of '%s' in '%s'. %s" ],
Review comment:
how about `The value of parameter '%s' in %s is invalid: %s`. Then we can have something like
`The value of parameter 'key' in function 'aes_encrypt/aes_decrypt' is invalid: expects a binary value with 16, 24 or 32 bytes, but got 9 bytes.`
--
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