[SPARK-36792][SQL] InSet should handle NaN #34033

AngersZhuuuu · 2021-09-17T16:30:38Z

What changes were proposed in this pull request?

InSet should handle NaN

InSet(Literal(Double.NaN), Set(Double.NaN, 1d)) should return true, but return false.

Why are the changes needed?

InSet should handle NaN

Does this PR introduce any user-facing change?

No

How was this patch tested?

Added UT

SparkQA · 2021-09-17T17:20:12Z

Test build #143421 has finished for PR 34033 at commit a74a310.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-09-17T17:44:43Z

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

SparkQA · 2021-09-17T17:53:11Z

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

SparkQA · 2021-09-18T03:50:44Z

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

SparkQA · 2021-09-18T03:58:09Z

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

SparkQA · 2021-09-18T04:14:36Z

Test build #143431 has finished for PR 34033 at commit c3addf6.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-09-19T03:42:03Z

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

SparkQA · 2021-09-19T03:51:01Z

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

SparkQA · 2021-09-19T04:40:07Z

Test build #143440 has finished for PR 34033 at commit 141dc5f.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-09-23T03:15:01Z

Test build #143527 has finished for PR 34033 at commit 87df7b0.

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

AngersZhuuuu · 2021-09-23T03:15:36Z

ping @cloud-fan

cloud-fan · 2021-09-23T03:24:50Z

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

    if (set.contains(value)) {
      true
+    } else if (isNaN(value)) {
+      set.exists(isNaN(_))


can we have a hasNaN variable to avoid repeated computing?

How about current?

SparkQA · 2021-09-23T03:53:27Z

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

SparkQA · 2021-09-23T04:00:38Z

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

SparkQA · 2021-09-23T04:39:28Z

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

SparkQA · 2021-09-23T04:45:53Z

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

SparkQA · 2021-09-23T05:16:22Z

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

SparkQA · 2021-09-23T05:18:20Z

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

SparkQA · 2021-09-23T06:02:57Z

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

SparkQA · 2021-09-23T06:20:46Z

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

cloud-fan · 2021-09-23T06:29:01Z

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

+    case FloatType => (value: Any) => java.lang.Float.isNaN(value.asInstanceOf[java.lang.Float])
+    case _ => (_: Any) => false
+  }
+  @transient private[this] lazy val hasNaN = set.exists(isNaN)


nit: we can avoid iterating the set if type is not float/double.

nit: we can avoid iterating the set if type is not float/double.

DOne

SparkQA · 2021-09-23T08:06:41Z

Test build #143528 has finished for PR 34033 at commit 174ac71.

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

SparkQA · 2021-09-23T08:09:36Z

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

SparkQA · 2021-09-23T09:15:42Z

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

SparkQA · 2021-09-23T09:57:20Z

Test build #143533 has finished for PR 34033 at commit 293daea.

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

SparkQA · 2021-09-23T12:02:05Z

Test build #143541 has finished for PR 34033 at commit 4878d5c.

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

