[SPARK-23921][SQL] Add array_sort function #21021

kiszk · 2018-04-10T06:26:10Z

What changes were proposed in this pull request?

The PR adds the SQL function array_sort. The behavior of the function is based on Presto's one.

The function sorts the input array in ascending order. The elements of the input array must be orderable. Null elements will be placed at the end of the returned array.

How was this patch tested?

Added UTs

viirya · 2018-04-10T06:34:16Z

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

Add since?

Do we have to add since in this file? I cannot find since in collectionOperations.scala. Of course, functions.scala and functions.py have since.

Every expression needs it .. many of functions don't have it because that field was added from 2.3.0. You could do it:

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

Line 511 in 2ca9bb0

since = "2.2.0")

which will be shown in SQL documentation and the extended description command in sql syntax

Thanks for letting know the example.

HyukjinKwon · 2018-04-10T06:35:45Z

Seems JIRA number wrong ..

viirya · 2018-04-10T06:36:37Z

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

Can we let SortArray and ArraySort both implement this and don't have a default implementation here?

kiszk · 2018-04-10T06:37:26Z

@HyukjinKwon thanks, fixed it.

HyukjinKwon · 2018-04-10T06:41:14Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameFunctionsSuite.scala

Maybe Seq.empty[Int]

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

Test build #89088 has finished for PR 21021 at commit 00ac4fb.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
trait ArraySortUtil extends ExpectsInputTypes with CodegenFallback
case class SortArray(base: Expression, ascendingOrder: Expression)
case class ArraySort(child: Expression) extends UnaryExpression with ArraySortUtil

viirya · 2018-04-10T07:11:41Z

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

...not support sorting array of type ${dt.simpleString} which is not orderable.

The original message is compatible with SortArray. Is it better to update the message in SortArray with this change?

Sounds good to me.

mgaido91 · 2018-04-10T11:23:31Z

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

why do we need this? Can't we just reuse the previous SortArray fixing the right to be TrueLiteral?

As you can see the result in UT, null handing is different. As you suggested, to reuse existing code as possible, I refactored by using ArraySortUtil.

I see, thanks. Then can't we add another flag for it? I just find a bit weird that we have two functions which do basically the same thing. Just one has one more flag than the other and they behave differently with nulls. As a user I would not understand when to use one or the other. What do you think? I'd be also interested in @gatorsmile's point of view, since he created the JIRA for this.

Moreover, can we at least improve than the description of the SortArray case class in order to point out clearly how it behaves with nulls? Now there is no mention about it.

Yeah, as you said they are doing similar things. Therefore, a new trait is not introduced to reuse as possible.
When one is subset of another one (e.g. size v.s. cardinality), we could take an approach that one calls another one. What I am doing in cardinality.

Good point about the description. I will add the description on how it works with null.

SparkQA · 2018-04-10T11:27:43Z

Test build #89092 has finished for PR 21021 at commit 359fc7f.

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

SparkQA · 2018-04-10T12:09:42Z

Test build #89095 has finished for PR 21021 at commit 0a500d3.

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

gatorsmile · 2018-04-10T15:38:34Z

cc @ueshin

SparkQA · 2018-04-10T19:55:09Z

Test build #89144 has finished for PR 21021 at commit 0203920.

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

kiszk · 2018-04-11T04:38:17Z

retest this please

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

Test build #89170 has finished for PR 21021 at commit 0203920.

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

mgaido91 · 2018-04-11T07:54:06Z

retest this please

SparkQA · 2018-04-11T11:50:41Z

Test build #89179 has finished for PR 21021 at commit 0203920.

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

SparkQA · 2018-04-18T22:01:40Z

Test build #89525 has finished for PR 21021 at commit 26ef2bb.

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

kiszk · 2018-04-19T13:52:10Z

retest this please

SparkQA · 2018-04-19T17:05:20Z

Test build #89576 has finished for PR 21021 at commit 9977f64.

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

