Github user kiszk commented on a diff in the pull request: https://github.com/apache/spark/pull/22375#discussion_r219397495 --- 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 -- The your first is correct since this patch addresses only codegen-on case. We can add another code to address codegen-off case. Regarding the your second point, have we ever distingished a wrong output from a bad written UT when we defect the difference between `expression` and `expected`. I think that the distinguishment is nice to have, but not mandatory to have. I have one question about your approach: ``` assert(containsNull(expected) && isNullable(expression.dataType)) ``` Since the above two conditions evaluates `expected` and `expression` independently, how this works for the following case? I think that the assertion would be passed ``` expression: dataType = StructType(ArrayType(IntegerType, false), ArrayType(IntegerType, true)) Struct(Array(0, null), Array(1, 0)) expected: Struct(Array(0, 0), Array(1, null)) ```
--- --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org