SPARK-1630: Make PythonRDD handle NULL elements and strings gracefully

[kalpit]

Have added a unit test that validates the fix. We no longer NPE.

[rxin]

It's more obvious to just do

if (other ==null) {
} else {
}

then a pattern matching.

[AmplabJenkins]

Can one of the admins verify this patch?

[rxin]

here also

[rxin]

Thanks, @kalpit. This looks pretty good. I left a couple comments on style.

[rxin]

the indent is off here (2-space indent)

[rxin]

Can you update this one also?

[rxin]

Thanks - just one more tiny thing about indent ...

[mateiz]

Jenkins, test this please

[AmplabJenkins]

Merged build triggered.

[AmplabJenkins]

Merged build started.

[mateiz]

I'm curious, when did you get nulls in practice? Wouldn't it be better to pass a null to Python and have it display as None?

[AmplabJenkins]

Merged build finished. All automated tests passed.

[AmplabJenkins]

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14500/

[kalpit]

@mateiz I ran into this when my custom RDD produced nulls for some elements within a partition/split (during compute()).

It would indeed be better to pass a null to Python and have it display it as None. One solution is to a pick a TOKEN that we write into the tmp file and then translate it to a "None" during read. This, however, is not failsafe because there is a remote possibility of string data being identical to the TOKEN. Perhaps we could address that by fencing regular data by a special character and treating data lacking that fence as tokens.

In any case, the above solution (or an alternative) would be a relatively larger change, and I preferred fixing at least the NPEs in PythonRDD for short term (stack trace is in JIRA ticket).

What do you think ?

[mateiz]

But that means that the NPEs are only happening with your custom RDD, right? They won't happen for regular Spark users.

I think we should pass None here. One way to do it is to select a negative length (e.g. -3) to represent null, and pass that to Python. We already use other negative lengths for other special flags.

[kalpit]

I suspect that the NPEs will happen for any PySpark User who has an RDD that returns null for some input "x" based on the lambda/transform. Check out the test case I added to "PythonRDDSuite.scala" to reproduce the NPE.

I considered the idea of using negative length (-4) to pass "None" to python (PythonRDD.SpecialLengths -1 to -3 are taken). The tricky part however is that the read() method returns an array of bytes based on the length. Existing code treats empty array as end of data/stream. So I am not sure how we would communicate "None" to python. Thoughts ?

[mateiz]

Lambdas in Python that return None will work fine because we use pickling for all data after that. The only way this problem can happen is if a Java RDD has null in it. Do you have an example in Python only (with the current PySpark) where this happens?

[kalpit]

I see your point. I don't have a Python-only use-case that can trigger the NPE.

My custom RDD implementation had a corner-case in which RDD's compute() method returned a "null" in the iterator stream. I have fixed my custom RDD implementation to not do that, so I don't run into this NPE anymore. However, should anyone else out there ever implement a custom RDD of similar nature (has nulls for some elements in a partition's iterator stream) and tries accessing such an RDD from PySpark, he/she would run into the NPE, so I thought it would be nicer if we handled nulls in the stream gracefully.

[mateiz]

Yeah, but in that case I think we have to figure out a way with the lengths. I haven't had time to look into it, but basically the UTF decoder in Python needs to deal with negative lengths sent from Scala.

[kanzhang]

I considered the idea of using negative length (-4) to pass "None" to python (PythonRDD.SpecialLengths -1 to -3 are taken). The tricky part however is that the read() method returns an array of bytes based on the length. Existing code treats empty array as end of data/stream. So I am not sure how we would communicate "None" to python. Thoughts ?

@kalpit pls take a look at #644, where I propose to use null to signal end of stream instead of an empty array.

[JoshRosen]

Hi @kalpit,

Since this PR has been superseded by #644, do you mind closing it? Thanks!

[AmplabJenkins]

Can one of the admins verify this patch?

[mateiz]

I've closed this since it was fixed separately. Thanks for sending a patch here.

[SparkQA]

Can one of the admins verify this patch?