Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/22375#discussion_r218094296
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelperSuite.scala
---
@@ -35,6 +36,13 @@ class ExpressionEvalHelperSuite extends SparkFunSuite
with ExpressionEvalHelper
val e = intercept[RuntimeException] {
checkEvaluation(BadCodegenExpression(), 10) }
assert(e.getMessage.contains("some_variable"))
}
+
+ test("SPARK-25388: checkEvaluation should fail if nullable in DataType
is incorrect") {
+ val e = intercept[RuntimeException] {
+ checkEvaluation(MapIncorrectDataTypeExpression(), Map(3 -> 7, 6 ->
null))
--- End diff --
yes, thanks for the summary, it states more clearly what I thought.
My point is that this fix works properly only when we test both codegen on
and off, but it would fail to detect the error condition it claims to fix if
only one of them (for any reason) is tested. So I am wondering if it is
possible to perform a check on the expected value, instead of this fix.
Something like:
```
assert(containsNull(expected) && isNullable(expression.dataType))
```
where `containsNull` and `isNullable` have to be defined properly. In this
way we should fail properly independently from whether codegen is on or not.
And we can also give a more clear hint in the error message about the problem
being most likely a bad UT.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
