dilipbiswal commented on a change in pull request #24073: [SPARK-27134][SQL] 
array_distinct function does not work correctly with columns containing array 
of array
URL: https://github.com/apache/spark/pull/24073#discussion_r265220430
 
 

 ##########
 File path: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala
 ##########
 @@ -3112,29 +3112,30 @@ case class ArrayDistinct(child: Expression)
     (data: Array[AnyRef]) => new 
GenericArrayData(data.distinct.asInstanceOf[Array[Any]])
   } else {
     (data: Array[AnyRef]) => {
-      var foundNullElement = false
-      var pos = 0
+      val arrayBuffer = new scala.collection.mutable.ArrayBuffer[Any]
+      var alreadyStoredNull = false
+      var found = false
       for (i <- 0 until data.length) {
-        if (data(i) == null) {
-          if (!foundNullElement) {
-            foundNullElement = true
-            pos = pos + 1
+        if (data(i) != null) {
+          found = false
+          var j = 0;
+          while (!found && j < arrayBuffer.size) {
+            val va = arrayBuffer(j)
+            found = (va != null) && ordering.equiv(va, data(i))
+            j += 1
           }
-        } else {
-          var j = 0
-          var done = false
-          while (j <= i && !done) {
-            if (data(j) != null && ordering.equiv(data(j), data(i))) {
-              done = true
-            }
-            j = j + 1
+          if (!found) {
+            arrayBuffer += data(i)
           }
-          if (i == j - 1) {
-            pos = pos + 1
+        } else {
+          // De-duplicate the null values.
+          if (!alreadyStoredNull) {
+            arrayBuffer += data(i)
+            alreadyStoredNull = true
           }
         }
       }
-      new GenericArrayData(data.slice(0, pos))
 
 Review comment:
   @viirya 
   >Looks like original implementation assumes the duplicate items are placed 
at the end of data?
   
   Probably. The thing is, it does not rearrange the data in any way. So i 
don't know how we can just return the slice of the original array.
   > I think it not only affects array of array, but also other element type 
like BinaryType
   
   Yeah.. I will add your test case or enhance the Array[Binary] test case.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
[email protected]


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to