-
Notifications
You must be signed in to change notification settings - Fork 28.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-47965][CORE] Avoid orNull in TypedConfigBuilder and OptionalConfigEntry #46197
Conversation
9f6632d
to
1941b66
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to introduce this breaking change?
@@ -196,12 +196,6 @@ class ConfigEntrySuite extends SparkFunSuite { | |||
assert(conversionError.getMessage === s"${conversionTest.key} should be double, but was abc") | |||
} | |||
|
|||
test("default value handling is null-safe") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we have a test coverage, it seems that we remove a valid use case, @HyukjinKwon .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, since this is an internal Spark change at Apache Spark 4.0.0,
+1, LGTM.
yeah, it should not be a breaking change I believe. I checked the configuration related interface, and we don't allow setting |
f3a6d1e
to
2a253a8
Compare
Merged to master. |
…ptionalConfigEntry` ### What changes were proposed in this pull request? This PR partially reverts #46197 because of the behaviour change below: ```python >>> spark.conf.get("spark.sql.optimizer.excludedRules") '<undefined>' ``` ### Why are the changes needed? To avoid behaviour change. ### Does this PR introduce _any_ user-facing change? No, the main change has not been released out yet. ### How was this patch tested? Manually as described above. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #46472 from HyukjinKwon/SPARK-47965-followup. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
…nfigEntry ### What changes were proposed in this pull request? This PR proposes to avoid orNull in `TypedConfigBuilder`. Keys and values cannot be set `null` anyway, see `RuntimeConfig` and `SparkConf`. Also, uses `ConfigEntry.UNDEFINED` in `OptionalConfigEntry` instead of `null`. ### Why are the changes needed? For code cleanup. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? CI in this PR should verify them. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#46197 from HyukjinKwon/SPARK-47965. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
…ptionalConfigEntry` ### What changes were proposed in this pull request? This PR partially reverts apache#46197 because of the behaviour change below: ```python >>> spark.conf.get("spark.sql.optimizer.excludedRules") '<undefined>' ``` ### Why are the changes needed? To avoid behaviour change. ### Does this PR introduce _any_ user-facing change? No, the main change has not been released out yet. ### How was this patch tested? Manually as described above. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#46472 from HyukjinKwon/SPARK-47965-followup. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
What changes were proposed in this pull request?
This PR proposes to avoid orNull in
TypedConfigBuilder
. Keys and values cannot be setnull
anyway, seeRuntimeConfig
andSparkConf
.Also, uses
ConfigEntry.UNDEFINED
inOptionalConfigEntry
instead ofnull
.Why are the changes needed?
For code cleanup.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
CI in this PR should verify them.
Was this patch authored or co-authored using generative AI tooling?
No.