[SPARK-23912][SQL]add array_distinct #21050

huaxingao · 2018-04-12T04:57:37Z

What changes were proposed in this pull request?

Add array_distinct to remove duplicate value from the array.

How was this patch tested?

Add unit tests

HyukjinKwon · 2018-04-12T05:55:29Z

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

indentation ..

HyukjinKwon · 2018-04-12T05:56:15Z

...st/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollectionExpressionsSuite.scala

Seq.empty[Integer]

@HyukjinKwon Thanks for your comments. Will fix the problems. Sorry for the late reply.

SparkQA · 2018-04-12T07:05:01Z

Test build #89236 has finished for PR 21050 at commit 3bfb23e.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ArrayDistinct(child: Expression)

kiszk · 2018-04-12T15:19:03Z

retest this please

SparkQA · 2018-04-12T18:49:07Z

Test build #89276 has finished for PR 21050 at commit 3bfb23e.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ArrayDistinct(child: Expression)

SparkQA · 2018-04-24T20:13:59Z

Test build #89793 has finished for PR 21050 at commit e3f7ee6.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ArrayDistinct(child: Expression)

SparkQA · 2018-04-25T05:21:54Z

Test build #89813 has finished for PR 21050 at commit 91c3076.

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

SparkQA · 2018-04-26T01:27:56Z

Test build #89862 has finished for PR 21050 at commit f55abae.

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

mn-mikke · 2018-04-26T17:14:14Z

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

Shouldn't you check $array.isNullAt($j) in this loop as well? Especially, when elementType is primitive?

mn-mikke · 2018-04-26T17:30:56Z

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

Just wondering about cases with big arrays with lots of duplicated values. In such cases, $tempArray is unnecesserily big. What about performing the filtering in two runs? The first run would calculate the result array size and the second would copy items from the source to the result?.

@mn-mikke Thanks for your comments. I will have two runs as you suggested: one to get the distinct array size and another to copy the value. Will also add the $array.isNullAt($j)

gatorsmile · 2018-05-01T23:10:44Z

cc @ueshin

SparkQA · 2018-05-02T00:51:26Z

Test build #90003 has finished for PR 21050 at commit e57056b.

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

huaxingao · 2018-05-02T04:59:22Z

retest this please

SparkQA · 2018-05-02T07:05:01Z

Test build #90028 has finished for PR 21050 at commit e57056b.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

ueshin · 2018-05-03T08:48:01Z

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

I guess nested loop must be very slow. Can you use a hash table or something to check duplication? Maybe OpenHashSet?

@ueshin Thanks for your comment. I will change the code to use OpenHashSet.

ueshin · 2018-05-03T09:23:40Z

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

Do we need to keep the element order? If so, does distinct keep the order?

Yes, distinct keeps the order.

SparkQA · 2018-05-18T02:41:05Z

Test build #90766 has finished for PR 21050 at commit 647528f.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

kiszk · 2018-05-18T03:04:12Z

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

IIUC, do we need to execute this then-block only for the first null element in $array?
In other words, if we introduce a flag like foundNullElement, we can avoid to execute a loop for the rest of null elements.

@kiszk Thanks for your comment. I have changed the code.

kiszk · 2018-05-18T03:05:36Z

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

kiszk · 2018-05-18T03:05:50Z

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

kiszk · 2018-05-18T03:08:56Z

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

Does it work for boolean, float, or double?

Yes, It works for boolean, float and double. Here are the generated code:

org.apache.spark.util.collection.OpenHashSet project_hs_1 = new org.apache.spark.util.collection.OpenHashSet(scala.reflect.ClassTag$.MODULE$.Boolean());

org.apache.spark.util.collection.OpenHashSet project_hs_1 = new org.apache.spark.util.collection.OpenHashSet(scala.reflect.ClassTag$.MODULE$.Double());

org.apache.spark.util.collection.OpenHashSet project_hs_1 = new org.apache.spark.util.collection.OpenHashSet(scala.reflect.ClassTag$.MODULE$.Float());

I also added UT for boolean, float and double.

Sorry, it should work. But, it is not specialized for boolean, float, or double. nvm.
Thanks.

SparkQA · 2018-05-18T05:50:16Z

Test build #90765 has finished for PR 21050 at commit 9a42723.

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

kiszk · 2018-05-18T19:48:37Z

...st/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollectionExpressionsSuite.scala

Could you please add test cases with complex types (e.g. Array[Binary] or others)? See #21361.
cc @ueshin

Will do. Thanks!

