[SPARK-23919][SQL] Add array_position function #21037

kiszk · 2018-04-11T06:24:59Z

What changes were proposed in this pull request?

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

The function returns the position of the first occurrence of the element in array x (or 0 if not found) using 1-based index as BigInt.

How was this patch tested?

Added UTs

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

Test build #89176 has finished for PR 21037 at commit 535e511.

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

kiszk · 2018-04-11T11:07:36Z

retest this please

HyukjinKwon · 2018-04-11T12:43:59Z

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

since :)?

Oh, good catch

SparkQA · 2018-04-11T13:39:44Z

Test build #89188 has finished for PR 21037 at commit 535e511.

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

SparkQA · 2018-04-11T14:54:45Z

Test build #89200 has finished for PR 21037 at commit 908472c.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-04-11T23:02:59Z

Test build #89218 has finished for PR 21037 at commit ce5df02.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ArrayPosition(str: Expression, substr: Expression)

bersprockets · 2018-04-12T00:37:27Z

Is array_position a string function? The function array_contains, for example, works on arrays.

Also, this new function seems very similar as the existing instr ("Locate the position of the first occurrence of substr column in the given string"), except the return type is BigDecimal rather than Integer.

For example, I ran a query using your branch:

scala> val df = Seq((Seq(1, 2, 3), "this is a test"), (Seq(7, 8, 9, 3), "yeah this")).toDF("a", "b")
df: org.apache.spark.sql.DataFrame = [a: array, b: string]

