You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "MaxGekk (via GitHub)" <gi...@apache.org> on 2024/02/18 16:39:21 UTC
[PR] [WIP][SQL] Raise Spark's exception with an error class in config value check [spark]
MaxGekk opened a new pull request, #45156:
URL: https://github.com/apache/spark/pull/45156
### What changes were proposed in this pull request?
In the PR, I propose to extend the `TypedConfigBuilder` API by new method:
```scala
def checkValue(
validator: T => Boolean,
errorClass: String,
parameters: Map[String, String]): TypedConfigBuilder[T] = {
```
which raises `SparkIllegalArgumentException` with an error class when `checkValue` fails.
As an example, I ported the check of the SQL config `spark.sql.session.timeZone` on the new method.
### Why are the changes needed?
To improve user's experience with Spark SQL by migration on unified error framework.
### Does this PR introduce _any_ user-facing change?
It can if user's code depends on particular format of error messages.
### How was this patch tested?
By running the affected test suites:
```
$ build/sbt "test:testOnly *SQLConfSuite"
$ build/sbt "core/testOnly *SparkThrowableSuite"
```
### Was this patch authored or co-authored using generative AI tooling?
No.
--
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
Re: [PR] [SPARK-47087][SQL] Raise Spark's exception with an error class in config value check [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #45156:
URL: https://github.com/apache/spark/pull/45156#discussion_r1494082768
##########
core/src/main/scala/org/apache/spark/internal/config/ConfigBuilder.scala:
##########
@@ -111,6 +112,24 @@ private[spark] class TypedConfigBuilder[T](
}
}
+ /** Checks if the user-provided value for the config matches the validator.
+ * If it doesn't match, raise Spark's exception with the given error class. */
+ def checkValue(
+ validator: T => Boolean,
+ errorClass: String,
+ parameters: Map[String, String]): TypedConfigBuilder[T] = {
Review Comment:
Sorry, I didn't get how `parameters` are related to the signature of `validator`. Just in case, here is one of examples where we would need the `parameters`:
https://github.com/apache/spark/blob/174a19c1c0391b1084e6a0b8fdbc5c019a662b01/core/src/main/scala/org/apache/spark/internal/config/package.scala#L1128-L1131
And if you remove `parameters` how would you overload the existing one?
```scala
def checkValue(validator: T => Boolean, errorMsg: String): TypedConfigBuilder[T] = {
```
We could "inline" the parameters to error message format, but need to migrate all configs in one shot to:
```scala
def checkValue(validator: T => Boolean, errorClass: String): TypedConfigBuilder[T] = {
```
--
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
Re: [PR] [SPARK-47087][SQL] Raise Spark's exception with an error class in config value check [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #45156:
URL: https://github.com/apache/spark/pull/45156#discussion_r1493993228
##########
core/src/main/scala/org/apache/spark/internal/config/ConfigBuilder.scala:
##########
@@ -111,6 +112,24 @@ private[spark] class TypedConfigBuilder[T](
}
}
+ /** Checks if the user-provided value for the config matches the validator.
+ * If it doesn't match, raise Spark's exception with the given error class. */
+ def checkValue(
+ validator: T => Boolean,
+ errorClass: String,
+ parameters: Map[String, String]): TypedConfigBuilder[T] = {
Review Comment:
do we need this parameter? Looking at the signature of `validator`, `T => Boolean`, I don't think a config definition can provide extra parameters.
--
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
Re: [PR] [SPARK-47087][SQL] Raise Spark's exception with an error class in config value check [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #45156:
URL: https://github.com/apache/spark/pull/45156#discussion_r1494602700
##########
core/src/main/scala/org/apache/spark/internal/config/ConfigBuilder.scala:
##########
@@ -111,6 +112,24 @@ private[spark] class TypedConfigBuilder[T](
}
}
+ /** Checks if the user-provided value for the config matches the validator.
+ * If it doesn't match, raise Spark's exception with the given error class. */
+ def checkValue(
+ validator: T => Boolean,
+ errorClass: String,
+ parameters: Map[String, String]): TypedConfigBuilder[T] = {
Review Comment:
Oh I get it now. We can't reference Scala objects in the JSON file, and have to use parameters.
--
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
Re: [PR] [SPARK-47087][SQL] Raise Spark's exception with an error class in config value check [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #45156: [SPARK-47087][SQL] Raise Spark's exception with an error class in config value check
URL: https://github.com/apache/spark/pull/45156
--
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
Re: [PR] [SPARK-47087][SQL] Raise Spark's exception with an error class in config value check [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #45156:
URL: https://github.com/apache/spark/pull/45156#issuecomment-1952568093
Merging to master. Thank you, @cloud-fan 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