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 2020/10/15 01:54:50 UTC
[GitHub] [spark] leanken opened a new pull request #30042: protect setActionSession and clearActiveSession
leanken opened a new pull request #30042:
URL: https://github.com/apache/spark/pull/30042
### What changes were proposed in this pull request?
This PR is a sub-task of [SPARK-33138](https://issues.apache.org/jira/browse/SPARK-33138). In order to make SQLConf.get reliable and stable, we need to make sure user can't pollute the SQLConf and SparkSession Context via calling setActiveSession and clearActiveSession.
Change of the PR:
* add legacy config spark.sql.legacy.allowModifyActiveSession to fallback to old behavior if user do need to call these two API.
* by default, if user call these two API, it will throw exception
* add extra two internal and private API setActiveSessionInternal and clearActiveSessionInternal for current internal usage
* change all internal reference to new internal API except for SQLContext.setActive and SQLContext.clearActive
### Why are the changes needed?
Make SQLConf.get reliable and stable.
### Does this PR introduce any user-facing change?
No.
### How was this patch tested?
* Add UT in SparkSessionBuilderSuite to test the legacy config
* Existing test
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-708953882
**[Test build #129818 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129818/testReport)** for PR 30042 at commit [`d4db8ca`](https://github.com/apache/spark/commit/d4db8ca6228cb564b0bbbdb6170fae369aa47eca).
----------------------------------------------------------------
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.
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] zsxwing commented on a change in pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
zsxwing commented on a change in pull request #30042:
URL: https://github.com/apache/spark/pull/30042#discussion_r521813419
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala
##########
@@ -984,7 +984,16 @@ object SparkSession extends Logging {
*
* @since 2.0.0
*/
+ @deprecated("This method is deprecated and will be removed in future versions.", "3.1.0")
def setActiveSession(session: SparkSession): Unit = {
+ if (SQLConf.get.legacyAllowModifyActiveSession) {
+ setActiveSessionInternal(session)
+ } else {
+ throw new UnsupportedOperationException("Not allowed to modify active Spark session.")
Review comment:
Can we add the flag in the error message? Then users will know how to unblock themselves without googling.
----------------------------------------------------------------
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.
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 #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-708888128
----------------------------------------------------------------
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.
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 #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-709300040
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-709053594
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34423/
----------------------------------------------------------------
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.
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 #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-709817334
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.
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 removed a comment on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-709312903
----------------------------------------------------------------
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.
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 removed a comment on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-708887075
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/129783/
Test FAILed.
----------------------------------------------------------------
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.
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 removed a comment on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-708946779
Merged build finished. Test FAILed.
----------------------------------------------------------------
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.
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 removed a comment on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-708887069
Merged build finished. Test FAILed.
----------------------------------------------------------------
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.
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] leanken commented on a change in pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
leanken commented on a change in pull request #30042:
URL: https://github.com/apache/spark/pull/30042#discussion_r505967525
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala
##########
@@ -984,7 +984,16 @@ object SparkSession extends Logging {
*
* @since 2.0.0
*/
+ @deprecated("This method is deprecated and will be removed in future versions.", "3.1.0")
Review comment:
done
----------------------------------------------------------------
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.
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 removed a comment on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-708888136
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/34390/
Test FAILed.
----------------------------------------------------------------
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.
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 #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #30042:
URL: https://github.com/apache/spark/pull/30042#discussion_r505158893
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala
##########
@@ -985,6 +985,14 @@ object SparkSession extends Logging {
* @since 2.0.0
*/
def setActiveSession(session: SparkSession): Unit = {
Review comment:
let's add deprecated 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.
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] HyukjinKwon commented on a change in pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #30042:
URL: https://github.com/apache/spark/pull/30042#discussion_r506085277
##########
File path: python/pyspark/sql/session.py
##########
@@ -230,7 +230,10 @@ def __init__(self, sparkContext, jsparkSession=None):
SparkSession._instantiatedSession = self
SparkSession._activeSession = self
self._jvm.SparkSession.setDefaultSession(self._jsparkSession)
- self._jvm.SparkSession.setActiveSession(self._jsparkSession)
+ self._jvm.java.lang.Class.forName("org.apache.spark.sql.SparkSession$")\
+ .getDeclaredField("MODULE$")\
+ .get(None)\
+ .setActiveSessionInternal(self._jsparkSession)
Review comment:
Hey, you don't need to manually reflect here. package level private accessor is already accessible in Java as you did so you can just mimic it here via `getattr(getattr(spark._jvm, "SparkSession$"), "MODULE$").setActiveSessionInternal`(...).
----------------------------------------------------------------
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.
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 removed a comment on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-708962035
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/34410/
Test FAILed.
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-709037161
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34423/
----------------------------------------------------------------
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.
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] leanken commented on a change in pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
leanken commented on a change in pull request #30042:
URL: https://github.com/apache/spark/pull/30042#discussion_r505967587
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SparkSessionBuilderSuite.scala
##########
@@ -281,4 +281,23 @@ class SparkSessionBuilderSuite extends SparkFunSuite with BeforeAndAfterEach {
()
}
}
+
+ test("SPARK-33139: Test spark.sql.legacy.allowModifyActiveSession") {
+ Seq(true, false).foreach { allowModifyActiveSession =>
+ val session = SparkSession.builder()
+ .master("local")
+ .config(SQLConf.LEGACY_ALLOW_MODIFY_ACTIVE_SESSION.key, allowModifyActiveSession.toString)
Review comment:
done
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
##########
@@ -2675,6 +2675,15 @@ object SQLConf {
.checkValues(LegacyBehaviorPolicy.values.map(_.toString))
.createWithDefault(LegacyBehaviorPolicy.EXCEPTION.toString)
+ val LEGACY_ALLOW_MODIFY_ACTIVE_SESSION =
Review comment:
done
----------------------------------------------------------------
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.
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] leanken commented on a change in pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
leanken commented on a change in pull request #30042:
URL: https://github.com/apache/spark/pull/30042#discussion_r505371784
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala
##########
@@ -984,7 +984,16 @@ object SparkSession extends Logging {
*
* @since 2.0.0
*/
+ @deprecated("This method is deprecated and will be removed in future versions.", "3.1.0")
Review comment:
unlike other deprecated API, this API does not have a better "Option"; we kind of hoping that user not touch these API at all. I will consider putting the legacy config in the error message as a hint in case user really really need these two API.
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-708845249
**[Test build #129783 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129783/testReport)** for PR 30042 at commit [`fc8a91a`](https://github.com/apache/spark/commit/fc8a91a2bebf209ed44e0cb29afce36ff1f9b063).
----------------------------------------------------------------
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.
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] leanken commented on a change in pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
leanken commented on a change in pull request #30042:
URL: https://github.com/apache/spark/pull/30042#discussion_r505183253
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala
##########
@@ -985,6 +985,14 @@ object SparkSession extends Logging {
* @since 2.0.0
*/
def setActiveSession(session: SparkSession): Unit = {
Review comment:
done
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala
##########
@@ -985,6 +985,14 @@ object SparkSession extends Logging {
* @since 2.0.0
*/
def setActiveSession(session: SparkSession): Unit = {
+ if (SQLConf.get.legacyAllowModifyActiveSession) {
+ setActiveSessionInternal(session)
+ } else {
+ throw new IllegalStateException("Not allowed to modify active Spark session.")
Review comment:
done
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-708946582
**[Test build #129805 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129805/testReport)** for PR 30042 at commit [`90c3515`](https://github.com/apache/spark/commit/90c3515dc13d060e9a7c120d9dfa77c6076f8f0f).
* This patch **fails due to an unknown error code, -9**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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.
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] yaooqinn commented on a change in pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #30042:
URL: https://github.com/apache/spark/pull/30042#discussion_r505371108
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SparkSessionBuilderSuite.scala
##########
@@ -281,4 +281,23 @@ class SparkSessionBuilderSuite extends SparkFunSuite with BeforeAndAfterEach {
()
}
}
+
+ test("SPARK-33139: Test spark.sql.legacy.allowModifyActiveSession") {
+ Seq(true, false).foreach { allowModifyActiveSession =>
+ val session = SparkSession.builder()
+ .master("local")
+ .config(SQLConf.LEGACY_ALLOW_MODIFY_ACTIVE_SESSION.key, allowModifyActiveSession.toString)
Review comment:
nit: allowModifyActiveSession is enough 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.
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] leanken commented on a change in pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
leanken commented on a change in pull request #30042:
URL: https://github.com/apache/spark/pull/30042#discussion_r505404869
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala
##########
@@ -984,7 +984,16 @@ object SparkSession extends Logging {
*
* @since 2.0.0
*/
+ @deprecated("This method is deprecated and will be removed in future versions.", "3.1.0")
Review comment:
sure, i will update the migration guide as well.
----------------------------------------------------------------
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.
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 #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #30042:
URL: https://github.com/apache/spark/pull/30042
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-708886939
**[Test build #129783 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129783/testReport)** for PR 30042 at commit [`fc8a91a`](https://github.com/apache/spark/commit/fc8a91a2bebf209ed44e0cb29afce36ff1f9b063).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-708950379
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34410/
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-709311726
**[Test build #129818 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129818/testReport)** for PR 30042 at commit [`d4db8ca`](https://github.com/apache/spark/commit/d4db8ca6228cb564b0bbbdb6170fae369aa47eca).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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.
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 #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-708887069
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-709231930
**[Test build #129835 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129835/testReport)** for PR 30042 at commit [`aa7bf5b`](https://github.com/apache/spark/commit/aa7bf5b50c4388e6caa24826d1bc83572af60e5a).
----------------------------------------------------------------
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.
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 #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-726575683
Sorry, we may need to revert this. I got feedback about these 2 APIs, which can be valid use cases: Users don't want to pass a `SparkSession` parameter all the way around, and want to leverage the set ad get API (The same reason for using SQLConf.get in Spark).
On the other hand, these 2 APIs are not very risky. Spark always wrap rules execution with `session.withActive`, so end-users can't really change the active session in the middle(active session is thread-local). It's not 100% safe as users can write a UDF to change active session, and the UDF can be executed in the middle by the constant folding rule, but that's a really corner case.
@leanken what do you think?
----------------------------------------------------------------
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.
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] HyukjinKwon commented on a change in pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #30042:
URL: https://github.com/apache/spark/pull/30042#discussion_r506086362
##########
File path: python/pyspark/sql/session.py
##########
@@ -230,7 +230,10 @@ def __init__(self, sparkContext, jsparkSession=None):
SparkSession._instantiatedSession = self
SparkSession._activeSession = self
self._jvm.SparkSession.setDefaultSession(self._jsparkSession)
- self._jvm.SparkSession.setActiveSession(self._jsparkSession)
+ self._jvm.java.lang.Class.forName("org.apache.spark.sql.SparkSession$")\
Review comment:
`Class.forName` should better not directly used. This is banned by Scala style:
https://github.com/apache/spark/blob/e93b8f02cd706bedc47c9b55a73f632fe9e61ec3/scalastyle-config.xml#L197-L206
----------------------------------------------------------------
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.
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 removed a comment on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-709053612
----------------------------------------------------------------
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.
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 #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-709053612
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-708921252
**[Test build #129805 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129805/testReport)** for PR 30042 at commit [`90c3515`](https://github.com/apache/spark/commit/90c3515dc13d060e9a7c120d9dfa77c6076f8f0f).
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-708962007
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34410/
----------------------------------------------------------------
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.
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] HyukjinKwon commented on a change in pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #30042:
URL: https://github.com/apache/spark/pull/30042#discussion_r506087792
##########
File path: python/pyspark/sql/session.py
##########
@@ -230,7 +230,10 @@ def __init__(self, sparkContext, jsparkSession=None):
SparkSession._instantiatedSession = self
SparkSession._activeSession = self
self._jvm.SparkSession.setDefaultSession(self._jsparkSession)
- self._jvm.SparkSession.setActiveSession(self._jsparkSession)
+ self._jvm.java.lang.Class.forName("org.apache.spark.sql.SparkSession$")\
+ .getDeclaredField("MODULE$")\
+ .get(None)\
+ .setActiveSessionInternal(self._jsparkSession)
Review comment:
Thanks, please go ahead for a followup.
----------------------------------------------------------------
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.
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 #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #30042:
URL: https://github.com/apache/spark/pull/30042#discussion_r505159391
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala
##########
@@ -985,6 +985,14 @@ object SparkSession extends Logging {
* @since 2.0.0
*/
def setActiveSession(session: SparkSession): Unit = {
+ if (SQLConf.get.legacyAllowModifyActiveSession) {
+ setActiveSessionInternal(session)
+ } else {
+ throw new IllegalStateException("Not allowed to modify active Spark session.")
Review comment:
`UnsupportedOperationException` is better IMO
----------------------------------------------------------------
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.
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] leanken commented on a change in pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
leanken commented on a change in pull request #30042:
URL: https://github.com/apache/spark/pull/30042#discussion_r506086345
##########
File path: python/pyspark/sql/session.py
##########
@@ -230,7 +230,10 @@ def __init__(self, sparkContext, jsparkSession=None):
SparkSession._instantiatedSession = self
SparkSession._activeSession = self
self._jvm.SparkSession.setDefaultSession(self._jsparkSession)
- self._jvm.SparkSession.setActiveSession(self._jsparkSession)
+ self._jvm.java.lang.Class.forName("org.apache.spark.sql.SparkSession$")\
+ .getDeclaredField("MODULE$")\
+ .get(None)\
+ .setActiveSessionInternal(self._jsparkSession)
Review comment:
OK, I will test and update in next PR, thanks @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.
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] SparkQA commented on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-708921252
**[Test build #129805 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129805/testReport)** for PR 30042 at commit [`90c3515`](https://github.com/apache/spark/commit/90c3515dc13d060e9a7c120d9dfa77c6076f8f0f).
----------------------------------------------------------------
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.
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] yaooqinn commented on a change in pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #30042:
URL: https://github.com/apache/spark/pull/30042#discussion_r505374682
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
##########
@@ -2675,6 +2675,15 @@ object SQLConf {
.checkValues(LegacyBehaviorPolicy.values.map(_.toString))
.createWithDefault(LegacyBehaviorPolicy.EXCEPTION.toString)
+ val LEGACY_ALLOW_MODIFY_ACTIVE_SESSION =
Review comment:
Should this be a static config? I am not quite sure if there is any case that one SparkSession can set this to true, then other SparkSessions are affected silently.
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-709285209
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34440/
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-708888118
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34390/
----------------------------------------------------------------
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.
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 removed a comment on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-708962028
Merged build finished. Test FAILed.
----------------------------------------------------------------
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.
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] leanken commented on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
leanken commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-708846151
@cloud-fan FYI
----------------------------------------------------------------
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.
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] leanken commented on a change in pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
leanken commented on a change in pull request #30042:
URL: https://github.com/apache/spark/pull/30042#discussion_r505183314
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/SQLContext.scala
##########
@@ -1031,6 +1031,7 @@ object SQLContext {
*/
@deprecated("Use SparkSession.setActiveSession instead", "2.0.0")
def setActive(sqlContext: SQLContext): Unit = {
+ // For code-review, this is the only place that call SparkSession.setActiveSession
Review comment:
removed
----------------------------------------------------------------
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.
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] leanken commented on a change in pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
leanken commented on a change in pull request #30042:
URL: https://github.com/apache/spark/pull/30042#discussion_r505405113
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
##########
@@ -2675,6 +2675,15 @@ object SQLConf {
.checkValues(LegacyBehaviorPolicy.values.map(_.toString))
.createWithDefault(LegacyBehaviorPolicy.EXCEPTION.toString)
+ val LEGACY_ALLOW_MODIFY_ACTIVE_SESSION =
Review comment:
sounds reasonable, will update.
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-709442905
**[Test build #129835 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129835/testReport)** for PR 30042 at commit [`aa7bf5b`](https://github.com/apache/spark/commit/aa7bf5b50c4388e6caa24826d1bc83572af60e5a).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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.
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] yaooqinn commented on a change in pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #30042:
URL: https://github.com/apache/spark/pull/30042#discussion_r505358074
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala
##########
@@ -984,7 +984,16 @@ object SparkSession extends Logging {
*
* @since 2.0.0
*/
+ @deprecated("This method is deprecated and will be removed in future versions.", "3.1.0")
Review comment:
Shall we add some comments to tell users how to restore here or in the error msg?
----------------------------------------------------------------
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.
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 #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #30042:
URL: https://github.com/apache/spark/pull/30042#discussion_r521818085
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala
##########
@@ -984,7 +984,16 @@ object SparkSession extends Logging {
*
* @since 2.0.0
*/
+ @deprecated("This method is deprecated and will be removed in future versions.", "3.1.0")
def setActiveSession(session: SparkSession): Unit = {
+ if (SQLConf.get.legacyAllowModifyActiveSession) {
+ setActiveSessionInternal(session)
+ } else {
+ throw new UnsupportedOperationException("Not allowed to modify active Spark session.")
Review comment:
+1. @leanken can you create a follow-up? thanks!
----------------------------------------------------------------
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.
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] yaooqinn commented on a change in pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #30042:
URL: https://github.com/apache/spark/pull/30042#discussion_r505361174
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SparkSessionBuilderSuite.scala
##########
@@ -281,4 +281,23 @@ class SparkSessionBuilderSuite extends SparkFunSuite with BeforeAndAfterEach {
()
}
}
+
+ test("SPARK-33139: Test spark.sql.legacy.allowModifyActiveSession") {
Review comment:
Shall we use a more meaningful test name 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.
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] leanken commented on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
leanken commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-726589343
> Sorry, we may need to revert this. I got feedback about these 2 APIs, which can be valid use cases: Users don't want to pass a `SparkSession` parameter all the way around, and want to leverage the set ad get API (The same reason for using SQLConf.get in Spark).
>
> On the other hand, these 2 APIs are not very risky. Spark always wrap rules execution with `session.withActive`, so end-users can't really change the active session in the middle(active session is thread-local). It's not 100% safe as users can write a UDF to change active session, and the UDF can be executed in the middle by the constant folding rule, but that's a really corner case.
>
> @leanken what do you think?
OK. I will do the revert.
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-708879173
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34390/
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-708953882
**[Test build #129818 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129818/testReport)** for PR 30042 at commit [`d4db8ca`](https://github.com/apache/spark/commit/d4db8ca6228cb564b0bbbdb6170fae369aa47eca).
----------------------------------------------------------------
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.
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] SparkQA removed a comment on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-709231930
**[Test build #129835 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129835/testReport)** for PR 30042 at commit [`aa7bf5b`](https://github.com/apache/spark/commit/aa7bf5b50c4388e6caa24826d1bc83572af60e5a).
----------------------------------------------------------------
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.
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 removed a comment on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-708888128
Merged build finished. Test FAILed.
----------------------------------------------------------------
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.
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 removed a comment on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-709444242
----------------------------------------------------------------
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.
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] leanken commented on a change in pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
leanken commented on a change in pull request #30042:
URL: https://github.com/apache/spark/pull/30042#discussion_r505365788
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SparkSessionBuilderSuite.scala
##########
@@ -281,4 +281,23 @@ class SparkSessionBuilderSuite extends SparkFunSuite with BeforeAndAfterEach {
()
}
}
+
+ test("SPARK-33139: Test spark.sql.legacy.allowModifyActiveSession") {
Review comment:
how about
"SPARK-33139: Test SparkSession.setActiveSession/clearActiveSession"
----------------------------------------------------------------
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.
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] leanken commented on a change in pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
leanken commented on a change in pull request #30042:
URL: https://github.com/apache/spark/pull/30042#discussion_r505372747
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SparkSessionBuilderSuite.scala
##########
@@ -281,4 +281,23 @@ class SparkSessionBuilderSuite extends SparkFunSuite with BeforeAndAfterEach {
()
}
}
+
+ test("SPARK-33139: Test spark.sql.legacy.allowModifyActiveSession") {
+ Seq(true, false).foreach { allowModifyActiveSession =>
+ val session = SparkSession.builder()
+ .master("local")
+ .config(SQLConf.LEGACY_ALLOW_MODIFY_ACTIVE_SESSION.key, allowModifyActiveSession.toString)
Review comment:
thanks
----------------------------------------------------------------
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.
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 removed a comment on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-708946786
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/129805/
Test FAILed.
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #30042: protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-708845249
**[Test build #129783 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129783/testReport)** for PR 30042 at commit [`fc8a91a`](https://github.com/apache/spark/commit/fc8a91a2bebf209ed44e0cb29afce36ff1f9b063).
----------------------------------------------------------------
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.
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 #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-709312903
----------------------------------------------------------------
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.
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 #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #30042:
URL: https://github.com/apache/spark/pull/30042#discussion_r505158762
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/SQLContext.scala
##########
@@ -1031,6 +1031,7 @@ object SQLContext {
*/
@deprecated("Use SparkSession.setActiveSession instead", "2.0.0")
def setActive(sqlContext: SQLContext): Unit = {
+ // For code-review, this is the only place that call SparkSession.setActiveSession
Review comment:
you can put it as a PR comment, not code comment that gets committed to the code base.
----------------------------------------------------------------
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.
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 removed a comment on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-709300040
----------------------------------------------------------------
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.
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] leanken commented on a change in pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
leanken commented on a change in pull request #30042:
URL: https://github.com/apache/spark/pull/30042#discussion_r521847730
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala
##########
@@ -984,7 +984,16 @@ object SparkSession extends Logging {
*
* @since 2.0.0
*/
+ @deprecated("This method is deprecated and will be removed in future versions.", "3.1.0")
def setActiveSession(session: SparkSession): Unit = {
+ if (SQLConf.get.legacyAllowModifyActiveSession) {
+ setActiveSessionInternal(session)
+ } else {
+ throw new UnsupportedOperationException("Not allowed to modify active Spark session.")
Review comment:
OK.
----------------------------------------------------------------
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.
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] leanken commented on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
leanken commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-709677024
@cloud-fan Test passed, if no more comment, ready to merge.
----------------------------------------------------------------
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.
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] SparkQA commented on pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-709300000
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34440/
----------------------------------------------------------------
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.
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] yaooqinn commented on a change in pull request #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #30042:
URL: https://github.com/apache/spark/pull/30042#discussion_r505387231
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala
##########
@@ -984,7 +984,16 @@ object SparkSession extends Logging {
*
* @since 2.0.0
*/
+ @deprecated("This method is deprecated and will be removed in future versions.", "3.1.0")
Review comment:
I agree that these APIs are not something users can call casually.
But I am not sure the deprecated annotation can cover the behavior change here. The config is internal and will not go to the public doc. Maybe we should add a migration guide if this is not only deprecated but also desupported
----------------------------------------------------------------
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.
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 #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-708962028
----------------------------------------------------------------
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.
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 #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-709444242
----------------------------------------------------------------
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.
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 #30042: [SPARK-33139][SQL] protect setActionSession and clearActiveSession
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30042:
URL: https://github.com/apache/spark/pull/30042#issuecomment-708946779
----------------------------------------------------------------
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.
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