[SPARK-23706][PYTHON] spark.conf.get(value, default=None) should produce None in PySpark #20841

HyukjinKwon · 2018-03-16T04:37:49Z

What changes were proposed in this pull request?

Scala:

scala> spark.conf.get("hey", null)
res1: String = null

scala> spark.conf.get("spark.sql.sources.partitionOverwriteMode", null)
res2: String = null

Python:

Before

>>> spark.conf.get("hey", None)
...
py4j.protocol.Py4JJavaError: An error occurred while calling o30.get.
: java.util.NoSuchElementException: hey
...

>>> spark.conf.get("spark.sql.sources.partitionOverwriteMode", None)
u'STATIC'

After

>>> spark.conf.get("hey", None) is None
True

>>> spark.conf.get("spark.sql.sources.partitionOverwriteMode", None) is None
True

*Note that this PR preserves the case below:

>>> spark.conf.get("spark.sql.sources.partitionOverwriteMode")
u'STATIC'

How was this patch tested?

Manually tested and unit tests were added.

HyukjinKwon · 2018-03-16T04:39:19Z

cc @cloud-fan, @viirya, @ueshin, @BryanCutler who I can directly think of for now.

SparkQA · 2018-03-16T05:03:48Z

Test build #88291 has finished for PR 20841 at commit 1a6cfce.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-03-16T05:15:06Z

retest this please

SparkQA · 2018-03-16T05:48:46Z

Test build #88294 has finished for PR 20841 at commit 1a6cfce.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2018-03-16T06:24:11Z

python/pyspark/sql/context.py