SparkQA · 2018-05-18T22:19:26Z

Test build #90801 has finished for PR 21050 at commit 8ae2db0.

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

ueshin

I'm sorry for the super delay.
After I reviewed this for the first time, we found we can't use hash for some data types.
Could you see #21028 and follow the implementation there? In short, we can't support MapType element, and we need to use nested loop for BinaryType or other non-primitive types as you did before.
Thanks!

ueshin · 2018-06-07T18:10:06Z

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

Maybe we can skip some checks here for just counting the distinct array length, such as if (!($foundNullElement)) or if (!($hs.contains($getValue))).

We can simply do $foundNullElement = true if null found, otherwise $hs.add($getValue), and the length will be $hs.size() + ($foundNullElement ? 1 : 0).

@ueshin Thanks for your review. I made some changes based on your comments. Could you please review one more time? Thanks!

SparkQA · 2018-06-12T22:37:24Z

Test build #91726 has finished for PR 21050 at commit 298954f.

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

SparkQA · 2018-06-13T00:58:04Z

Test build #91732 has finished for PR 21050 at commit ba0d60f.

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

ueshin · 2018-06-15T20:31:03Z

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

+           |$openHashSet $hs = new $openHashSet($classTag);
+           |for (int $i = 0; $i < $array.numElements(); $i++) {
+           |  if ($array.isNullAt($i)) {
+           |     if (!($foundNullElement)) {


We don't need to check this and can do simply $foundNullElement = true;?

ueshin · 2018-06-15T20:31:22Z

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

+           |     }
+           |  }
+           |  else {
+           |    if (!($hs.contains($getValue1))) {


ueshin · 2018-06-15T20:35:54Z

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

+    """.stripMargin
+  }
+
+  private def setValueForbruteForceEval(


nit: setValueForBruteForceEval?

ueshin · 2018-06-15T20:38:19Z

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

+    s"""
+       |int $j;
+       |for ($j = 0; $j < $i; $j ++) {
+       |  if (!$inputArray.isNullAt($j) && $isEqual)


Seems like { is missing?

ueshin · 2018-06-15T20:38:30Z

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

+       |    $setValue;
+       |    $pos = $pos + 1;
+       |  }
+    """.stripMargin


Seems like } is missing?

ueshin · 2018-06-15T20:55:39Z

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

+        s"""
+           |$distinctArray[$pos] = null;
+        """.
+          stripMargin


It's okay to use s"$distinctArray[$pos] = null" for 1-line code.

ueshin · 2018-06-15T20:55:45Z

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

+        s"""
+           |$distinctArray.setNullAt($pos);
+        """.
+          stripMargin


ueshin · 2018-06-15T20:56:23Z

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

+    if (!isPrimitive) {
+      s"""
+         |$distinctArray[$pos] = $getValue1;
+      """.stripMargin


ueshin · 2018-06-15T20:56:29Z

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

+    } else {
+      s"""
+         |$distinctArray.set$primitiveValueTypeName($pos, $getValue1);
+      """.stripMargin


ueshin · 2018-06-15T21:03:40Z

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

+           |${ev.value} = $distinctArray;
+        """.stripMargin
+      } else {
+        val setValueForbruteForce = setValueForbruteForceEval(true, i, j,


I don't think we need this clause because primitive types always support equals.

@ueshin Thanks fo your comments. I have made changes. Could you please take a look again? Thanks!

SparkQA · 2018-06-17T11:08:16Z

Test build #91992 has finished for PR 21050 at commit 2df2cff.

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

SparkQA · 2018-06-17T11:31:35Z

Test build #91993 has finished for PR 21050 at commit 2d77880.

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

ueshin

LGTM except for some nits.

ueshin · 2018-06-20T05:45:55Z

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

+        """.stripMargin
+      } else {
+        val setValueForBruteForce = setValueForBruteForceEval(false, i, j,
+            inputArray, distinctArray, pos, getValue1, isEqual, "")


nit: indent

ueshin · 2018-06-20T05:46:05Z

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

+      val setNullForPrimitive = setNull(true, foundNullElement, distinctArray, pos)
+      val classTag = s"scala.reflect.ClassTag$$.MODULE$$.$primitiveValueTypeName()"
+      val setValueForFast =
+          setValueForFastEval(true, hs, distinctArray, pos, getValue1, primitiveValueTypeName)


nit: indent

ueshin · 2018-06-20T05:49:07Z

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

+            }
+            j = j + 1
+          }
+          if (i == j-1) {


nit: (i == j - 1)?

ueshin · 2018-06-20T05:50:42Z

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

+    } else {
+      var foundNullElement = false
+      var pos = 0
+      for(i <- 0 until data.length) {


nit: for (?

ueshin · 2018-06-20T05:50:55Z

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

+      val openHashSet = classOf[OpenHashSet[_]].getName
+      val hs = ctx.freshName("hs")
+      val classTag = s"scala.reflect.ClassTag$$.MODULE$$.Object()"
+      if(elementTypeSupportEquals) {


nit: if (?

kiszk · 2018-06-20T07:34:26Z

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

+        |$openHashSet $hs = new $openHashSet($classTag);
+        |for (int $i = 0; $i < $inputArray.numElements(); $i ++) {
+        |  if ($inputArray.isNullAt($i)) {
+        |     $setNullForPrimitive;


nit: indent

huaxingao · 2018-06-20T23:05:06Z

@ueshin @kiszk Thanks for your comments. I fixed the problems.
I am not sure if I should use $i++ or $i ++ in the for loop. It seems other people use $i ++, so I also used $i ++

SparkQA · 2018-06-21T02:56:18Z

Test build #92148 has finished for PR 21050 at commit 3f5d03b.

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

ueshin · 2018-06-21T03:23:59Z

Thanks! merging to master.

huaxingao · 2018-06-21T09:26:33Z

Thank you very much for your help!

HyukjinKwon reviewed Apr 12, 2018

View reviewed changes

huaxingao force-pushed the spark-23912 branch from 3bfb23e to e3f7ee6 Compare April 24, 2018 16:27

huaxingao force-pushed the spark-23912 branch from 91c3076 to f55abae Compare April 25, 2018 21:42

mn-mikke reviewed Apr 26, 2018

View reviewed changes

ueshin reviewed May 3, 2018

View reviewed changes

huaxingao force-pushed the spark-23912 branch from e57056b to 9a42723 Compare May 18, 2018 02:14

kiszk reviewed May 18, 2018

View reviewed changes

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala Outdated

Copy link

Member

kiszk May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

kiszk reviewed May 18, 2018

View reviewed changes

...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala Outdated

Copy link

Member

kiszk May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

kiszk reviewed May 18, 2018

View reviewed changes

ueshin reviewed Jun 7, 2018

View reviewed changes

huaxingao force-pushed the spark-23912 branch from 8ae2db0 to 298954f Compare June 12, 2018 18:43

huaxingao added 4 commits June 12, 2018 13:21

[SPARK-23912][SQL]add array_distinct

4890fac

resolve conflicts

1cefd72

address comments(2)

3ce3ddc

use OpenHashSet to check duplication in the array

1219626

huaxingao added 2 commits June 12, 2018 13:21

address comments(3)

d127ac4

add complex data type support

ba0d60f

huaxingao force-pushed the spark-23912 branch from 298954f to ba0d60f Compare June 12, 2018 21:24

ueshin reviewed Jun 15, 2018

View reviewed changes

huaxingao added 2 commits June 17, 2018 09:20

address comments

2df2cff

change b to B in setValueForbruteForce

2d77880

ueshin reviewed Jun 20, 2018

View reviewed changes

ueshin mentioned this pull request Jun 20, 2018

[SPARK-23914][SQL] Add array_union function #21061

Closed

kiszk reviewed Jun 20, 2018

View reviewed changes

fix comments

3f5d03b

asfgit closed this in 9de11d3 Jun 21, 2018

[SPARK-23912][SQL]add array_distinct #21050

[SPARK-23912][SQL]add array_distinct #21050

Uh oh!

Conversation

huaxingao commented Apr 12, 2018

What changes were proposed in this pull request?

How was this patch tested?

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 Apr 12, 2018

Uh oh!

kiszk commented Apr 12, 2018

Uh oh!

SparkQA commented Apr 12, 2018

Uh oh!

SparkQA commented Apr 24, 2018

Uh oh!

SparkQA commented Apr 25, 2018

Uh oh!

SparkQA commented Apr 26, 2018

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!

gatorsmile commented May 1, 2018

Uh oh!

SparkQA commented May 2, 2018

Uh oh!

huaxingao commented May 2, 2018

Uh oh!

SparkQA commented May 2, 2018

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, 2018

Uh oh!

kiszk May 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 18, 2018

Uh oh!

kiszk May 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 18, 2018

Uh oh!

ueshin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

kiszk May 18, 2018 •

edited

Loading

kiszk May 18, 2018 •

edited

Loading