SPARK-8336 Fix NullPointerException with functions.rand()

[tedyu]

This PR fixes the problem reported by Justin Yip in the thread 'NullPointerException with functions.rand()'

Tested using spark-shell and verified that the following works:
sqlContext.createDataFrame(Seq((1,2), (3, 100))).withColumn("index", rand(30)).show()

[JoshRosen]

Can you file a JIRA for this?

[JoshRosen]

Jenkins, retest this please.

[squito]

can you please add a test case to prevent regression

[tedyu]

Mind telling me which suite the new test should be added to ?

Thanks

[tedyu]

At first glance, none of the test suites under sql/catalyst/src/test//scala/org/apache/spark/sql seems proper for the new test.

[rxin]

We should create a RandomSuite.scala in expressions, and add tests for that. Take a look at other suites in that package.

[tedyu]

I looked at UnsafeFixedWidthAggregationMapSuite.scala in expressions package.

Is RandomSuite.scala going to test Rand and Randn only ?

A bit more hint is appreciated.

[rxin]

Ok you managed to pick one suite that isn't a good example. Take a look at https://github.com/apache/spark/blob/master/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ConditionalExpressionSuite.scala

basically use checkEvaluation function.

[tedyu]

I am trying to figure out how checkEvaluation should be used for the new test.

protected def checkEvaluation(
expression: Expression, expected: Any, inputRow: Row = EmptyRow): Unit = {

w.r.t. Rand(), the expected value is not deterministic.

[tedyu]

Looking at ArithmeticExpressionSuite.scala, it has some checks in the following form:
checkDoubleEvaluation(c1 - c2, (-0.9 +- 0.001), row)

This seems to be better fit for checking the return value from Rand()

[JoshRosen]

Don't we have some way of setting the RNG seed for testing?

[SparkQA]

Test build #34839 has finished for PR 6793 at commit 750f92c.

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

[rxin]

can we create a new test case, instead of adding it to the existing one?

I've been meaning to take the existing one apart for a while.

[rxin]

Also we should have a case where we explicitly set taskcontext

[tedyu]

Looking at the tests under sql, I don't see how TaskContext is explicitly set.

Creating a new test is fine. The new test would contain a method containing one line.
Just want to make sure this is fine.

[tedyu]

I am in Beijing now.
Except for difficulty of accessing gmail, github is quite slow as well.

[SparkQA]

Test build #34961 has finished for PR 6793 at commit 62fd97b.

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

[rxin]

Thanks. Merging in master & branch-1.4.

[punya]

@rxin it looks like the branch-1.4 cherry-pick of this commit broke a unit test, because it relies on ExpressionEvalHelper (which is absent on 1.4 afaik). I found this out because I was trying to backport an unrelated docs fix to branch-1.4. Any idea why the automated tests didn't catch this?

[punya]

Here's the relevant bit of the log (from testing #6842):

[error] /home/jenkins/workspace/SparkPullRequestBuilder/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/RandomSuite.scala:27: not found: type ExpressionEvalHelper
[error] class RandomSuite extends SparkFunSuite with ExpressionEvalHelper {
[error]                                              ^
[error] /home/jenkins/workspace/SparkPullRequestBuilder/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/RandomSuite.scala:30: not found: value create_row
[error]     val row = create_row(1.1, 2.0, 3.1, null)
[error]               ^
[error] /home/jenkins/workspace/SparkPullRequestBuilder/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/RandomSuite.scala:31: not found: value checkDoubleEvaluation
[error]     checkDoubleEvaluation(Rand(30), (0.7363714192755834 +- 0.001), row)
[error]     ^

[rxin]

Jenkins only run against master. Do you mind submitting a fix against branch-1.4 for this? I will merge it.