[SPARK-35389][SQL] V2 ScalarFunction should support magic method with null arguments #32553

sunchao · 2021-05-14T17:49:09Z

What changes were proposed in this pull request?

When creating Invoke and StaticInvoke for ScalarFunction's magic method, set propagateNull to false.

Why are the changes needed?

When propgagateNull is true (which is the default value), Invoke and StaticInvoke will return null if any of the argument is null. For scalar function this is incorrect, as we should leave the logic to function implementation instead.

Does this PR introduce any user-facing change?

Yes. Now null arguments shall be properly handled with magic method.

How was this patch tested?

Added new tests.

SparkQA · 2021-05-14T19:28:40Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43085/

SparkQA · 2021-05-14T19:28:42Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43085/

dongjoon-hyun · 2021-05-14T21:04:27Z

cc @cloud-fan

SparkQA · 2021-05-14T22:26:17Z

Test build #138564 has finished for PR 32553 at commit 8c4dfb8.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

viirya

looks correct.

cloud-fan · 2021-05-17T14:54:05Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

how about primitive type parameters?

I think it shouldn't matter. Here we're making sure that the magic method will always be invoked regardless of null-ness of the arguments.

For primitive types this has less significant meaning because even if progagateNull is true, needNullCheck

protected lazy val needNullCheck: Boolean = propagateNull && arguments.exists(_.nullable)

in InvokeLike also considers null-ness of arguments.

How can users handle nullable int values if the UDF is something like isPositive(int i) which can't accept null argument?

Ah I see what you mean, thanks. Seems we should allow users to define magic method with boxed primitive types for this case? We could also follow the behavior of ScalaUDF and returns null if any of the primitive type parameter is nullable and the input is null, however currently InvokeLike cannot handle the case where a subset of the input types are of primitive nullable type.

SparkQA · 2021-05-17T23:06:30Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43169/

SparkQA · 2021-05-17T23:08:15Z

Test build #138649 has finished for PR 32553 at commit df6eec2.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-05-17T23:51:55Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43170/

SparkQA · 2021-05-17T23:52:26Z

Test build #138650 has finished for PR 32553 at commit 6a87a27.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-05-18T00:51:32Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43171/

SparkQA · 2021-05-18T00:53:35Z

Test build #138651 has finished for PR 32553 at commit 136867f.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-05-18T02:34:04Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43177/

cloud-fan · 2021-05-18T02:45:33Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala

  def propagateNull: Boolean

-  protected lazy val needNullCheck: Boolean = propagateNull && arguments.exists(_.nullable)
+  def propagateNullForPrimitive: Boolean


This reminds me of the rule HandleNullInputsForUDF.

For primitive inputs, I don't think we have a choice and we must propagate null (isPositive(int i) can't handle null values). Can we detect it automatically instead of adding a new boolean flag?

OK. Let me remove the flag then, and add an extra comment to propagateNull.

One issue with this approach is that it only applies to the magic method path, while for produceResult users will need to handle the null primitive values explicitly. I think we'll need to document this more carefully, otherwise it could cause confusion.

yea, I agree. If users want to handle null by themselves, they should use boxed primitive types as the UDF parameter type.

SparkQA · 2021-05-18T03:06:22Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43177/

SparkQA · 2021-05-18T05:21:14Z

Test build #138655 has finished for PR 32553 at commit 94fd2d0.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2021-05-18T06:22:50Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

          case Some(m) if Modifier.isStatic(m.getModifiers) =>
            StaticInvoke(scalarFunc.getClass, scalarFunc.resultType(),
-              MAGIC_METHOD_NAME, arguments, returnNullable = scalarFunc.isResultNullable)
+              MAGIC_METHOD_NAME, arguments, propagateNull = false,


So I think we need propagateNull as true, instead of false?

If propagateNull is true, we'd return null directly even if input arguments are of non-primitive type, which is not what we want.

Got it. Thanks.

viirya · 2021-05-18T06:29:03Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala

-  protected lazy val needNullCheck: Boolean = propagateNull && arguments.exists(_.nullable)
+  protected lazy val needNullCheck: Boolean = needNullCheckForIndex.contains(true)
+  protected lazy val needNullCheckForIndex: Array[Boolean] =
+    arguments.map(a => a.nullable && (propagateNull ||


For this change, I think the definition of propagateNull is somehow different now. Previously if propagateNull is true, null will be propagated, but now it also depends on if the argument is primitive type. It is better to update propagateNull param doc.

Oh, nvm. I saw you updated it below.

SparkQA · 2021-05-18T07:47:59Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43187/

SparkQA · 2021-05-18T08:27:13Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43187/

cloud-fan · 2021-05-18T08:45:53Z

thanks, merging to master!

SparkQA · 2021-05-18T11:06:21Z

Test build #138666 has finished for PR 32553 at commit fb0d959.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

sunchao · 2021-05-18T15:01:14Z

Thanks @cloud-fan and @viirya for the review!

github-actions bot added the SQL label May 14, 2021

viirya reviewed May 15, 2021

View reviewed changes

cloud-fan reviewed May 17, 2021

View reviewed changes

initial commit

44e7f8f

sunchao force-pushed the SPARK-35389 branch from 8c4dfb8 to df6eec2 Compare May 17, 2021 22:59

sunchao force-pushed the SPARK-35389 branch from df6eec2 to 6a87a27 Compare May 17, 2021 23:09

sunchao force-pushed the SPARK-35389 branch from 6a87a27 to 136867f Compare May 18, 2021 00:11

handle null primitive arguments

94fd2d0

sunchao force-pushed the SPARK-35389 branch from 136867f to 94fd2d0 Compare May 18, 2021 01:00

cloud-fan reviewed May 18, 2021

View reviewed changes

remove the boolean flag and more comments

fb0d959

viirya reviewed May 18, 2021

View reviewed changes

cloud-fan approved these changes May 18, 2021

View reviewed changes

cloud-fan closed this in 44d762a May 18, 2021

sunchao deleted the SPARK-35389 branch May 18, 2021 15:01

[SPARK-35389][SQL] V2 ScalarFunction should support magic method with null arguments #32553

[SPARK-35389][SQL] V2 ScalarFunction should support magic method with null arguments #32553

Uh oh!

Conversation

sunchao commented May 14, 2021

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

SparkQA commented May 14, 2021

Uh oh!

SparkQA commented May 14, 2021

Uh oh!

dongjoon-hyun commented May 14, 2021

Uh oh!

SparkQA commented May 14, 2021

Uh oh!

viirya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 17, 2021

Uh oh!

SparkQA commented May 17, 2021

Uh oh!

SparkQA commented May 17, 2021

Uh oh!

SparkQA commented May 17, 2021

Uh oh!

SparkQA commented May 18, 2021

Uh oh!

SparkQA commented May 18, 2021

Uh oh!

SparkQA commented May 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 18, 2021

Uh oh!

SparkQA commented May 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 18, 2021

Uh oh!

SparkQA commented May 18, 2021

Uh oh!

cloud-fan commented May 18, 2021

Uh oh!

SparkQA commented May 18, 2021

Uh oh!

sunchao commented May 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!