[GitHub] spark pull request #22259: [SPARK-25044][SQL] (take 2) Address translation o...

[maryannxue <gi...@git.apache.org>]

Github user maryannxue commented on a diff in the pull request:

    https://github.com/apache/spark/pull/22259#discussion_r224252642
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ScalaUDF.scala ---
    @@ -47,7 +48,8 @@ case class ScalaUDF(
         inputTypes: Seq[DataType] = Nil,
         udfName: Option[String] = None,
         nullable: Boolean = true,
    -    udfDeterministic: Boolean = true)
    +    udfDeterministic: Boolean = true,
    +    nullableTypes: Seq[Boolean] = Nil)
    --- End diff --
    
    I think the problem is more about the way we handle `nullableTypes` if not specified as in https://github.com/apache/spark/pull/22259/files#diff-57b3d87be744b7d79a9beacf8e5e5eb2R2157. The test failure of https://github.com/apache/spark/pull/21851/files#diff-e8dddba2915a147970654aa93bee30a7R344 would have been exposed if the `nullableTypes` had been updated in this PR. So I would say logically this parameter is required, but right now it is declared optional. In this case, things went wrong when `nullableTypes` was left unspecified, and this could happen not only with tests but in "source" too. I suggest we move this parameter up right after `inputTypes` so it can get the attention it needs.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org