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

mgaido91 · 2018-04-09T16:22:03Z

What changes were proposed in this pull request?

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

The function accepts an array of string which is to be joined, a string which is the delimiter to use between the items of the first argument and optionally a string which is used to replace null values.

How was this patch tested?

added UTs

SparkQA · 2018-04-09T19:37:38Z

Test build #89067 has finished for PR 21011 at commit 1408cfc.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ArrayJoin(

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

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

+    val df = Seq(
+      (Seq[String]("a", "b"), ","),
+      (Seq[String]("a", null, "b"), ","),
+      (Seq[String](), ",")


Maybe Seq.empty[String]

HyukjinKwon · 2018-04-10T06:46:13Z

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

+       hello world
+      > SELECT _FUNC_(array('hello', null ,'world'), ' ', ',');
+       hello , world
+  """)


and since.

kiszk · 2018-04-10T07:15:35Z

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

+       hello world
+      > SELECT _FUNC_(array('hello', null ,'world'), ' ', ',');
+       hello , world
+  """)


add since. see this discussion.

SparkQA · 2018-04-10T09:54:31Z

Test build #89094 has finished for PR 21011 at commit e52ff85.

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

mgaido91 · 2018-04-10T10:47:43Z

retest this please

SparkQA · 2018-04-10T14:44:40Z

Test build #89106 has finished for PR 21011 at commit e52ff85.

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

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

cc @ueshin

ueshin

LGTM except for some nits.

ueshin · 2018-04-13T11:47:08Z

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

+         """.stripMargin)
+    } else {
+      ev.copy(s"""
+           |boolean ${ev.isNull} = false;


nit: I guess we can remove this?

ueshin · 2018-04-13T12:23:42Z

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

+             |}
+             |$buffer.append(${replacementGen.value});
+             |$firstItem = false;
+               """.stripMargin


nit: indent

ueshin · 2018-04-13T12:24:35Z

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

+           |$code
+         """.stripMargin)
+    } else {
+      ev.copy(s"""


nit: maybe we need a line break between copy( and s"""?

ueshin · 2018-04-13T12:24:50Z

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

+        s"""
+           |${replacementGen.code}
+           |$execCode
+           """.stripMargin


nit: indent

ueshin · 2018-04-13T12:25:47Z

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

+    if (delimiterEval == null) return null
+    val nullReplacementEval = nullReplacement.map(_.eval(input))
+    if (nullReplacementEval.contains(null)) return null
+


nit: remove an extra line.

I removed the other one.... :)

ueshin · 2018-04-13T12:26:50Z

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

+      Seq(ArrayType(StringType), StringType, StringType)
+    } else {
+      Seq(ArrayType(StringType), StringType)
+    }


nit: indent?

I don't think the indent is wrong since this is for the if...else and not for the method itself

Hmm, I think the indent is 2 spaces in this case. For example, namedExpressions.scala#L170-L174 or regexpExpressions.scala#L46-L51.

ops, you are right....I am not sure where I saw it differently...maybe I just got confused...sorry, I am fixing it

ueshin · 2018-04-13T12:26:57Z

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

+      Seq(array, delimiter, nullReplacement.get)
+    } else {
+      Seq(array, delimiter)
+    }


nit: indent?

SparkQA · 2018-04-13T16:11:15Z

Test build #89342 has finished for PR 21011 at commit ad0d4aa.

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

mgaido91 · 2018-04-13T16:18:50Z

retest this please

SparkQA · 2018-04-13T16:37:55Z

Test build #89340 has finished for PR 21011 at commit dd9482e.

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

SparkQA · 2018-04-13T20:18:28Z

Test build #89351 has finished for PR 21011 at commit ad0d4aa.

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

mgaido91 · 2018-04-17T11:27:32Z

any more comments @ueshin ?

SparkQA · 2018-04-17T15:20:19Z

Test build #89451 has finished for PR 21011 at commit 703c09c.

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

ueshin · 2018-04-18T07:38:01Z

python/pyspark/sql/functions.py

+def array_join(col, delimiter, null_replacement=None):
+    """
+    Concatenates the elements of `column` using the `delimiter`. Null values are replaced with
+    `nullReplacement` if set, otherwise they are ignored.


nit: null_replacement?

SparkQA · 2018-04-19T01:37:11Z

Test build #89534 has finished for PR 21011 at commit b1597d7.

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

mgaido91 · 2018-04-20T16:32:37Z

any more comments?

SparkQA · 2018-04-20T20:15:39Z

Test build #89645 has finished for PR 21011 at commit e9d7baa.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class Summarizer(object):
class SummaryBuilder(JavaWrapper):
case class Reverse(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
case class ArrayPosition(left: Expression, right: Expression)
case class ElementAt(left: Expression, right: Expression) extends GetMapValueUtil
case class Concat(children: Seq[Expression]) extends Expression
abstract class GetMapValueUtil extends BinaryExpression with ImplicitCastInputTypes
case class GetMapValue(child: Expression, key: Expression)
class ArrayDataIndexedSeq[T](arrayData: ArrayData, dataType: DataType) extends IndexedSeq[T]

mgaido91 · 2018-04-25T13:37:23Z

kindly ping @ueshin

ueshin · 2018-04-26T04:36:50Z

Thanks! merging to master.

[SPARK-23916][SQL] Add array_join function

1408cfc

HyukjinKwon reviewed Apr 10, 2018

View reviewed changes

kiszk reviewed Apr 10, 2018

View reviewed changes

mgaido91 added 3 commits April 10, 2018 09:51

Merge branch 'master' of github.com:apache/spark into SPARK-23916

b9cea24

fix python doctest

144202f

add since + address comment

e52ff85

ueshin reviewed Apr 13, 2018

View reviewed changes

mgaido91 added 2 commits April 13, 2018 15:00

review comments

dd9482e

fix indent

ad0d4aa

Merge branch 'master' of github.com:apache/spark into SPARK-23916

703c09c

ueshin reviewed Apr 18, 2018

View reviewed changes

fix null_replacement

b1597d7

Merge branch 'master' of github.com:apache/spark into SPARK-23916

e9d7baa

asfgit closed this in cd10f9d Apr 26, 2018

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

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

Uh oh!

Conversation

mgaido91 commented Apr 9, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

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

SparkQA commented Apr 10, 2018

Uh oh!

mgaido91 commented Apr 10, 2018

Uh oh!

SparkQA commented Apr 10, 2018

Uh oh!

gatorsmile commented Apr 10, 2018

Uh oh!

ueshin 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!

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

Uh oh!

mgaido91 commented Apr 13, 2018

Uh oh!

SparkQA commented Apr 13, 2018

Uh oh!

SparkQA commented Apr 13, 2018

Uh oh!

mgaido91 commented Apr 17, 2018

Uh oh!

SparkQA commented Apr 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 19, 2018

Uh oh!

mgaido91 commented Apr 20, 2018

Uh oh!

SparkQA commented Apr 20, 2018

Uh oh!

mgaido91 commented Apr 25, 2018

Uh oh!

ueshin commented Apr 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone