[SPARK-51896][CORE][SQL] Add Java Enum Support for TypedConfigBuilder #50691
Conversation
|
cc @dongjoon-hyun @cloud-fan @LuciferYang, thank you in advance |
|
|
||
| test("local mode, FIFO scheduler") { | ||
| val conf = new SparkConf().set(SCHEDULER_MODE, "FIFO") | ||
| val conf = new SparkConf().set(SCHEDULER_MODE.key, "FIFO") |
There was a problem hiding this comment.
It seems that I missed this in Scala version PR. I'm curious if we can avoid this kind of programing behavior change.
There was a problem hiding this comment.
Hi @dongjoon-hyun，set with ConfigEntry is in private scope
| "Invalid value for 'spark.sql.planChangeLog.level'. Valid values are " + | ||
| s"${VALID_LOG_LEVELS.mkString(", ")}.") | ||
| .createWithDefault("trace") | ||
| .enumConf(classOf[Level]) |
There was a problem hiding this comment.
It's org.slf4j.event.Level
There was a problem hiding this comment.
If slf4j is no longer used in the future, will there be a risk of introducing breaking changes?
There was a problem hiding this 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
| s"${VALID_LOG_LEVELS.mkString(", ")}.") | ||
| .createWithDefault("trace") | ||
| .enumConf(classOf[Level]) | ||
| .createWithDefault(Level.TRACE) |
There was a problem hiding this comment.
What do we get if we get this conf from PySpark (or SparkR)?
There was a problem hiding this comment.
set/get as string is still available due to the underlying T=>str/str=>T conversion
| case Some(enum) => enum | ||
| case None => | ||
| throw new IllegalArgumentException( | ||
| s"$key should be one of ${enumClass.getEnumConstants.mkString(", ")}, but was $s") |
There was a problem hiding this comment.
should we have an error condition for it?
There was a problem hiding this comment.
ditto for the scala enum
There was a problem hiding this comment.
We can have a separate PR for this to make all the condition-less errors uniform
|
Merged to master, thank you @cloud-fan @dongjoon-hyun @HyukjinKwon @LuciferYang for the review |
|
+1, late LGTM. |
|
late LGTM |
### What changes were proposed in this pull request? Like what we‘ve improved in apache#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 Closes apache#50691 from yaooqinn/SPARK-51896. Authored-by: Kent Yao <[email protected]> Signed-off-by: Kent Yao <[email protected]>
5 participants
What changes were proposed in this pull request?
Like what we‘ve improved in #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