[SPARK-8636][SQL] Fix equalNullSafe comparison

[vinodkc]

No description provided.

[animeshbaranawal]

Just changing the loop structure is not sufficient because changing the loop structure causes the case when tests in ConditionalExpressionSuite to fail.

[vinodkc]

Ok, I'll update that test suite.

@smola , In predicates.scala, eval method of case class EqualNullSafe has similar if (l == null && r == null) check , shall I modify that condition too?

[animeshbaranawal]

Sorry for opening another PR @vinodkc
Even changing the testsuite alone would not work. Some changes to the gencode function are also required.

[dragos]

This is far enough from Java/Scala semantics that it warrants a scaladoc and a pointer to NULL semantics in SQL, IMO

[marmbrus]

ok to test

[marmbrus]

We should at least consider renaming this function. Null safe equals in sql typically returns true for null <=> null

http://dev.mysql.com/doc/refman/5.0/en/comparison-operators.html#operator_equal-to

[vinodkc]

@marmbrus
Shall I rename the function private def equalNullSafe(l: Any, r: Any)
to private def equal(l: Any, r: Any) ?

[marmbrus]

or threeValueEquals ?

[SparkQA]

Test build #36862 has finished for PR 7040 at commit aac9f67.

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

[cloud-fan]

According to the JIRA, if key is null, we can just return the else part(or null if there is no else), right?
I think we can simplify the evaluation logic like

if (key == null) {
  return else part or null
} else {
  for(iterate when parts) {
    // as key is not null here, we can just use ==, and remove that nullSafeEqual
    if (key == when) {
      return then part
    }
  }
}

[SparkQA]

Test build #37120 has finished for PR 7040 at commit be5e641.

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

[marmbrus]

I'm going to merge this to fix the bug. Thanks!

Feel free to do a follow-up to further simplify / optimize.

[AmplabJenkins]

Can one of the admins verify this patch?

[marmbrus]

Can you close this please, now that it has been merged?