[SPARK-24574][SQL] array_contains, array_position, array_remove and element_at functions deal with Column type #21581

chongguang · 2018-06-17T20:26:45Z

What changes were proposed in this pull request?

For the function def array_contains(column: Column, value: Any): Column , if we pass the value parameter as a Column type, it will yield a runtime exception.

This PR proposes a pattern matching to detect if value is of type Column. If yes, it will use the .expr of the column, otherwise it will work as it used to.

Same thing for array_position, array_remove and element_at functions

How was this patch tested?

Unit test modified to cover this code change.

Ping @ueshin

maropu · 2018-06-18T02:34:44Z

sql/core/src/main/scala/org/apache/spark/sql/functions.scala

@@ -3077,12 +3077,16 @@ object functions {
  //////////////////////////////////////////////////////////////////////////////////////////////

  /**
-   * Returns null if the array is null, true if the array contains `value`, and false otherwise.
+   * Returns null if the array is null, true if the array contains `value` or the content of


We need to update this comment? I think content of value is a little ambiguous.

Hey, thanks for the message. Do you want me to change the comment back? I see that you have started the test, is it too late?

you can fix now

maropu · 2018-06-18T02:36:35Z

BTW, I found the other 3 similar issues there;

scala> Seq((Seq(1, 2, 3), 2)).toDF("a", "b").selectExpr("array_position(a, b)").show
+--------------------+
|array_position(a, b)|
+--------------------+
|                   2|
+--------------------+

scala> Seq((Seq(1, 2, 3), 2)).toDF("a", "b").selectExpr("element_at(a, b)").show
+----------------+
|element_at(a, b)|
+----------------+
|               2|
+----------------+

scala> Seq((Seq(1, 2, 3), 2)).toDF("a", "b").selectExpr("array_remove(a, b)").show
+------------------+
|array_remove(a, b)|
+------------------+
|            [1, 3]|
+------------------+

I think this is a tiny fix, so IMHO this pr might need to address all the issues here. cc: @ueshin

HyukjinKwon · 2018-06-18T04:15:25Z

ok to test

SparkQA · 2018-06-18T07:05:02Z

Test build #92015 has finished for PR 21581 at commit 28aa515.

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

maropu · 2018-06-18T07:12:03Z

retest this please

viirya · 2018-06-18T09:32:57Z

sql/core/src/main/scala/org/apache/spark/sql/functions.scala

   * @group collection_funcs
   * @since 1.5.0
   */
  def array_contains(column: Column, value: Any): Column = withExpr {
-    ArrayContains(column.expr, Literal(value))


Other similar expressions are ArrayPosition, ElementAt, ArrayRemove.

Oh, I just found @maropu has pointed it out in #21581 (comment).

SparkQA · 2018-06-18T10:52:48Z

Test build #92017 has finished for PR 21581 at commit 28aa515.

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

SparkQA · 2018-06-18T15:58:49Z

Test build #92028 has finished for PR 21581 at commit e65611e.

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

SparkQA · 2018-06-18T15:59:34Z

Test build #92030 has finished for PR 21581 at commit 8440d5b.

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

SparkQA · 2018-06-18T16:18:18Z

Test build #92029 has finished for PR 21581 at commit b6e150c.

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

chongguang · 2018-06-18T20:23:28Z

@viirya @maropu @HyukjinKwon All the 4 functions have been modified.

ueshin

I'm sorry for the delay.
I left some comments. Thanks!

ueshin · 2018-06-20T02:42:12Z

sql/core/src/main/scala/org/apache/spark/sql/functions.scala

@@ -3082,7 +3082,10 @@ object functions {
   * @since 1.5.0
   */
  def array_contains(column: Column, value: Any): Column = withExpr {
-    ArrayContains(column.expr, Literal(value))
+    value match {


On second thoughts, we should use lit(), e.g., ArrayContains(column.expr, lit(value).expr)? WDYT? @viirya @maropu @HyukjinKwon

Yup, that's what I was thinking too from my glance.

Yes, I think so.

ueshin · 2018-06-20T02:42:55Z

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

+    checkAnswer(
+      df.select(array_contains(df("a"), df("c"))),
+      Seq(Row(true), Row(false))
+    )


Can you add another test to use selectExpr, e.g., df.selectExpr("array_contains(a, c)")?

ueshin · 2018-06-20T09:27:49Z

sql/core/src/main/scala/org/apache/spark/sql/functions.scala

@@ -3082,7 +3082,10 @@ object functions {
   * @since 1.5.0
   */
  def array_contains(column: Column, value: Any): Column = withExpr {
-    ArrayContains(column.expr, Literal(value))
+    value match {


@chongguang I mean, use just ArrayContains(column.expr, lit(value).expr) instead of value match { .... The lit() should handle literals and columns well.

SparkQA · 2018-06-20T10:23:09Z

Test build #92126 has finished for PR 21581 at commit f8c5b43.

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

chongguang · 2018-06-20T10:35:29Z

@ueshin Done! :) But tests fail... can you launch the tests again please?

HyukjinKwon · 2018-06-20T10:43:34Z

you could just push the newer changes. that will retrigger the tests.

SparkQA · 2018-06-20T16:16:38Z

Test build #92136 has finished for PR 21581 at commit ddd94f7.

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

ueshin · 2018-06-20T17:12:49Z

Jenkins, retest this please.

ueshin · 2018-06-20T17:17:03Z

LGTM pending tests.

HyukjinKwon

LGTM too

SparkQA · 2018-06-20T20:36:48Z

Test build #92141 has finished for PR 21581 at commit ddd94f7.

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

ueshin · 2018-06-21T03:12:32Z

@chongguang I think this pr is ready to merge, so I tried, but seems like the commits in this pr aren't connected with your GitHub account. If you want to connect the merge commit to your account, could you let me know the email address connected to your account? Thanks!

HyukjinKwon · 2018-06-21T03:45:19Z

oh haha FYI that works after it's merged if @chongguang link the email into his Github profile too. I asked the same thing in databricks/spark-xml before :)

ueshin · 2018-06-21T04:21:00Z

But the emails of commits in this pr seem not valid, just for the local computer.

HyukjinKwon · 2018-06-21T04:42:44Z

Oh, I see.

chongguang · 2018-06-21T06:11:54Z

Hey @ueshin I just updated the email address linked to my github account, it is now lcg31439@gmail.com

Thanks

HyukjinKwon · 2018-06-21T06:59:03Z

Merged to master.

Chongguang LIU added 2 commits June 17, 2018 20:17

array_contains function deals with Column type for the second parameter.

27733f9

add unit test for Column type

28aa515

maropu reviewed Jun 18, 2018

View reviewed changes

viirya reviewed Jun 18, 2018

View reviewed changes

Chongguang LIU added 4 commits June 18, 2018 14:06

array_position deals with Column Type

e65611e

array_remove function deals with Column Type

b6e150c

element_at function deals with Column Type

eabdb6e

reset doc for array_contains function

8440d5b

chongguang changed the title ~~[SPARK-24574][SQL] array_contains function deals with Column type~~ [SPARK-24574][SQL] array_contains, array_position, array_remove and element_at functions deal with Column type Jun 18, 2018

ueshin reviewed Jun 20, 2018

View reviewed changes

use lit() and add unit tests

f8c5b43

ueshin reviewed Jun 20, 2018

View reviewed changes

use lit() to unify the cases

ddd94f7

HyukjinKwon approved these changes Jun 20, 2018

View reviewed changes

asfgit closed this in 7236e75 Jun 21, 2018

asmello mentioned this pull request Mar 8, 2019

[SPARK-26979][PYTHON] Add missing string column name support for some SQL functions #23882

Closed

[SPARK-24574][SQL] array_contains, array_position, array_remove and element_at functions deal with Column type #21581

[SPARK-24574][SQL] array_contains, array_position, array_remove and element_at functions deal with Column type #21581

Conversation

chongguang commented Jun 17, 2018 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maropu commented Jun 18, 2018

HyukjinKwon commented Jun 18, 2018

SparkQA commented Jun 18, 2018

maropu commented Jun 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jun 18, 2018

SparkQA commented Jun 18, 2018

SparkQA commented Jun 18, 2018

SparkQA commented Jun 18, 2018

chongguang commented Jun 18, 2018

ueshin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jun 20, 2018

chongguang commented Jun 20, 2018 • edited Loading

HyukjinKwon commented Jun 20, 2018

SparkQA commented Jun 20, 2018

ueshin commented Jun 20, 2018

ueshin commented Jun 20, 2018

HyukjinKwon left a comment

Choose a reason for hiding this comment

SparkQA commented Jun 20, 2018

ueshin commented Jun 21, 2018

HyukjinKwon commented Jun 21, 2018 • edited Loading

ueshin commented Jun 21, 2018

HyukjinKwon commented Jun 21, 2018

chongguang commented Jun 21, 2018

HyukjinKwon commented Jun 21, 2018

chongguang commented Jun 17, 2018 •

edited

Loading

chongguang commented Jun 20, 2018 •

edited

Loading

HyukjinKwon commented Jun 21, 2018 •

edited

Loading