You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "hvanhovell (via GitHub)" <gi...@apache.org> on 2024/12/04 14:55:33 UTC
[PR] [SPARK-49709][CONNECT][SQL] Support ConfigEntry in the RuntimeConfig interface [spark]
hvanhovell opened a new pull request, #49062:
URL: https://github.com/apache/spark/pull/49062
### What changes were proposed in this pull request?
This PR adds support for ConfigEntry to the RuntimeConfig interface. This was removed in https://github.com/apache/spark/pull/47980.
### Why are the changes needed?
This functionality is used a lot by Spark libraries. Removing them caused friction, and adding them does not pollute the RuntimeConfig interface.
### Does this PR introduce _any_ user-facing change?
No. This is developer API.
### How was this patch tested?
Exiting 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-49709][CONNECT][SQL] Support ConfigEntry in the RuntimeConfig interface [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #49062:
URL: https://github.com/apache/spark/pull/49062#discussion_r1870461716
##########
sql/core/src/test/scala/org/apache/spark/sql/RuntimeConfigSuite.scala:
##########
@@ -108,4 +108,26 @@ class RuntimeConfigSuite extends SparkFunSuite {
// this set should not fail
conf.set(DEFAULT_PARALLELISM.key, "1")
}
+
+ test("config entry") {
+ val conf = newConf()
+
+ val entry = SQLConf.FILES_MAX_PARTITION_NUM
+ assert(conf.get(entry.key) === null)
Review Comment:
yeah, i think it should. `def get[T](entry: ConfigEntry[T]): T` isn't an API so I think we should be fine with changing this behaviour too.
--
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-49709][CONNECT][SQL] Support ConfigEntry in the RuntimeConfig interface [spark]
Posted by "asfgit (via GitHub)" <gi...@apache.org>.
asfgit closed pull request #49062: [SPARK-49709][CONNECT][SQL] Support ConfigEntry in the RuntimeConfig interface
URL: https://github.com/apache/spark/pull/49062
--
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-49709][CONNECT][SQL] Support ConfigEntry in the RuntimeConfig interface [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on PR #49062:
URL: https://github.com/apache/spark/pull/49062#issuecomment-2518464596
Merging to master.
--
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-49709][CONNECT][SQL] Support ConfigEntry in the RuntimeConfig interface [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #49062:
URL: https://github.com/apache/spark/pull/49062#discussion_r1869958584
##########
sql/core/src/test/scala/org/apache/spark/sql/RuntimeConfigSuite.scala:
##########
@@ -108,4 +108,26 @@ class RuntimeConfigSuite extends SparkFunSuite {
// this set should not fail
conf.set(DEFAULT_PARALLELISM.key, "1")
}
+
+ test("config entry") {
+ val conf = newConf()
+
+ val entry = SQLConf.FILES_MAX_PARTITION_NUM
+ assert(conf.get(entry.key) === null)
Review Comment:
@HyukjinKwon @cloud-fan this returns null. I was expecting a `NoSuchElementException` here. IMO this is a bug. WDYT?
##########
sql/core/src/test/scala/org/apache/spark/sql/RuntimeConfigSuite.scala:
##########
@@ -108,4 +108,26 @@ class RuntimeConfigSuite extends SparkFunSuite {
// this set should not fail
conf.set(DEFAULT_PARALLELISM.key, "1")
}
+
+ test("config entry") {
+ val conf = newConf()
+
+ val entry = SQLConf.FILES_MAX_PARTITION_NUM
+ assert(conf.get(entry.key) === null)
Review Comment:
@HyukjinKwon @cloud-fan this returns `null`. I was expecting a `NoSuchElementException` here. IMO this is a bug. WDYT?
--
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