ganeshashree commented on code in PR #58407:
URL: https://github.com/apache/spark/pull/58407#discussion_r3894384221
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/higherOrderFunctions.scala:
##########
@@ -384,10 +384,14 @@ trait SimpleHigherOrderFunction extends
HigherOrderFunction with BinaryLike[Expr
|$nullSafeEval
""")
} else {
+ // A declared non-null argument can still be null at runtime, so
null-check the reference
+ // here too, matching `eval` above.
ev.copy(code = code"""
|${argumentGen.code}
|${CodeGenerator.javaType(dataType)} ${ev.value} =
${CodeGenerator.defaultValue(dataType)};
- |$resultCode
+ |if (${argumentGen.value} != null) {
Review Comment:
I think this is fine for transform/filter, but it looks off for
exists/forall since they return a boolean and go through the same path. This
branch only runs when the argument is declared `non-null`, and if that argument
turns out to be null at runtime, `ev.value` stays `false` with `isNull =
FalseLiteral`, so exists/forall return false, while the interpreted eval
returns null. The reference-typed results get away with it because the
nested-null guard downstream catches them, but there's nothing catching the
primitive case.
It's still an improvement over the NPE we had before, so not a blocker, but
to really match eval the result needs to be nullable here. The new agreement
test only exercises `ArrayTransform`, so it slips through. Might be worth
adding an exists/forall case to cover it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
