[SPARK-8747][SQL] fix EqualNullSafe for binary type

[cloud-fan]

also improve tests for binary comparison.

[AmplabJenkins]

Merged build triggered.

[AmplabJenkins]

Merged build started.

[SparkQA]

Test build #36227 has started for PR 7143 at commit d19e9c0.

[SparkQA]

Test build #36227 has finished for PR 7143 at commit d19e9c0.

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

[AmplabJenkins]

Merged build finished. Test PASSed.

[cloud-fan]

There is another place that need to handle binary type, I will fix it in another PR.
But I'm wondering whether we should watch out every place that need column equals, or create a wrapper class for binary type?
cc @marmbrus @davies

[davies]

The hard part is that BinaryType could be used in ArrayType and MapType, we need to also fix them.

As @marmbrus suggested, it's better to create a wrapper for BinaryType internal, let it handle hashCode and equality check. We can call it Binary, it's a large change.

[marmbrus]

This test is not very easy to read (the original was also pretty confusing IMHO). When writing tests, it is great if I can tell that they are correct by looking at as few lines of code as possible. This means two things:

1. (which you are already fixing) Its better to avoid indirection unless we are actually testing that part of the code. For example, don't create a row and a bound reference and then an expression that uses the bound reference). Instead just create an expression that compares literals.
2. Avoid having the expression and the answer far away from each other (even if it means slightly more typing):

This is very clearly correct, and I don't have to look all over the file the validate it:
checkEvaluation(Literal(1) > Literal(2), false)

In contrast, in order to understand if Seq(false, true, false) is correct I have to trace up to the function and manually line up and understand all of the code in lines 139-146.

[marmbrus]

Good catch on this bug. I do agree that we probably need to create an internal Binary type at some point.

[AmplabJenkins]

Merged build triggered.

[AmplabJenkins]

Merged build started.

[SparkQA]

Test build #36364 has started for PR 7143 at commit 28a5b76.

[SparkQA]

Test build #36364 has finished for PR 7143 at commit 28a5b76.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • trait ExpectsInputTypes
  • trait AutoCastInputTypes
  • abstract class BinaryExpression extends Expression with trees.BinaryNode[Expression]
  • abstract class BinaryOperator extends BinaryExpression
  • abstract class BinaryArithmetic extends BinaryOperator
  • case class UnHex(child: Expression) extends UnaryExpression with Serializable
  • abstract class BinaryComparison extends BinaryOperator with Predicate

[AmplabJenkins]

Merged build finished. Test PASSed.

[davies]

LGTM

[marmbrus]

This is much clearer :)