SparkQA · 2018-04-19T17:19:43Z

Test build #89577 has finished for PR 21021 at commit 9977f64.

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

ueshin · 2018-04-20T09:17:12Z

python/pyspark/sql/functions.py


    :param col: name of column or expression

+    >>> from pyspark.sql.functions import sort_array


Do we need this?

ueshin · 2018-04-20T09:17:37Z

python/pyspark/sql/functions.py

+
+    :param col: name of column or expression
+
+    >>> from pyspark.sql.functions import array_sort


ueshin · 2018-04-20T09:39:04Z

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

-  }
+  // If -1, place null element at the end of the array
+  // If 1, place null element at the beginning of the array
+  protected def placeNullAtEnd: Int


I guess the name is confusing.

How about nullOrder?
nullOrder = -1 means null is the least, place at the beginning for asc and at the end for desc, whereas 1 means the greatest, place at the end for asc and at the beginning for desc.
And maybe we should define the constants like:

object ArraySortUtil { type NullOrder = Int object NullOrder { val Least: NullOrder = -1 val Greatest: NullOrder = 1 } }

ueshin · 2018-04-20T09:43:39Z

python/pyspark/sql/functions.py

+    >>> df = spark.createDataFrame([([2, 1, None, 3],),([1],),([],)], ['data'])
+    >>> df.select(array_sort(df.data).alias('r')).collect()
+    [Row(r=[1, 2, 3, None]), Row(r=[1]), Row(r=[])]
+     """


nit: an extra space?

ueshin · 2018-04-20T09:45:36Z

python/pyspark/sql/functions.py

    :param col: name of column or expression

+    >>> from pyspark.sql.functions import sort_array
    >>> df = spark.createDataFrame([([2, 1, 3],),([1],),([],)], ['data'])


We should include None to show where the None comes?

SparkQA · 2018-04-29T18:50:58Z

Test build #89969 has finished for PR 21021 at commit d1b0483.

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

kiszk · 2018-04-29T22:27:39Z

retest this please

SparkQA · 2018-04-30T02:23:34Z

Test build #89975 has finished for PR 21021 at commit d1b0483.

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

ueshin · 2018-05-01T04:04:42Z

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

+        s"org.apache.spark.sql.types.DataTypes.createDecimalType($p, $s)"
+      case ArrayType(et, cn) =>
+        s"org.apache.spark.sql.types.DataTypes.createArrayType($et, $cn)"
+      case MapType(kt, vt, cn) =>


We don't need for MapType because MapType is not orderable?

ueshin · 2018-05-01T04:06:00Z

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

+          s"apply(new java.util.ArrayList(${f.length}))"
+      case _ =>
+        s"org.apache.spark.sql.types.DataTypes.$elementType"
+    }


I'm wondering if this will work for all complex types, e.g. ArrayType(ArrayType(IntegerType))?
How about using reference object of elementType?

ueshin · 2018-05-01T05:01:33Z

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

+        s"org.apache.spark.sql.types.DataTypes.$elementType"
+    }
+    s"""
+       |Object[] $array = (Object[]) (($arrayData) $base).toArray(


How about using toObjectArray which doesn't need ClassTag?

ueshin · 2018-05-01T05:02:20Z

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

+    val genericArrayData = classOf[GenericArrayData].getName
+    val array = ctx.freshName("array")
+    val c = ctx.freshName("c")
+    val sort = if (elementType == NullType) "" else {


How about just copying the original array if elementType == NullType?

ueshin · 2018-05-01T10:08:12Z

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

+          s"apply(new java.util.ArrayList(${f.length}))"
+      case _ =>
+        s"org.apache.spark.sql.types.$elementType$$.MODULE$$"
+    }


I'm still wondering whether this will work or not. What if elementType is ArrayType(ArrayType(IntegerType))?
Can't we use a reference object of elementType?

Could you also add some tests using ArrayType and StructType for elementType?

Definitely, I added some complex test cases with nests.

SparkQA · 2018-05-02T03:37:24Z

Test build #90021 has finished for PR 21021 at commit 9f63a76.

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

kiszk · 2018-05-02T05:37:11Z

retest this please

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

Test build #90031 has finished for PR 21021 at commit 9f63a76.

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

gatorsmile · 2018-05-02T21:02:41Z

retest this please

kiszk · 2018-05-02T23:09:04Z

retest this please

SparkQA · 2018-05-03T02:58:22Z

Test build #90082 has finished for PR 21021 at commit 9f63a76.

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

kiszk · 2018-05-03T11:08:26Z

retest this please

SparkQA · 2018-05-03T14:54:12Z

Test build #90118 has finished for PR 21021 at commit 9f63a76.

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

SparkQA · 2018-05-04T03:21:37Z

Test build #90163 has finished for PR 21021 at commit e3fcaaa.

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

ueshin

LGTM except for some nits.

ueshin · 2018-05-04T07:05:13Z

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

+    val array = ctx.freshName("array")
+    val c = ctx.freshName("c")
+    if (elementType == NullType) {
+      s"${ev.value} = (($arrayData) $base).copy();"


nit: do we need cast base to ArrayData?

Good catch, done

ueshin · 2018-05-04T07:05:23Z

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

+        s"int $c = ${ctx.genComp(elementType, s"(($jt) $o1)", s"(($jt) $o2)")};"
+      }
+      s"""
+         |Object[] $array = (Object[]) (($arrayData) $base).toObjectArray($dataTypes);


ueshin · 2018-05-04T07:08:25Z

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

+    if (elementType == NullType) {
+      s"${ev.value} = (($arrayData) $base).copy();"
+    } else {
+      val dataTypes = ctx.addReferenceObj("dataType", elementType)


How about elementTypeTerm or something?

ueshin · 2018-05-04T07:13:17Z

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

-  extends BinaryExpression with ExpectsInputTypes with CodegenFallback {
-
-  def this(e: Expression) = this(e, Literal(true))
+trait ArraySortUtil extends ExpectsInputTypes {


How about ArraySortLike?

Sure, thank you for your review while it is a long-holiday week in Japan.

mgaido91 · 2018-05-04T10:11:23Z

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

+        val v1 = ctx.freshName("v1")
+        val v2 = ctx.freshName("v2")
+        s"""
+           |$jt $v1 = (($bt) $o1).${jt}Value();


why do we need to enforce the boxing? An why do we need to cast to the java type in the non primitive scenario?

IIUC, this is because compare() in java.util.Arrays.sort accepts two Object arguments. Thus, we do boxing here.
Now, I realized java.util.Arrays.sort has sort() method only for ascending. Let me use them for ascending and non-null.

Now I see, thanks.

mgaido91 · 2018-05-04T10:20:00Z

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

+        s"int $c = ${ctx.genComp(elementType, s"(($jt) $o1)", s"(($jt) $o2)")};"
+      }
+      s"""
+         |Object[] $array = $base.toObjectArray($elementTypeTerm);


can we avoid boxing? Probably it is quite hard for null handling, so we can ignore this. Did you try?

Since java.util.Arrays.sort accepts Object[], this PR always uses Object[]. For now, I support the cases for ascending and non-null primitive types by using java.util.Array.sort.
Like this thread, another PR can avoid boxing by using type-specialized sorting implementation in generic cases.

I am sorry that I cannot understand this phase Probably it is quite hard for null handling. Would it be possible to elaborate on your thought?

Now I added the handling of primitives in the PR you mentioned, so it handles that now.

My sentence meant that probably the main issue is to avoid problems with nulls for primitive types, since in that case the returned array doesn't contain null but the default value for null items IIUC. So probably it is some extra effort (we should also check if it is worth). I am fine having this done in a followup PR.

In my opinion, main issue is to find or prepare sort implementations to handle required specification (ascending, descending, or ordering of null, with various types, in specialized implementation).
If we can use existing implementation for now, we can handle them in this PR. Otherwise, we would like to handle such a case in another PR since it requires to implement sort algorithms.

Yes, probably the performance gain would not be worth this effort. I agree going on with the current implementation.

SparkQA · 2018-05-04T13:51:15Z

Test build #90189 has finished for PR 21021 at commit 2ad6bb8.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
trait ArraySortLike extends ExpectsInputTypes
case class ArraySort(child: Expression) extends UnaryExpression with ArraySortLike

SparkQA · 2018-05-04T14:01:51Z

Test build #90190 has finished for PR 21021 at commit 2c4404c.

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

SparkQA · 2018-05-04T17:14:10Z

Test build #90198 has finished for PR 21021 at commit 21521d8.

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

ueshin · 2018-05-07T06:21:54Z

Thanks! merging to master.

viirya reviewed Apr 10, 2018

View reviewed changes

kiszk changed the title ~~[SPARK-23916][SQL] Add array_sort function~~ [SPARK-23921][SQL] Add array_sort function Apr 10, 2018

HyukjinKwon reviewed Apr 10, 2018

View reviewed changes

viirya reviewed Apr 10, 2018

View reviewed changes

kiszk mentioned this pull request Apr 10, 2018

[SPARK-23916][SQL] Add array_join function #21011

Closed

mgaido91 reviewed Apr 10, 2018

View reviewed changes

kiszk force-pushed the SPARK-23921 branch from 0203920 to 26ef2bb Compare April 18, 2018 18:11

kiszk added 4 commits April 19, 2018 22:02

initial commit

72f31b3

address review comments

a9b6e3b

adding an update in functions.py

d57c14a

address review comment

9977f64

kiszk force-pushed the SPARK-23921 branch from 26ef2bb to 9977f64 Compare April 19, 2018 13:04

ueshin reviewed Apr 20, 2018

View reviewed changes

address review comments

172b2c5

ueshin reviewed May 1, 2018

View reviewed changes

address review comments

04a3ae5

ueshin reviewed May 1, 2018

View reviewed changes

address review comments

9f63a76

use ctx.addReferenceObj() for DataType in generated code

e3fcaaa

ueshin reviewed May 4, 2018

View reviewed changes

kiszk added 2 commits May 4, 2018 10:54

address review comments

2ad6bb8

Remove redundant cast

2c4404c

mgaido91 reviewed May 4, 2018

View reviewed changes

avoid boxing as possible

21521d8

asfgit closed this in 7564a9a May 7, 2018


		:param col: name of column or expression

		>>> from pyspark.sql.functions import sort_array


		:param col: name of column or expression

		>>> from pyspark.sql.functions import array_sort

[SPARK-23921][SQL] Add array_sort function #21021

[SPARK-23921][SQL] Add array_sort function #21021

Uh oh!

Conversation

kiszk commented Apr 10, 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!

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!

HyukjinKwon commented Apr 10, 2018

Uh oh!

viirya Apr 10, 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!

kiszk commented Apr 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 10, 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!

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

Uh oh!

SparkQA commented Apr 10, 2018

Uh oh!

gatorsmile commented Apr 10, 2018

Uh oh!

SparkQA commented Apr 10, 2018

Uh oh!

kiszk commented Apr 11, 2018

Uh oh!

SparkQA commented Apr 11, 2018

Uh oh!

mgaido91 commented Apr 11, 2018

Uh oh!

SparkQA commented Apr 11, 2018

Uh oh!

SparkQA commented Apr 18, 2018

Uh oh!

kiszk commented Apr 19, 2018

Uh oh!

SparkQA commented Apr 19, 2018

Uh oh!

SparkQA commented Apr 19, 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!

viirya Apr 10, 2018 •

edited

Loading