[SPARK-23924][SQL] Add element_at function #21053

kiszk · 2018-04-12T13:23:54Z

What changes were proposed in this pull request?

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

This function returns element of array at given index in value if column is array, or returns value for the given key in value if column is map.

How was this patch tested?

Added UTs

SparkQA · 2018-04-12T15:57:37Z

Test build #89266 has finished for PR 21053 at commit d7ec6cc.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ElementAt(left: Expression, right: Expression) extends GetMapValueUtil
abstract class GetMapValueUtil extends BinaryExpression with ImplicitCastInputTypes
case class GetMapValue(child: Expression, key: Expression) extends GetMapValueUtil

kiszk · 2018-04-12T17:27:28Z

retest this please

SparkQA · 2018-04-12T20:45:12Z

Test build #89284 has finished for PR 21053 at commit d7ec6cc.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ElementAt(left: Expression, right: Expression) extends GetMapValueUtil
abstract class GetMapValueUtil extends BinaryExpression with ImplicitCastInputTypes
case class GetMapValue(child: Expression, key: Expression) extends GetMapValueUtil

gatorsmile · 2018-04-13T06:47:49Z

cc @ueshin

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

Test build #89313 has finished for PR 21053 at commit bb0ab45.

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

kiszk · 2018-04-13T07:25:45Z

retest this please

SparkQA · 2018-04-13T10:17:36Z

Test build #89321 has finished for PR 21053 at commit bb0ab45.

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

viirya · 2018-04-13T12:45:28Z

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

@@ -287,3 +287,106 @@ case class ArrayContains(left: Expression, right: Expression)

  override def prettyName: String = "array_contains"
 }
+
+/**
+ * Returns the value of index `right` in Array `left` or key right` in Map `left`.


We can improve this doc. There is also broken quote.

good catch, thanks

SparkQA · 2018-04-13T14:13:08Z

Test build #89336 has finished for PR 21053 at commit 0146945.

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

SparkQA · 2018-04-13T20:34:53Z

Test build #89356 has finished for PR 21053 at commit 35844f8.

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

rxin · 2018-04-13T23:28:13Z

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

@@ -413,6 +413,78 @@ class DataFrameFunctionsSuite extends QueryTest with SharedSQLContext {
    )
  }

+  test("element at function") {


why do we need so many test cases here? this is just to verify the api works end to end.

also the function is element_at, not "element at" ...

good catch, thanks

ueshin

Could you correct the description saying element_at instead of array_position?

ueshin · 2018-04-16T03:56:23Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeExtractors.scala

+ * We need to do type checking here as `key` expression maybe unresolved.
+ */
+case class GetMapValue(child: Expression, key: Expression) extends GetMapValueUtil
+  with ExtractValue with NullIntolerant {


nit: maybe extends ... should be this line.

ueshin · 2018-04-16T04:05:52Z

python/pyspark/sql/functions.py

@@ -1846,6 +1846,27 @@ def array_contains(col, value):
    return Column(sc._jvm.functions.array_contains(_to_java_column(col), value))


+@since(2.4)


We need to annotate as @ignore_unicode_prefix?

I see, thanks

ueshin · 2018-04-16T04:22:43Z

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

+             |    ${ev.value} = ${CodeGenerator.getValue(eval1, dataType, index)};
+             |  }
+             |}
+           """


stripMargin is missing?

good catch, thanks

ueshin · 2018-04-16T04:22:57Z

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

+               |if ($eval1.isNullAt($index)) {
+               |  ${ev.isNull} = true;
+               |} else
+             """


stripMargin is missing?

viirya · 2018-04-16T05:05:18Z

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

+    )
+  }
+
+  override def nullable: Boolean = true


~~Maybe? ```scala override def nullable: Boolean = left.dataType match { case a: ArrayType => a.containsNull case m: MapType => m.valueContainsNull } || left.nullable || right.nullable ```~~

I'm afraid it's wrong because this returns null when the given index is "out of bounds" (array.numElements() < math.abs(index)) for array type or the given key doesn't exist for map type.

ah, I see. Invalid right can cause null result too.

yeah, may depend on right value, too.

viirya · 2018-04-16T05:12:48Z

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

