You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "jonmio (via GitHub)" <gi...@apache.org> on 2026/02/23 17:55:43 UTC
[PR] [Minor] Update SparkSessionBuilder.create to use existing SparkContext if it already exists [spark]
jonmio opened a new pull request, #54429:
URL: https://github.com/apache/spark/pull/54429
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
As titled this is a minor hygiene improvement which brings the create and getOrCreate codepaths closer together. Prior to this PR, the create codepath would always create a new SparkConf and pass that to SparkContext.getOrCreate. This is not necessary and diverges from the getOrCreate codepath when a session/SparkContext already exists.
### Why are the changes needed?
Code hygene
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
Existing tests are sufficient here
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55658][PYTHON] SparkSessionBuilder.create in PySpark classic should mirror getOrCreate path as much as possible [spark]
Posted by "holdenk (via GitHub)" <gi...@apache.org>.
holdenk commented on PR #54429:
URL: https://github.com/apache/spark/pull/54429#issuecomment-3955774504
I am concerned about changing the behavior when a user has some dynamic conf specified, would love to see test coverage for that first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55658] SparkSessionBuilder.create in PySpark classic should mirror getOrCreate path as much as possible [spark]
Posted by "jonmio (via GitHub)" <gi...@apache.org>.
jonmio commented on code in PR #54429:
URL: https://github.com/apache/spark/pull/54429#discussion_r2847927124
##########
python/pyspark/sql/session.py:
##########
@@ -610,12 +610,16 @@ def create(self) -> "SparkSession":
from pyspark.core.context import SparkContext
with self._lock:
- # Build SparkConf from options
- sparkConf = SparkConf()
- for key, value in self._options.items():
- sparkConf.set(key, str(value))
-
- sc = SparkContext.getOrCreate(sparkConf)
+ session = SparkSession._instantiatedSession
Review Comment:
Yes create always returns a new session. We are just reusing the sc of the instantiated session if it exists. I renamed the variables to make this more clear
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [Minor] Update SparkSessionBuilder.create to use existing SparkContext if it already exists [spark]
Posted by "holdenk (via GitHub)" <gi...@apache.org>.
holdenk commented on PR #54429:
URL: https://github.com/apache/spark/pull/54429#issuecomment-3946758746
This is not minor, changing the core constructor path needs a JIRA at least and some testing. I don't see a benefit to this 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
Re: [PR] [SPARK-55658][PYTHON] SparkSessionBuilder.create in PySpark classic should mirror getOrCreate path as much as possible [spark]
Posted by "jonmio (via GitHub)" <gi...@apache.org>.
jonmio commented on PR #54429:
URL: https://github.com/apache/spark/pull/54429#issuecomment-3976255369
@holdenk I added this test here -https://github.com/apache/spark/pull/54429/changes#diff-4b94e735b3f0856a6c13ccbee9ab6519d07543d17370d9835006dbbdbbfc8d57R630. Could you please take another look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [Minor] Update SparkSessionBuilder.create to use existing SparkContext if it already exists [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54429:
URL: https://github.com/apache/spark/pull/54429#discussion_r2844323333
##########
python/pyspark/sql/session.py:
##########
@@ -610,12 +610,16 @@ def create(self) -> "SparkSession":
from pyspark.core.context import SparkContext
with self._lock:
- # Build SparkConf from options
- sparkConf = SparkConf()
- for key, value in self._options.items():
- sparkConf.set(key, str(value))
-
- sc = SparkContext.getOrCreate(sparkConf)
+ session = SparkSession._instantiatedSession
Review Comment:
what does this mean? I think `def create` should always create a new session?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55658][PYTHON] SparkSessionBuilder.create in PySpark classic should mirror getOrCreate path as much as possible [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #54429:
URL: https://github.com/apache/spark/pull/54429#issuecomment-3957975094
@holdenk we only allow one `SparkContext` instance per driver JVM, so even if we try to create `SparkContext` before this PR, it will return the existing one too. I think this PR just make the code more explicit about it, plus saving the creation of `SparkConf`. It should not have any behavior 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
Re: [PR] [SPARK-55658][PYTHON] SparkSessionBuilder.create in PySpark classic should mirror getOrCreate path as much as possible [spark]
Posted by "jonmio (via GitHub)" <gi...@apache.org>.
jonmio commented on PR #54429:
URL: https://github.com/apache/spark/pull/54429#issuecomment-3995019293
@holdenk just want to followup on this. Could you please take another look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55658][PYTHON] SparkSessionBuilder.create in PySpark classic should mirror getOrCreate path as much as possible [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #54429: [SPARK-55658][PYTHON] SparkSessionBuilder.create in PySpark classic should mirror getOrCreate path as much as possible
URL: https://github.com/apache/spark/pull/54429
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55658] SparkSessionBuilder.create in PySpark classic should mirror getOrCreate path as much as possible [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on PR #54429:
URL: https://github.com/apache/spark/pull/54429#issuecomment-3954729917
@holdenk Just want to confirm before merging — are you okay with the latest changes in this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55658] SparkSessionBuilder.create in PySpark classic should mirror getOrCreate path as much as possible [spark]
Posted by "jonmio (via GitHub)" <gi...@apache.org>.
jonmio commented on PR #54429:
URL: https://github.com/apache/spark/pull/54429#issuecomment-3953037351
> This is not minor, changing the core constructor path needs a JIRA at least and some testing.
@holdenk I created a JIRA ticket and linked it to this PR. Also added a test verifying the change made here. While this does change the core `create` codepath it is only modifying a [very recently added branch for PySpark classic](https://github.com/apache/spark/pull/53820). This was added around 6 weeks ago.
> I don't see a benefit to this change.
The main benefits here are minimizing divergence with the `getOrCreate` codepath and removing the unnecessary `SparkConf` creation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55658] SparkSessionBuilder.create in PySpark classic should mirror getOrCreate path as much as possible [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54429:
URL: https://github.com/apache/spark/pull/54429#discussion_r2847993550
##########
python/pyspark/sql/session.py:
##########
@@ -610,12 +610,16 @@ def create(self) -> "SparkSession":
from pyspark.core.context import SparkContext
with self._lock:
- # Build SparkConf from options
- sparkConf = SparkConf()
- for key, value in self._options.items():
- sparkConf.set(key, str(value))
-
- sc = SparkContext.getOrCreate(sparkConf)
+ instantiated_session = SparkSession._instantiatedSession
+ # Get SparkContext
+ if instantiated_session is None or instantiated_session._sc._jsc is None:
Review Comment:
nit: I feel it's more readable to write the condition in a positive way:
```
if instantiated_session is not None or instantiated_session._sc._jsc is not None:
// return existing SparkContext
...
else:
...
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55658][PYTHON] SparkSessionBuilder.create in PySpark classic should mirror getOrCreate path as much as possible [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #54429:
URL: https://github.com/apache/spark/pull/54429#issuecomment-4380673469
thanks, merging to master/4.x/4.2!
--
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