Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20637#discussion_r212817682
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala
---
@@ -223,8 +223,9 @@ trait ExpressionEvalHelper extends
GeneratorDrivenPropertyChecks with PlanTestBa
}
} else {
val lit = InternalRow(expected, expected)
+ val dtAsNullable = expression.dataType.asNullable
--- End diff --
Sorry, I think I have not explained properly my idea/suggestion and I think
there is somthing wrong in what you are saying. Without #22126 and with this PR
we have two scenarios:
- Using this `asNullable`: the test fails with incorrect evaluation.
- Without this `asNullable` the test fails with NullPointerException (it
is not passing unexpectedly).
So, IMHO, the right behavior is the second one. For 2 reasons: this is what
a user would get in such a scenario; in the first case what you return is a
wrong result, which may also be equal to the right one in very weird conditions
(with the risk of non-detecting error situation).
So I am strongly against introducing this `asNullable` here. I think we
just need to leave it as before.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