+@ExpressionDescription(
+  usage = """
+    _FUNC_(array, index) - Returns element of array at given index. If index < 0, accesses elements
+      from the last to the first.


nit: Returns NULL if the index exceeds the length of the array.

viirya · 2018-04-16T05:14:45Z

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

+          } else {
+            array.numElements() + index
+          }
+          if (array.isNullAt(idx)) {


nit:

if(left.dataType.asInstanceOf[ArrayType].containsNull && array.isNullAt(idx)) { ... }

SparkQA · 2018-04-16T05:56:36Z

Test build #89383 has finished for PR 21053 at commit 013a53e.

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

SparkQA · 2018-04-16T20:09:26Z

Test build #89403 has finished for PR 21053 at commit 5bdee7e.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class GetMapValue(child: Expression, key: Expression)

kiszk · 2018-04-18T02:20:41Z

@ueshin would it be possible to review this again?

SparkQA · 2018-04-18T06:00:50Z

Test build #89476 has finished for PR 21053 at commit 98465b1.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class GetMapValue(child: Expression, key: Expression)

ueshin

LGTM except for some nits.

ueshin · 2018-04-18T06:46:14Z

python/pyspark/sql/functions.py

+    returns value for the given key in value if col is map.
+
+    :param col: name of column containing array or map
+    :param value: value to check for in array or key to check for in map


Can you add a note or something to notice the index is 1-based.

ueshin · 2018-04-18T06:59:52Z

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

+@ExpressionDescription(
+  usage = """
+    _FUNC_(array, index) - Returns element of array at given index. If index < 0, accesses elements
+      from the last to the first. Returns NULL if the index exceeds the length of the array.


ueshin · 2018-04-18T07:03:41Z

python/pyspark/sql/functions.py

@@ -1846,6 +1846,28 @@ def array_contains(col, value):
    return Column(sc._jvm.functions.array_contains(_to_java_column(col), value))


+@ignore_unicode_prefix
+@since(2.4)
+def element_at(col, value):


The name value is confusing because this is for an index or a key?

Umm, the 2nd argument acts as index for an array or key for a map. This is why I used value.
Can I use index?

How about extraction which is from Column.apply(extraction)? cc @gatorsmile

ueshin · 2018-04-18T07:07:56Z

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

+  override def dataType: DataType = left.dataType match {
+    case _: ArrayType => left.dataType.asInstanceOf[ArrayType].elementType
+    case _: MapType => left.dataType.asInstanceOf[MapType].valueType
+  }


nit: how about:

override def dataType: DataType = left.dataType match { case ArrayType(elementType, _) => elementType case MapType(_, valueType, _) => valueType }

SparkQA · 2018-04-18T15:32:41Z

Test build #89502 has finished for PR 21053 at commit c734607.

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

SparkQA · 2018-04-18T18:21:13Z

Test build #89520 has finished for PR 21053 at commit 68e8907.

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

kiszk · 2018-04-18T18:48:26Z

retest this please

SparkQA · 2018-04-18T22:44:24Z

Test build #89528 has finished for PR 21053 at commit 68e8907.

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

SparkQA · 2018-04-19T03:54:52Z

Test build #89545 has finished for PR 21053 at commit 2fbb1e8.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

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

Test build #89553 has finished for PR 21053 at commit 96dd82b.

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

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

Test build #89554 has finished for PR 21053 at commit 90e026e.

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

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

Test build #89552 has finished for PR 21053 at commit 06fb27e.

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

kiszk · 2018-04-19T07:33:27Z

retest this please

SparkQA · 2018-04-19T11:36:35Z

Test build #89558 has finished for PR 21053 at commit 90e026e.

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

ueshin · 2018-04-19T11:59:08Z

Thanks! merging to master.

kiszk · 2018-04-19T12:08:40Z

thank you very much for your comments

viirya reviewed Apr 13, 2018

View reviewed changes

rxin reviewed Apr 13, 2018

View reviewed changes

ueshin reviewed Apr 16, 2018

View reviewed changes

viirya reviewed Apr 16, 2018

View reviewed changes

kiszk force-pushed the SPARK-23924 branch from 013a53e to 5bdee7e Compare April 16, 2018 16:23

kiszk force-pushed the SPARK-23924 branch from 5bdee7e to 98465b1 Compare April 18, 2018 02:20

ueshin reviewed Apr 18, 2018

View reviewed changes

kiszk force-pushed the SPARK-23924 branch from 98465b1 to c734607 Compare April 18, 2018 12:26

kiszk added 3 commits April 19, 2018 12:36

initial commit

b8c781d

fix test failure

77b572a

fix test failure

9d38e4b

kiszk added 6 commits April 19, 2018 12:36

address review comment

61c904b

address review comment

872a500

address review comments

d167b4b

address review comments

5163679

fix python test failure

a705397

rebase with master

2fbb1e8

kiszk force-pushed the SPARK-23924 branch from 68e8907 to 2fbb1e8 Compare April 19, 2018 03:48

kiszk added 3 commits April 19, 2018 06:16

fix mistakes during rebase

06fb27e

add @ignore_unicode_prefix

96dd82b

Improve comment

90e026e

asfgit closed this in 46bb2b5 Apr 19, 2018

		@@ -1846,6 +1846,27 @@ def array_contains(col, value):
		return Column(sc._jvm.functions.array_contains(_to_java_column(col), value))


		@since(2.4)

[SPARK-23924][SQL] Add element_at function #21053

[SPARK-23924][SQL] Add element_at function #21053

Conversation

kiszk commented Apr 12, 2018 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Apr 12, 2018

kiszk commented Apr 12, 2018

SparkQA commented Apr 12, 2018

gatorsmile commented Apr 13, 2018

SparkQA commented Apr 13, 2018

kiszk commented Apr 13, 2018

SparkQA commented Apr 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Apr 13, 2018

SparkQA commented Apr 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

viirya Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiszk Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Apr 16, 2018

SparkQA commented Apr 16, 2018

kiszk commented Apr 18, 2018

SparkQA commented Apr 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

Choose a reason for hiding this comment

SparkQA commented Apr 18, 2018

SparkQA commented Apr 18, 2018

kiszk commented Apr 18, 2018

SparkQA commented Apr 18, 2018

SparkQA commented Apr 19, 2018

SparkQA commented Apr 19, 2018

SparkQA commented Apr 19, 2018

SparkQA commented Apr 19, 2018

kiszk commented Apr 19, 2018

SparkQA commented Apr 19, 2018

ueshin commented Apr 19, 2018

kiszk commented Apr 19, 2018

kiszk commented Apr 12, 2018 •

edited

Loading

viirya Apr 16, 2018 •

edited

Loading

kiszk Apr 16, 2018 •

edited

Loading