You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/03/23 11:31:37 UTC
[GitHub] [spark] jackylee-ch opened a new pull request #35950: [SPARK-38419][SQL] change spark.sessionstate.conf.getConf/setConf operation to spark.conf.get/set
jackylee-ch opened a new pull request #35950:
URL: https://github.com/apache/spark/pull/35950
### Why are the changes needed?
In the sql module, we provide `SparkSession.conf` as a unified entry for `SQLConf.set/get`, which can prevent users or logic from modifying StaticSQLConf and Spark configs. However, I found `SparkSession.sessionstate.conf` is used in some code to getConf or setConf, which can skip the check of `RuntimeConfig`.
In this PR, we want to unify the behavior of `SQLConf.getConf/setConf` to `SparkSession.conf`.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Origin GA
--
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
[GitHub] [spark] srowen commented on pull request #35950: [SPARK-38419][SQL] change spark.sessionstate.conf.getConf/setConf operation to spark.conf.get/set
Posted by GitBox <gi...@apache.org>.
srowen commented on pull request #35950:
URL: https://github.com/apache/spark/pull/35950#issuecomment-1076342913
I don't know this part well, but it seems reasonable to unify the checks here
--
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
[GitHub] [spark] jackylee-ch commented on a change in pull request #35950: [SPARK-38419][SQL] Change spark.sessionstate.conf.getConf/setConf operation to spark.conf.get/set
Posted by GitBox <gi...@apache.org>.
jackylee-ch commented on a change in pull request #35950:
URL: https://github.com/apache/spark/pull/35950#discussion_r833395141
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/RuntimeConfig.scala
##########
@@ -60,6 +60,16 @@ class RuntimeConfig private[sql](sqlConf: SQLConf = new SQLConf) {
set(key, value.toString)
}
+ /**
+ * Sets the given Spark runtime configuration property.
+ *
+ * @since 3.4.0
+ */
+ def set[T](entry: ConfigEntry[T], value: T): Unit = {
Review comment:
Sure, I will change 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
[GitHub] [spark] AmplabJenkins commented on pull request #35950: [SPARK-38419][SQL] Change spark.sessionstate.conf.getConf/setConf operation to spark.conf.get/set
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #35950:
URL: https://github.com/apache/spark/pull/35950#issuecomment-1078492535
Can one of the admins verify this patch?
--
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
[GitHub] [spark] cloud-fan commented on pull request #35950: [SPARK-38419][SQL] Change spark.sessionstate.conf.getConf/setConf operation to spark.conf.get/set
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #35950:
URL: https://github.com/apache/spark/pull/35950#issuecomment-1084416918
At least we shouldn't link to an unrelated JIRA... Please create a new one for this PR as it's not a minor one-line 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
[GitHub] [spark] cloud-fan commented on a change in pull request #35950: [SPARK-38419][SQL] change spark.sessionstate.conf.getConf/setConf operation to spark.conf.get/set
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35950:
URL: https://github.com/apache/spark/pull/35950#discussion_r833285706
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/RuntimeConfig.scala
##########
@@ -60,6 +60,16 @@ class RuntimeConfig private[sql](sqlConf: SQLConf = new SQLConf) {
set(key, value.toString)
}
+ /**
+ * Sets the given Spark runtime configuration property.
+ *
+ * @since 3.4.0
+ */
+ def set[T](entry: ConfigEntry[T], value: T): Unit = {
Review comment:
`ConfigEntry` is a private class, that's why `protected[sql] def get[T](entry: ConfigEntry[T]): T` is not public. I think we should do the same for `set`.
--
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
[GitHub] [spark] jackylee-ch commented on pull request #35950: [SPARK-38419][SQL] Change spark.sessionstate.conf.getConf/setConf operation to spark.conf.get/set
Posted by GitBox <gi...@apache.org>.
jackylee-ch commented on pull request #35950:
URL: https://github.com/apache/spark/pull/35950#issuecomment-1084288757
cc @srowen @cloud-fan @HyukjinKwon
--
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
[GitHub] [spark] jackylee-ch commented on a change in pull request #35950: [SPARK-38419][SQL] Change spark.sessionstate.conf.getConf/setConf operation to spark.conf.get/set
Posted by GitBox <gi...@apache.org>.
jackylee-ch commented on a change in pull request #35950:
URL: https://github.com/apache/spark/pull/35950#discussion_r834989295
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/RuntimeConfig.scala
##########
@@ -60,6 +60,16 @@ class RuntimeConfig private[sql](sqlConf: SQLConf = new SQLConf) {
set(key, value.toString)
}
+ /**
+ * Sets the given Spark runtime configuration property.
+ *
+ * @since 3.4.0
Review comment:
okey
--
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
[GitHub] [spark] jackylee-ch commented on pull request #35950: [SPARK-38419][SQL] change spark.sessionstate.conf.getConf/setConf operation to spark.conf.get/set
Posted by GitBox <gi...@apache.org>.
jackylee-ch commented on pull request #35950:
URL: https://github.com/apache/spark/pull/35950#issuecomment-1076334650
cc @srowen @cloud-fan @HyukjinKwon
--
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
[GitHub] [spark] jackylee-ch commented on pull request #35950: [SPARK-38419][SQL] Change spark.sessionstate.conf.getConf/setConf operation to spark.conf.get/set
Posted by GitBox <gi...@apache.org>.
jackylee-ch commented on pull request #35950:
URL: https://github.com/apache/spark/pull/35950#issuecomment-1078578801
@cloud-fan @HyukjinKwon Any more question about this pr?
BTW, can we remove the [AmplabJenkins](https://github.com/AmplabJenkins) report Since we won't use Jenkins any more?
--
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
[GitHub] [spark] jackylee-ch commented on pull request #35950: [SPARK-38419][SQL] Change spark.sessionstate.conf.getConf/setConf operation to spark.conf.get/set
Posted by GitBox <gi...@apache.org>.
jackylee-ch commented on pull request #35950:
URL: https://github.com/apache/spark/pull/35950#issuecomment-1084422216
https://issues.apache.org/jira/browse/SPARK-38713
This is the new Created Jira. @cloud-fan
--
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
[GitHub] [spark] jackylee-ch commented on pull request #35950: [SPARK-38419][SQL] Change spark.sessionstate.conf.getConf/setConf operation to spark.conf.get/set
Posted by GitBox <gi...@apache.org>.
jackylee-ch commented on pull request #35950:
URL: https://github.com/apache/spark/pull/35950#issuecomment-1084418470
> At least we shouldn't link to an unrelated JIRA... Please create a new one for this PR as it's not a minor one-line change.
Sorry, I will create one inmediatly.
--
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
[GitHub] [spark] jackylee-ch commented on pull request #35950: [SPARK-38419][SQL] Change spark.sessionstate.conf.getConf/setConf operation to spark.conf.get/set
Posted by GitBox <gi...@apache.org>.
jackylee-ch commented on pull request #35950:
URL: https://github.com/apache/spark/pull/35950#issuecomment-1084415178
> @jackylee-ch seems you link to the wrong JIRA ticket?
This is a minor change, thus I didn't create a JIRA for 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
[GitHub] [spark] cloud-fan commented on a change in pull request #35950: [SPARK-38419][SQL] Change spark.sessionstate.conf.getConf/setConf operation to spark.conf.get/set
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35950:
URL: https://github.com/apache/spark/pull/35950#discussion_r834986410
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/RuntimeConfig.scala
##########
@@ -60,6 +60,16 @@ class RuntimeConfig private[sql](sqlConf: SQLConf = new SQLConf) {
set(key, value.toString)
}
+ /**
+ * Sets the given Spark runtime configuration property.
+ *
+ * @since 3.4.0
Review comment:
non-public API doesn't need the since annotation.
--
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
[GitHub] [spark] cloud-fan closed pull request #35950: [SPARK-38419][SQL] Change spark.sessionstate.conf.getConf/setConf operation to spark.conf.get/set
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #35950:
URL: https://github.com/apache/spark/pull/35950
--
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
[GitHub] [spark] cloud-fan commented on pull request #35950: [SPARK-38419][SQL] Change spark.sessionstate.conf.getConf/setConf operation to spark.conf.get/set
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #35950:
URL: https://github.com/apache/spark/pull/35950#issuecomment-1084414123
@jackylee-ch seems you link to the wrong JIRA ticket?
--
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
[GitHub] [spark] cloud-fan commented on pull request #35950: [SPARK-38419][SQL] Change spark.sessionstate.conf.getConf/setConf operation to spark.conf.get/set
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #35950:
URL: https://github.com/apache/spark/pull/35950#issuecomment-1084413106
thanks, 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