+    def getConf(self, key, defaultValue=_NoValue):
        """Returns the value of Spark SQL configuration property for the given key.

        If the key is not set and defaultValue is not None, return


We should update this doc.

SparkQA · 2018-03-16T07:05:02Z

Test build #88298 has finished for PR 20841 at commit d8ee18f.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2018-03-16T07:16:00Z

retest this please.

SparkQA · 2018-03-16T07:48:27Z

Test build #88300 has finished for PR 20841 at commit d8ee18f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

ueshin

LGTM except for one comment.

ueshin · 2018-03-16T17:32:39Z

python/pyspark/sql/tests.py


+        self.assertRaisesRegexp(Exception, "hyukjin", lambda: spark.conf.get("hyukjin"))
+        self.assertEqual(spark.conf.get("hyukjin", None), None)
+        self.assertEqual(spark.conf.get("spark.sql.sources.partitionOverwriteMode", None), None)


Can you add one more test to get "spark.sql.sources.partitionOverwriteMode" without None to see the difference between with and without None? Hopefully with a simple comment.

BryanCutler · 2018-03-16T22:54:49Z

Shouldn't it be the same as in the application conf here https://github.com/apache/spark/blob/master/python/pyspark/conf.py#L174?

Here the default is None and it returns None if the conf doesn't contain the and the user doesn't explicitly set a default. There isn't really any value in getting a PY4JError right?

HyukjinKwon · 2018-03-17T00:10:15Z

I think we should better match Python side behaviour to Scala side in general and throw a better exception if possible. I think the application conf was done like that because there wasn't _NoValue concept at that time. Both sql conf and application conf were not quite the same already ..

BTW, could I ask to fix it separately please? I actually have found several places that we might have to consider _NoValue ..

BryanCutler · 2018-03-17T01:34:06Z

Yeah, true that is how the Scala side works so I suppose that is best, but I kind of view this api similar to the python dict.get which returns None and won't raise an error, and that would act like getOption in scala. I don't feel strongly though, so up to you.

HyukjinKwon · 2018-03-17T04:42:08Z

Ah, I got the point. So, you mean a dictionary-like approach - get("non-existant-key") should return None always instead of throwing an error, language-specifically? I think we might consider to allow other dictionary-like operations together similarly but otherwise it might be fine to leave it consistent with Scala side.

Strictly, I blieve this PR doesn't change that behaviour if I understood correctly but just preserves it anyway.

HyukjinKwon · 2018-03-17T04:50:14Z

python/pyspark/sql/tests.py

        spark.conf.unset("bogo")
        self.assertEqual(spark.conf.get("bogo", "colombia"), "colombia")

+        self.assertRaisesRegexp(Exception, "hyukjin", lambda: spark.conf.get("hyukjin"))


@BryanCutler, so this test case prompted you. It's not related with this PR:

Before

>>> spark.conf.get("hyukjin") ... : java.util.NoSuchElementException: hyukjin ...

After

>>> spark.conf.get("hyukjin") ... : java.util.NoSuchElementException: hyukjin ...

Let me take this out.

SparkQA · 2018-03-17T05:31:10Z

Test build #88336 has finished for PR 20841 at commit 3a8de04.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-03-17T07:14:07Z

retest this please

SparkQA · 2018-03-17T07:47:24Z

Test build #88339 has finished for PR 20841 at commit 3a8de04.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

BryanCutler

LGTM

viirya · 2018-03-18T08:56:58Z

LGTM.

HyukjinKwon · 2018-03-18T11:29:51Z

Merged to master and branch-2.3.

Thank you @ueshin, @BryanCutler and @viirya for reviewing this.

…uce None in PySpark Scala: ``` scala> spark.conf.get("hey", null) res1: String = null ``` ``` scala> spark.conf.get("spark.sql.sources.partitionOverwriteMode", null) res2: String = null ``` Python: **Before** ``` >>> spark.conf.get("hey", None) ... py4j.protocol.Py4JJavaError: An error occurred while calling o30.get. : java.util.NoSuchElementException: hey ... ``` ``` >>> spark.conf.get("spark.sql.sources.partitionOverwriteMode", None) u'STATIC' ``` **After** ``` >>> spark.conf.get("hey", None) is None True ``` ``` >>> spark.conf.get("spark.sql.sources.partitionOverwriteMode", None) is None True ``` *Note that this PR preserves the case below: ``` >>> spark.conf.get("spark.sql.sources.partitionOverwriteMode") u'STATIC' ``` Manually tested and unit tests were added. Author: hyukjinkwon <[email protected]> Closes #20841 from HyukjinKwon/spark-conf-get. (cherry picked from commit 61487b3) Signed-off-by: hyukjinkwon <[email protected]>

…uce None in PySpark ## What changes were proposed in this pull request? Scala: ``` scala> spark.conf.get("hey", null) res1: String = null ``` ``` scala> spark.conf.get("spark.sql.sources.partitionOverwriteMode", null) res2: String = null ``` Python: **Before** ``` >>> spark.conf.get("hey", None) ... py4j.protocol.Py4JJavaError: An error occurred while calling o30.get. : java.util.NoSuchElementException: hey ... ``` ``` >>> spark.conf.get("spark.sql.sources.partitionOverwriteMode", None) u'STATIC' ``` **After** ``` >>> spark.conf.get("hey", None) is None True ``` ``` >>> spark.conf.get("spark.sql.sources.partitionOverwriteMode", None) is None True ``` *Note that this PR preserves the case below: ``` >>> spark.conf.get("spark.sql.sources.partitionOverwriteMode") u'STATIC' ``` ## How was this patch tested? Manually tested and unit tests were added. Author: hyukjinkwon <[email protected]> Closes apache#20841 from HyukjinKwon/spark-conf-get.

…uce None in PySpark Scala: ``` scala> spark.conf.get("hey", null) res1: String = null ``` ``` scala> spark.conf.get("spark.sql.sources.partitionOverwriteMode", null) res2: String = null ``` Python: **Before** ``` >>> spark.conf.get("hey", None) ... py4j.protocol.Py4JJavaError: An error occurred while calling o30.get. : java.util.NoSuchElementException: hey ... ``` ``` >>> spark.conf.get("spark.sql.sources.partitionOverwriteMode", None) u'STATIC' ``` **After** ``` >>> spark.conf.get("hey", None) is None True ``` ``` >>> spark.conf.get("spark.sql.sources.partitionOverwriteMode", None) is None True ``` *Note that this PR preserves the case below: ``` >>> spark.conf.get("spark.sql.sources.partitionOverwriteMode") u'STATIC' ``` Manually tested and unit tests were added. Author: hyukjinkwon <[email protected]> Closes apache#20841 from HyukjinKwon/spark-conf-get. (cherry picked from commit 61487b3) Signed-off-by: hyukjinkwon <[email protected]>

spark.conf.get(value, default=None) should produce None in PySpark

1a6cfce

viirya reviewed Mar 16, 2018

View reviewed changes

Fix few words

d8ee18f

ueshin reviewed Mar 16, 2018

View reviewed changes

HyukjinKwon commented Mar 17, 2018

View reviewed changes

Adds one more test with some comments

3a8de04

BryanCutler approved these changes Mar 17, 2018

View reviewed changes

asfgit closed this in 61487b3 Mar 18, 2018

HyukjinKwon deleted the spark-conf-get branch October 16, 2018 12:45

[SPARK-23706][PYTHON] spark.conf.get(value, default=None) should produce None in PySpark #20841

[SPARK-23706][PYTHON] spark.conf.get(value, default=None) should produce None in PySpark #20841

Uh oh!

Conversation

HyukjinKwon commented Mar 16, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

HyukjinKwon commented Mar 16, 2018

Uh oh!

SparkQA commented Mar 16, 2018

Uh oh!

HyukjinKwon commented Mar 16, 2018

Uh oh!

SparkQA commented Mar 16, 2018

Uh oh!

viirya Mar 16, 2018

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 16, 2018

Uh oh!

viirya commented Mar 16, 2018

Uh oh!

SparkQA commented Mar 16, 2018

Uh oh!

ueshin left a comment

Choose a reason for hiding this comment

Uh oh!

ueshin Mar 16, 2018

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Mar 17, 2018

Choose a reason for hiding this comment

Uh oh!

BryanCutler commented Mar 16, 2018

Uh oh!

HyukjinKwon commented Mar 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BryanCutler commented Mar 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HyukjinKwon commented Mar 17, 2018

Uh oh!

HyukjinKwon Mar 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 17, 2018

Uh oh!

HyukjinKwon commented Mar 17, 2018

Uh oh!

SparkQA commented Mar 17, 2018

Uh oh!

BryanCutler left a comment

Choose a reason for hiding this comment

Uh oh!

viirya commented Mar 18, 2018

Uh oh!

HyukjinKwon commented Mar 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HyukjinKwon commented Mar 17, 2018 •

edited

Loading

BryanCutler commented Mar 17, 2018 •

edited

Loading

HyukjinKwon Mar 17, 2018 •

edited

Loading