cloud-fan · 2021-09-23T15:26:28Z

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

  private def genCodeWithSet(ctx: CodegenContext, ev: ExprCode): ExprCode = {
    nullSafeCodeGen(ctx, ev, c => {
      val setTerm = ctx.addReferenceObj("set", set)
+      val hasNaNValue = ctx.addReferenceObj("hasNaN", hasNaN)


hasNaN is a boolean, we can just use it to generate code

|} else if (${isNaN(c)}) { | ${ev.value} = $hasNaN; |}

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

SparkQA · 2021-09-23T16:52:33Z

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

SparkQA · 2021-09-23T17:36:47Z

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

SparkQA · 2021-09-23T20:58:11Z

Test build #143554 has finished for PR 34033 at commit d6d517c.

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

SparkQA · 2021-09-24T04:03:27Z

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

SparkQA · 2021-09-24T04:51:14Z

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

SparkQA · 2021-09-24T08:08:01Z

Test build #143583 has finished for PR 34033 at commit ef0e81f.

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

cloud-fan · 2021-09-24T08:19:32Z

thanks, merging to master/3.2/3.1/3.0!

### What changes were proposed in this pull request? InSet should handle NaN ``` InSet(Literal(Double.NaN), Set(Double.NaN, 1d)) should return true, but return false. ``` ### Why are the changes needed? InSet should handle NaN ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Added UT Closes #34033 from AngersZhuuuu/SPARK-36792. Authored-by: Angerszhuuuu <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 64f4bf4) Signed-off-by: Wenchen Fan <[email protected]>

### What changes were proposed in this pull request? InSet should handle NaN ``` InSet(Literal(Double.NaN), Set(Double.NaN, 1d)) should return true, but return false. ``` ### Why are the changes needed? InSet should handle NaN ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Added UT Closes apache#34033 from AngersZhuuuu/SPARK-36792. Authored-by: Angerszhuuuu <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 64f4bf4) Signed-off-by: Wenchen Fan <[email protected]>

[SPARK-36792][SQL] InSet should handle NaN

a74a310

github-actions bot added the SQL label Sep 17, 2021

AngersZhuuuu marked this pull request as draft September 17, 2021 16:30

Update predicates.scala

320b5ea

AngersZhuuuu added 2 commits September 18, 2021 11:06

Update predicates.scala

1925e81

Update PredicateSuite.scala

c3addf6

AngersZhuuuu marked this pull request as ready for review September 18, 2021 03:07

Update predicates.scala

141dc5f

AngersZhuuuu added 3 commits September 22, 2021 14:33

Merge branch 'master' into SPARK-36792

4ad6439

Update predicates.scala

87df7b0

Update predicates.scala

174ac71

cloud-fan reviewed Sep 23, 2021

View reviewed changes

AngersZhuuuu added 2 commits September 23, 2021 12:15

Update predicates.scala

a48e77c

Update predicates.scala

293daea

cloud-fan reviewed Sep 23, 2021

View reviewed changes

Update predicates.scala

4878d5c

cloud-fan reviewed Sep 23, 2021

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala Show resolved Hide resolved

Update predicates.scala

d6d517c

trigger

ef0e81f

cloud-fan closed this in 64f4bf4 Sep 24, 2021

[SPARK-36792][SQL] InSet should handle NaN #34033

[SPARK-36792][SQL] InSet should handle NaN #34033

Uh oh!

Conversation

AngersZhuuuu commented Sep 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 Sep 17, 2021

Uh oh!

SparkQA commented Sep 17, 2021

Uh oh!

SparkQA commented Sep 17, 2021

Uh oh!

SparkQA commented Sep 18, 2021

Uh oh!

SparkQA commented Sep 18, 2021

Uh oh!

SparkQA commented Sep 18, 2021

Uh oh!

SparkQA commented Sep 19, 2021

Uh oh!

SparkQA commented Sep 19, 2021

Uh oh!

SparkQA commented Sep 19, 2021

Uh oh!

SparkQA commented Sep 23, 2021

Uh oh!

AngersZhuuuu commented Sep 23, 2021

Uh oh!

cloud-fan Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

AngersZhuuuu Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 23, 2021

Uh oh!

SparkQA commented Sep 23, 2021

Uh oh!

SparkQA commented Sep 23, 2021

Uh oh!

SparkQA commented Sep 23, 2021

Uh oh!

SparkQA commented Sep 23, 2021

Uh oh!

SparkQA commented Sep 23, 2021

Uh oh!

SparkQA commented Sep 23, 2021

Uh oh!

SparkQA commented Sep 23, 2021

Uh oh!

cloud-fan Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

AngersZhuuuu Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 23, 2021

Uh oh!

SparkQA commented Sep 23, 2021

Uh oh!

SparkQA commented Sep 23, 2021

Uh oh!

SparkQA commented Sep 23, 2021

Uh oh!

SparkQA commented Sep 23, 2021

Uh oh!

cloud-fan Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

AngersZhuuuu Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SparkQA commented Sep 23, 2021

Uh oh!

AngersZhuuuu commented Sep 17, 2021 •

edited

Loading

cloud-fan commented Sep 24, 2021 •

edited

Loading