scala> df.select(array_contains('a, 2), array_position('b, "this"), instr('b, "this")).show
+--------------------+-----------------------+--------------+
|array_contains(a, 2)|array_position(b, this)|instr(b, this)|
+--------------------+-----------------------+--------------+
|                true|                      1|             1|
|               false|                      6|             6|
+--------------------+-----------------------+--------------+


scala> 
scala> df.select(array_position('a, 3)).show
:35: error: type mismatch;
 found   : Int(3)
 required: String
       df.select(array_position('a, 3)).show
                                    ^

scala> df.select(array_contains('a, 3)).show
+--------------------+
|array_contains(a, 3)|
+--------------------+
|                true|
|                true|
+--------------------+


scala>

kiszk · 2018-04-12T02:06:53Z

@bersprockets You are absolutely right. I made mistake. I will completely reimplement this soon.

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

Test build #89238 has finished for PR 21037 at commit 3d687a0.

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

SparkQA · 2018-04-12T13:39:01Z

Test build #89255 has finished for PR 21037 at commit 6a464a6.

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

kiszk · 2018-04-12T13:49:33Z

retest this please

viirya · 2018-04-12T14:25:47Z

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

Looks like this is a broken sentence?

Good catch, thanks

viirya · 2018-04-12T14:27:43Z

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

What is the behavior when left contains null element?

Ah, if an array in left contains a null element, as you can see a UT, it works as an usual element.

val left = Literal.create(Seq[String](null, ""), ArrayType(StringType)) // contains null checkEvaluation(ArrayPosition(left, Literal(""), 2L) checkEvaluation(ArrayPosition(left, Literal.create(null, StringType)), null)

So we can't know the position of null in the array even if the array contains null?

According to these UTs in Presto, you are right.

SparkQA · 2018-04-12T16:25:30Z

Test build #89268 has finished for PR 21037 at commit 6a464a6.

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

SparkQA · 2018-04-13T07:05:02Z

Test build #89314 has finished for PR 21037 at commit 16ae59c.

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

kiszk · 2018-04-13T07:27:06Z

retest this please

SparkQA · 2018-04-13T11:03:08Z

Test build #89322 has finished for PR 21037 at commit 16ae59c.

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

ueshin · 2018-04-16T02:59:46Z

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

stripMargin is missing?

good catch, sorry again

ueshin · 2018-04-16T03:02:47Z

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

nit: $pos

ueshin · 2018-04-16T03:14:48Z

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

I guess left.dataType.asInstanceOf[ArrayType].containsNull is not related to the nullability of this function?

ah, you are right.

ueshin · 2018-04-16T03:16:47Z

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

So we can't know the position of null in the array even if the array contains null?

ueshin · 2018-04-16T03:35:14Z

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

I guess we need to check null for each array element?

E.g. for the test Array Position in CollectionExpressionsSuite, if a0 is as follows:

val a0 = Literal.create(Seq(1, null, 2, 3), ArrayType(IntegerType))

checkEvaluation(ArrayPosition(a0, Literal(0)), 0L) will fail.

I totally agree with you. I added one test case for this.

ueshin · 2018-04-16T03:42:24Z

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

nit: remove an extra line.

SparkQA · 2018-04-16T21:52:27Z

Test build #89411 has finished for PR 21037 at commit aac6f64.

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

kiszk · 2018-04-18T02:15:10Z

@ueshin could you please review again?

ueshin

LGTM except for some nits.

ueshin · 2018-04-18T03:06:24Z

python/pyspark/sql/functions.py

Returns 0 if substr could not be found in str -> Returns 0 if the value could not be found in the array or something?

ueshin · 2018-04-18T03:09:13Z

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

Returns 0 if substr could not be found in str -> Returns 0 if the value could not be found in the array or something?

ueshin · 2018-04-18T03:12:15Z

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

We don't need this here because this is the same as the one in BinaryExpression.

ueshin · 2018-04-18T03:13:43Z

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

nit: remove that?
The first character in str has index 1. -> The first element in the array has index 1. or something?

ueshin · 2018-04-18T06:05:58Z

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

nit: unnecessary import?

SparkQA · 2018-04-18T17:12:21Z

Test build #89506 has finished for PR 21037 at commit 7362b1c.

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

ueshin · 2018-04-19T02:58:52Z

Thanks! merging to master.

HyukjinKwon

LGTM too.

HyukjinKwon · 2018-04-19T03:15:07Z

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

+      > SELECT _FUNC_(array(3, 2, 1), 1);
+       3
+  """,
+  since = "2.4.0")


Just wanted to note that we can use note here too:

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

Lines 101 to 103 in 2ce37b5

note = """

Use RLIKE to match with standard regular expressions.

""")

I am mentioning this because we are adding many functions now :-).

HyukjinKwon reviewed Apr 11, 2018

View reviewed changes

kiszk force-pushed the SPARK-23919 branch from 908472c to ce5df02 Compare April 11, 2018 20:03

kiszk mentioned this pull request Apr 12, 2018

[SPARK-23923][SQL] Add cardinality function #21031

Closed

viirya reviewed Apr 12, 2018

View reviewed changes

ueshin reviewed Apr 16, 2018

View reviewed changes

kiszk force-pushed the SPARK-23919 branch from 16ae59c to aac6f64 Compare April 16, 2018 18:01

ueshin reviewed Apr 18, 2018

View reviewed changes

kiszk added 6 commits April 18, 2018 21:27

initial commit

fb04539

remove unnecessary scalastyle pragma

3d8069d

address review comment

5339a65

rebase with master

f1238b6

reimplement this for desired behavior

07305db

use long for result instead of BigInt

3a16231

kiszk added 3 commits April 18, 2018 21:38

address review comment

d4cebed

address review comments

9a0321d

address review comments

7362b1c

kiszk force-pushed the SPARK-23919 branch from aac6f64 to 7362b1c Compare April 18, 2018 13:21

asfgit closed this in d5bec48 Apr 19, 2018

HyukjinKwon reviewed Apr 19, 2018

View reviewed changes

	note = """
	Use RLIKE to match with standard regular expressions.
	""")

[SPARK-23919][SQL] Add array_position function #21037

[SPARK-23919][SQL] Add array_position function #21037

Uh oh!

Conversation

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

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Apr 11, 2018

Uh oh!

kiszk commented Apr 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 11, 2018

Uh oh!

SparkQA commented Apr 11, 2018

Uh oh!

SparkQA commented Apr 11, 2018

Uh oh!

bersprockets commented Apr 12, 2018

Uh oh!

kiszk commented Apr 12, 2018

Uh oh!

SparkQA commented Apr 12, 2018

Uh oh!

SparkQA commented Apr 12, 2018

Uh oh!

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

kiszk Apr 12, 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 Apr 16, 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!

SparkQA commented Apr 12, 2018

Uh oh!

SparkQA commented Apr 13, 2018

Uh oh!

kiszk commented Apr 13, 2018

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 16, 2018

Uh oh!

kiszk commented Apr 18, 2018

Uh oh!

ueshin left a comment

kiszk commented Apr 11, 2018 •

edited

Loading

kiszk Apr 12, 2018 •

edited

Loading

kiszk Apr 16, 2018 •

edited

Loading