[MINOR][DOCS][PYTHON] Adding missing boolean type for replacement value in fillna

[vundela]

What changes were proposed in this pull request?

Currently pyspark Dataframe.fillna API supports boolean type when we pass dict, but it is missing in documentation.

How was this patch tested?

spark.createDataFrame([Row(a=True),Row(a=None)]).fillna({"a" : True}).show()
+----+
| a|
+----+
|true|
|true|
+----+

Please review http://spark.apache.org/contributing.html before opening a pull request.

[felixcheung]

Can you fix up the PR title - it seems to get truncated. Also add [PYTHON] to the title
Is there any existing test to add to?

[vundela]

Hi @felixcheung Thanks for the review. I have added a small testcase.

[SparkQA]

Test build #3669 has finished for PR 17688 at commit acae068.

• This patch fails Python style tests.
• This patch merges cleanly.
• This patch adds no public classes.

[vundela]

My apologies for style failures, i have fixed them.

[felixcheung]

Jenkins, retest this please

[SparkQA]

Test build #76042 has finished for PR 17688 at commit 1ec1c0d.

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

[felixcheung]

LGTM
@holdenk what do you think?

[holdenk]

We should also update the list of types a few lines up while we are fixing this. thanks a lot for catching this @vundela

[vundela]

@holdenk Thanks for the review. Can you please let me know the line number where you are expecting list of types missing. Is this for fillna or other API?

[HyukjinKwon]

LGTM too. I just quickly checked if there are similar instances but I could not find and I checked R's one and Scala one.

[holdenk]

@vundela L1237

[felixcheung]

good catch - instead of duplicating it, perhaps just say supported data types or supported data types above

[HyukjinKwon]

I think this indicates the replacement If the value is a dict whereas param value can't be a bool as below:

>>> from pyspark.sql import Row
>>> spark.createDataFrame([Row(a=None), Row(a=True)]).fillna({"a": True}).first()
Row(a=True)
>>> spark.createDataFrame([Row(a=None), Row(a=True)]).fillna(True).first()
Row(a=None)

I can't find def fill(value: Boolean) in functions.scala. Namely, (I guess) this will call it with int (a parent type). So,

>>> spark.createDataFrame([Row(a=None), Row(a=0)]).fillna(True).first()
Row(a=1)
>>> spark.createDataFrame([Row(a=None), Row(a=0)]).fillna(False).first()
Row(a=0)

So, the current status looks correct to me.

BTW, probably, we should throw an exception in

if not isinstance(value, (float, int, long, basestring, dict)):
    raise ValueError("value should be a float, int, long, string, or dict")

in Python boolean is a int - https://www.python.org/dev/peps/pep-0285/

6. Should bool inherit from int?

=> Yes.

>>> isinstance(True, int)
True

However, this looks just a documentation fix and I guess there are many instances similar with it. I think it is fine with not fixing it here.

[holdenk]

That makes sense, I'd say that the eventual improvement would maybe be offering fill for bool for symetry with the rest of the types but its not necessary here rather than type checking for bool on the input.

[holdenk]

LGTM, thanks @HyukjinKwon for noticing the lack of bool in the scala code.

[vundela]

@holdenk, Thanks for reviewing it. I guess L1237 can't be changed until we support boolean type. L1240 specifically talks about the types of values supported in dict. Please let me know if you think otherwise.

[holdenk]

Yup, it's fine hence the LGTM :)

[vundela]

Thanks @holdenk

[HyukjinKwon]

LGTM too.

[felixcheung]

merged to master/2.2