Github user marmbrus commented on a diff in the pull request:
https://github.com/apache/spark/pull/7143#discussion_r33712653
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/PredicateSuite.scala
---
@@ -137,43 +132,47 @@ class PredicateSuite extends SparkFunSuite with
ExpressionEvalHelper {
checkEvaluation(And(InSet(one, hS), InSet(two, hS)), true)
}
+ private def binaryComparisonTest(
+ name: String,
+ op: (Expression, Expression) => Expression,
+ result: Seq[Boolean]): Unit = {
+ val smallValues = Seq(1, Decimal(1), Array(1.toByte),
"a").map(Literal(_))
+ val largeValues = Seq(2, Decimal(2), Array(2.toByte),
"b").map(Literal(_))
+ val equalValues = Seq(1, Decimal(1), Array(1.toByte),
"a").map(Literal(_))
+ test("BinaryComparison: " + name) {
+ for (i <- 0 until result.length) {
+ checkEvaluation(op(smallValues(i), largeValues(i)), result(0))
+ checkEvaluation(op(smallValues(i), equalValues(i)), result(1))
+ checkEvaluation(op(largeValues(i), smallValues(i)), result(2))
+ }
+ }
+ }
- test("BinaryComparison") {
- val row = create_row(1, 2, 3, null, 3, null)
- val c1 = 'a.int.at(0)
- val c2 = 'a.int.at(1)
- val c3 = 'a.int.at(2)
- val c4 = 'a.int.at(3)
- val c5 = 'a.int.at(4)
- val c6 = 'a.int.at(5)
+ binaryComparisonTest("<", LessThan, Seq(true, false, false))
+ binaryComparisonTest("<=", LessThanOrEqual, Seq(true, true, false))
+ binaryComparisonTest(">", GreaterThan, Seq(false, false, true))
+ binaryComparisonTest(">=", GreaterThanOrEqual, Seq(false, true, true))
+ binaryComparisonTest("===", EqualTo, Seq(false, true, false))
+ binaryComparisonTest("<=>", EqualNullSafe, Seq(false, true, false))
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
