Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-50251][PYTHON] Add getSystemProperty to PySpark SparkContext #48781

Closed
wants to merge 2 commits into from

Conversation

dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented Nov 6, 2024

What changes were proposed in this pull request?

This PR aims to add getSystemProperty to PySpark SparkContext like setSystemProperty at Apache Spark 4.0.0.

https://spark.apache.org/docs/4.0.0-preview2/api/python/reference/api/pyspark.SparkContext.setSystemProperty.html

Why are the changes needed?

Since Apache Spark 0.9.0, setSystemProperty has been provided because Python doesn't have JVM's SystemProperties concept. This is usefully used like the following.

SparkContext.setSystemProperty("spark.executor.uri", os.environ["SPARK_EXECUTOR_URI"])

This PR aims to add getSystemProperty additionally and symmetrically to provide an easier and better experience for both Spark developers and Spark App developers.

Does this PR introduce any user-facing change?

No. This is a new API.

How was this patch tested?

Pass the CIs with newly added doctests.

Was this patch authored or co-authored using generative AI tooling?

No.

@github-actions github-actions bot added the PYTHON label Nov 6, 2024
@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Nov 6, 2024

Could you review this, @xinrong-meng and @zhengruifeng ?

@github-actions github-actions bot added the DOCS label Nov 6, 2024
@dongjoon-hyun
Copy link
Member Author

Could you review this PR too, @HyukjinKwon ?

"""
SparkContext._ensure_initialized()
assert SparkContext._jvm is not None
return SparkContext._jvm.java.lang.System.getProperty(key)
Copy link
Contributor

@zhengruifeng zhengruifeng Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

three questions:
1, shall we name it to be more specific to the Java?
2, do we need to also introduce it in Scala side?
3, shall we make it a SparkSession method? so that we can implement it in SparkConnect later

also cc @cloud-fan for this new feature.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To @zhengruifeng , as I wrote in the PR description, this is a symmetric version of the existing setSystemProperty. So, no for (1) and (3). For (2), Scala can access System.getProperty directly already.

https://spark.apache.org/docs/4.0.0-preview2/api/python/reference/api/pyspark.SparkContext.setSystemProperty.html

@dongjoon-hyun
Copy link
Member Author

Could you review this PR, @viirya ?

Copy link
Member

@yaooqinn yaooqinn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from my side for polyglot feature parity

@dongjoon-hyun
Copy link
Member Author

Thank you so much, @yaooqinn !

@dongjoon-hyun
Copy link
Member Author

Thank you, @viirya !

@dongjoon-hyun
Copy link
Member Author

Merged to master. Thank you all.

@dongjoon-hyun dongjoon-hyun deleted the SPARK-50251 branch November 7, 2024 05:56
@zhengruifeng
Copy link
Contributor

Late LGTM, thanks!
It is reasonable to add this getter since we already have a setter

@dongjoon-hyun
Copy link
Member Author

Thank you, @zhengruifeng

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants