You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "yaooqinn (via GitHub)" <gi...@apache.org> on 2025/04/24 07:39:59 UTC
[PR] [SPARK-51896][CORE][SQL] Add Java Enum Support for TypedConfigBuilder [spark]
yaooqinn opened a new pull request, #50691:
URL: https://github.com/apache/spark/pull/50691
### What changes were proposed in this pull request?
Like what we improves in https://github.com/apache/spark/pull/50674.
This PR introduces TypedConfigBuilder for Java enums and leverages it for existing configurations that use enums as parameters.
Before this PR, we need to change them from Enumeration to string, string to Enumeration, back and forth... We also need to do upper-case transformation, .checkValues validation one by one.
After this PR, those steps are centralized.
### Why are the changes needed?
Better support for java-enum-like configurations
### Does this PR introduce _any_ user-facing change?
no
### How was this patch tested?
new tests
### 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-51896][CORE][SQL] Add Java Enum Support for TypedConfigBuilder [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #50691:
URL: https://github.com/apache/spark/pull/50691#discussion_r2059449288
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -415,21 +415,14 @@ object SQLConf {
private val VALID_LOG_LEVELS: Array[String] = Level.values.map(_.toString)
- private def isValidLogLevel(level: String): Boolean =
- VALID_LOG_LEVELS.contains(level.toUpperCase(Locale.ROOT))
-
val PLAN_CHANGE_LOG_LEVEL = buildConf("spark.sql.planChangeLog.level")
.internal()
.doc("Configures the log level for logging the change from the original plan to the new " +
s"plan after a rule or batch is applied. The value can be " +
s"${VALID_LOG_LEVELS.mkString(", ")}.")
.version("3.1.0")
- .stringConf
- .transform(_.toUpperCase(Locale.ROOT))
- .checkValue(isValidLogLevel,
- "Invalid value for 'spark.sql.planChangeLog.level'. Valid values are " +
- s"${VALID_LOG_LEVELS.mkString(", ")}.")
- .createWithDefault("trace")
+ .enumConf(classOf[Level])
Review Comment:
It's org.slf4j.event.Level
--
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-51896][CORE][SQL] Add Java Enum Support for TypedConfigBuilder [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #50691:
URL: https://github.com/apache/spark/pull/50691#discussion_r2058060800
##########
core/src/test/scala/org/apache/spark/JobCancellationSuite.scala:
##########
@@ -59,7 +59,7 @@ class JobCancellationSuite extends SparkFunSuite with Matchers with BeforeAndAft
}
test("local mode, FIFO scheduler") {
- val conf = new SparkConf().set(SCHEDULER_MODE, "FIFO")
+ val conf = new SparkConf().set(SCHEDULER_MODE.key, "FIFO")
Review Comment:
Ur, is this a breaking change, @yaooqinn ?
--
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-51896][CORE][SQL] Add Java Enum Support for TypedConfigBuilder [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #50691:
URL: https://github.com/apache/spark/pull/50691#discussion_r2059448726
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -461,12 +454,8 @@ object SQLConf {
"the resolved expression tree in the single-pass bottom-up Resolver. The value can be " +
s"${VALID_LOG_LEVELS.mkString(", ")}.")
.version("4.0.0")
- .stringConf
- .transform(_.toUpperCase(Locale.ROOT))
- .checkValue(isValidLogLevel,
- "Invalid value for 'spark.sql.expressionTreeChangeLog.level'. Valid values are " +
- s"${VALID_LOG_LEVELS.mkString(", ")}.")
- .createWithDefault("trace")
+ .enumConf(classOf[Level])
+ .createWithDefault(Level.TRACE)
Review Comment:
set/get as string is still available.
--
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-51896][CORE][SQL] Add Java Enum Support for TypedConfigBuilder [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50691:
URL: https://github.com/apache/spark/pull/50691#discussion_r2058957022
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -415,21 +415,14 @@ object SQLConf {
private val VALID_LOG_LEVELS: Array[String] = Level.values.map(_.toString)
- private def isValidLogLevel(level: String): Boolean =
- VALID_LOG_LEVELS.contains(level.toUpperCase(Locale.ROOT))
-
val PLAN_CHANGE_LOG_LEVEL = buildConf("spark.sql.planChangeLog.level")
.internal()
.doc("Configures the log level for logging the change from the original plan to the new " +
s"plan after a rule or batch is applied. The value can be " +
s"${VALID_LOG_LEVELS.mkString(", ")}.")
.version("3.1.0")
- .stringConf
- .transform(_.toUpperCase(Locale.ROOT))
- .checkValue(isValidLogLevel,
- "Invalid value for 'spark.sql.planChangeLog.level'. Valid values are " +
- s"${VALID_LOG_LEVELS.mkString(", ")}.")
- .createWithDefault("trace")
+ .enumConf(classOf[Level])
Review Comment:
`Level` is from log4j?
--
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-51896][CORE][SQL] Add Java Enum Support for TypedConfigBuilder [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #50691:
URL: https://github.com/apache/spark/pull/50691#discussion_r2058066254
##########
core/src/test/scala/org/apache/spark/JobCancellationSuite.scala:
##########
@@ -59,7 +59,7 @@ class JobCancellationSuite extends SparkFunSuite with Matchers with BeforeAndAft
}
test("local mode, FIFO scheduler") {
- val conf = new SparkConf().set(SCHEDULER_MODE, "FIFO")
+ val conf = new SparkConf().set(SCHEDULER_MODE.key, "FIFO")
Review Comment:
It seems that I missed this in Scala version PR. I'm curious if we can avoid this kind of programing behavior change.
--
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-51896][CORE][SQL] Add Java Enum Support for TypedConfigBuilder [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #50691:
URL: https://github.com/apache/spark/pull/50691#issuecomment-2829283158
Merged to master, thank you @cloud-fan @dongjoon-hyun @HyukjinKwon @LuciferYang for the review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51896][CORE][SQL] Add Java Enum Support for TypedConfigBuilder [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #50691:
URL: https://github.com/apache/spark/pull/50691#issuecomment-2829283424
+1, late LGTM.
--
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-51896][CORE][SQL] Add Java Enum Support for TypedConfigBuilder [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #50691:
URL: https://github.com/apache/spark/pull/50691#discussion_r2059355696
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -461,12 +454,8 @@ object SQLConf {
"the resolved expression tree in the single-pass bottom-up Resolver. The value can be " +
s"${VALID_LOG_LEVELS.mkString(", ")}.")
.version("4.0.0")
- .stringConf
- .transform(_.toUpperCase(Locale.ROOT))
- .checkValue(isValidLogLevel,
- "Invalid value for 'spark.sql.expressionTreeChangeLog.level'. Valid values are " +
- s"${VALID_LOG_LEVELS.mkString(", ")}.")
- .createWithDefault("trace")
+ .enumConf(classOf[Level])
+ .createWithDefault(Level.TRACE)
Review Comment:
What do we get if we get this conf from PySpark (or SparkR)?
--
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-51896][CORE][SQL] Add Java Enum Support for TypedConfigBuilder [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50691:
URL: https://github.com/apache/spark/pull/50691#discussion_r2059489024
##########
common/utils/src/main/scala/org/apache/spark/internal/config/ConfigBuilder.scala:
##########
@@ -57,6 +57,15 @@ private object ConfigHelpers {
}
}
+ def toEnum[E <: Enum[E]](s: String, enumClass: Class[E], key: String): E = {
+ enumClass.getEnumConstants.find(_.name().equalsIgnoreCase(s.trim)) match {
+ case Some(enum) => enum
+ case None =>
+ throw new IllegalArgumentException(
+ s"$key should be one of ${enumClass.getEnumConstants.mkString(", ")}, but was $s")
Review Comment:
should we have an error condition for it?
##########
common/utils/src/main/scala/org/apache/spark/internal/config/ConfigBuilder.scala:
##########
@@ -57,6 +57,15 @@ private object ConfigHelpers {
}
}
+ def toEnum[E <: Enum[E]](s: String, enumClass: Class[E], key: String): E = {
+ enumClass.getEnumConstants.find(_.name().equalsIgnoreCase(s.trim)) match {
+ case Some(enum) => enum
+ case None =>
+ throw new IllegalArgumentException(
+ s"$key should be one of ${enumClass.getEnumConstants.mkString(", ")}, but was $s")
Review Comment:
ditto for the scala enum
--
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-51896][CORE][SQL] Add Java Enum Support for TypedConfigBuilder [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn closed pull request #50691: [SPARK-51896][CORE][SQL] Add Java Enum Support for TypedConfigBuilder
URL: https://github.com/apache/spark/pull/50691
--
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-51896][CORE][SQL] Add Java Enum Support for TypedConfigBuilder [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #50691:
URL: https://github.com/apache/spark/pull/50691#issuecomment-2829286063
late LGTM
--
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-51896][CORE][SQL] Add Java Enum Support for TypedConfigBuilder [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #50691:
URL: https://github.com/apache/spark/pull/50691#discussion_r2059504615
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -415,21 +415,14 @@ object SQLConf {
private val VALID_LOG_LEVELS: Array[String] = Level.values.map(_.toString)
- private def isValidLogLevel(level: String): Boolean =
- VALID_LOG_LEVELS.contains(level.toUpperCase(Locale.ROOT))
-
val PLAN_CHANGE_LOG_LEVEL = buildConf("spark.sql.planChangeLog.level")
.internal()
.doc("Configures the log level for logging the change from the original plan to the new " +
s"plan after a rule or batch is applied. The value can be " +
s"${VALID_LOG_LEVELS.mkString(", ")}.")
.version("3.1.0")
- .stringConf
- .transform(_.toUpperCase(Locale.ROOT))
- .checkValue(isValidLogLevel,
- "Invalid value for 'spark.sql.planChangeLog.level'. Valid values are " +
- s"${VALID_LOG_LEVELS.mkString(", ")}.")
- .createWithDefault("trace")
+ .enumConf(classOf[Level])
Review Comment:
Got 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
Re: [PR] [SPARK-51896][CORE][SQL] Add Java Enum Support for TypedConfigBuilder [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #50691:
URL: https://github.com/apache/spark/pull/50691#issuecomment-2827111749
cc @dongjoon-hyun @cloud-fan @LuciferYang, thank you in advance
--
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-51896][CORE][SQL] Add Java Enum Support for TypedConfigBuilder [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #50691:
URL: https://github.com/apache/spark/pull/50691#discussion_r2058363547
##########
core/src/test/scala/org/apache/spark/JobCancellationSuite.scala:
##########
@@ -59,7 +59,7 @@ class JobCancellationSuite extends SparkFunSuite with Matchers with BeforeAndAft
}
test("local mode, FIFO scheduler") {
- val conf = new SparkConf().set(SCHEDULER_MODE, "FIFO")
+ val conf = new SparkConf().set(SCHEDULER_MODE.key, "FIFO")
Review Comment:
Hi @dongjoon-hyun，set with ConfigEntry is in private scope
--
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-51896][CORE][SQL] Add Java Enum Support for TypedConfigBuilder [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #50691:
URL: https://github.com/apache/spark/pull/50691#discussion_r2059501700
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -415,21 +415,14 @@ object SQLConf {
private val VALID_LOG_LEVELS: Array[String] = Level.values.map(_.toString)
- private def isValidLogLevel(level: String): Boolean =
- VALID_LOG_LEVELS.contains(level.toUpperCase(Locale.ROOT))
-
val PLAN_CHANGE_LOG_LEVEL = buildConf("spark.sql.planChangeLog.level")
.internal()
.doc("Configures the log level for logging the change from the original plan to the new " +
s"plan after a rule or batch is applied. The value can be " +
s"${VALID_LOG_LEVELS.mkString(", ")}.")
.version("3.1.0")
- .stringConf
- .transform(_.toUpperCase(Locale.ROOT))
- .checkValue(isValidLogLevel,
- "Invalid value for 'spark.sql.planChangeLog.level'. Valid values are " +
- s"${VALID_LOG_LEVELS.mkString(", ")}.")
- .createWithDefault("trace")
+ .enumConf(classOf[Level])
Review Comment:
Hi, there is no such risk since these changes are all targeting private APIs.
Assuming removing slf4j does break stuff here, it will break w/ or w/o this PR, see
https://github.com/apache/spark/pull/50691/files#diff-13c5b65678b327277c68d17910ae93629801af00117a0e3da007afd95b6c6764R5933
--
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-51896][CORE][SQL] Add Java Enum Support for TypedConfigBuilder [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #50691:
URL: https://github.com/apache/spark/pull/50691#discussion_r2059448726
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -461,12 +454,8 @@ object SQLConf {
"the resolved expression tree in the single-pass bottom-up Resolver. The value can be " +
s"${VALID_LOG_LEVELS.mkString(", ")}.")
.version("4.0.0")
- .stringConf
- .transform(_.toUpperCase(Locale.ROOT))
- .checkValue(isValidLogLevel,
- "Invalid value for 'spark.sql.expressionTreeChangeLog.level'. Valid values are " +
- s"${VALID_LOG_LEVELS.mkString(", ")}.")
- .createWithDefault("trace")
+ .enumConf(classOf[Level])
+ .createWithDefault(Level.TRACE)
Review Comment:
set/get as string is still available due to the underlying T=>str/str=>T conversion
--
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-51896][CORE][SQL] Add Java Enum Support for TypedConfigBuilder [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #50691:
URL: https://github.com/apache/spark/pull/50691#discussion_r2059496006
##########
common/utils/src/main/scala/org/apache/spark/internal/config/ConfigBuilder.scala:
##########
@@ -57,6 +57,15 @@ private object ConfigHelpers {
}
}
+ def toEnum[E <: Enum[E]](s: String, enumClass: Class[E], key: String): E = {
+ enumClass.getEnumConstants.find(_.name().equalsIgnoreCase(s.trim)) match {
+ case Some(enum) => enum
+ case None =>
+ throw new IllegalArgumentException(
+ s"$key should be one of ${enumClass.getEnumConstants.mkString(", ")}, but was $s")
Review Comment:
We can have a separate PR for this to make [all](https://github.com/apache/spark/pull/50691/files#diff-60aa58f28fb7587d96dcb4031dbb86750094d9ad4a1488eb0d115b64fd3e84f0R32-R66) the condition-less errors uniform
--
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-51896][CORE][SQL] Add Java Enum Support for TypedConfigBuilder [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #50691:
URL: https://github.com/apache/spark/pull/50691#discussion_r2059498684
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -415,21 +415,14 @@ object SQLConf {
private val VALID_LOG_LEVELS: Array[String] = Level.values.map(_.toString)
- private def isValidLogLevel(level: String): Boolean =
- VALID_LOG_LEVELS.contains(level.toUpperCase(Locale.ROOT))
-
val PLAN_CHANGE_LOG_LEVEL = buildConf("spark.sql.planChangeLog.level")
.internal()
.doc("Configures the log level for logging the change from the original plan to the new " +
s"plan after a rule or batch is applied. The value can be " +
s"${VALID_LOG_LEVELS.mkString(", ")}.")
.version("3.1.0")
- .stringConf
- .transform(_.toUpperCase(Locale.ROOT))
- .checkValue(isValidLogLevel,
- "Invalid value for 'spark.sql.planChangeLog.level'. Valid values are " +
- s"${VALID_LOG_LEVELS.mkString(", ")}.")
- .createWithDefault("trace")
+ .enumConf(classOf[Level])
Review Comment:
If slf4j is no longer used in the future, will there be a risk of introducing breaking changes?
--
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