[SPARK-23875][SQL] Add IndexedSeq wrapper for ArrayData #20984

viirya · 2018-04-05T14:37:54Z

What changes were proposed in this pull request?

We don't have a good way to sequentially access UnsafeArrayData with a common interface such as Seq. An example is MapObject where we need to access several sequence collection types together. But UnsafeArrayData doesn't implement ArrayData.array. Calling toArray will copy the entire array. We can provide an IndexedSeq wrapper for ArrayData, so we can avoid copying the entire array.

How was this patch tested?

Added test.

SparkQA · 2018-04-05T18:22:10Z

Test build #88942 has finished for PR 20984 at commit 27eddd7.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class ArrayDataIndexedSeq[T](arrayData: ArrayData, dataType: DataType) extends IndexedSeq[T]

kiszk · 2018-04-07T15:52:50Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/ArrayData.scala

+    case _ => (idx: Int) => arrayData.get(idx, dataType)
+  }
+
+  override def apply(idx: Int): T = if (idx < arrayData.numElements()) {


Do we need a check 0 <= idx, too? If so, it would be good to update a message in the exception.

Thanks. Added the check now.

SparkQA · 2018-04-09T15:20:27Z

Test build #89059 has finished for PR 20984 at commit 962d048.

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

kiszk · 2018-04-09T16:21:29Z

retest this please

SparkQA · 2018-04-09T16:48:10Z

Test build #89057 has finished for PR 20984 at commit 4f9752f.

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

SparkQA · 2018-04-09T20:10:08Z

Test build #89068 has finished for PR 20984 at commit 962d048.

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

viirya · 2018-04-10T04:09:53Z

cc @hvanhovell

kiszk · 2018-04-10T09:36:10Z

LGTM

viirya · 2018-04-10T09:46:21Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/ArrayData.scala

+
+  override def apply(idx: Int): T = if (0 <= idx && idx < arrayData.numElements()) {
+    if (arrayData.isNullAt(idx)) {
+      null.asInstanceOf[T]


For primitive ArrayData, this seems to be an issue for null elements, if the caller asks for a primitive type sequence like Seq[Int].

viirya · 2018-04-10T09:47:45Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/ArrayDataIndexedSeqSuite.scala

+    val unsafeArrayData = ExpressionEncoder[Array[String]].resolveAndBind().
+      toRow(stringArray).getArray(0)
+    assert(unsafeArrayData.isInstanceOf[UnsafeArrayData])
+    testArrayData(unsafeArrayData)


This test may need polish to test again all possible types.

+1 for all type test case. :)

viirya · 2018-04-10T09:48:33Z

Thanks @kiszk

hvanhovell · 2018-04-10T09:54:03Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/ArrayData.scala

+
+  private val accessor: (Int) => Any = getAccessor(dataType)
+
+  override def apply(idx: Int): T = if (0 <= idx && idx < arrayData.numElements()) {


NITish: Can you put the if statement on a separate line? This is kinda hard to read.

hvanhovell · 2018-04-10T10:10:27Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/ArrayData.scala

+ */
+class ArrayDataIndexedSeq[T](arrayData: ArrayData, dataType: DataType) extends IndexedSeq[T] {
+
+  private def getAccessor(dataType: DataType): (Int) => Any = dataType match {


Suggestion for a small follow-up: We could also use the accessor you create here to improve the foreach construct.

Ok. I will also want to reuse the accessor getter in #20981 too.

hvanhovell

LGTM - pending jenkins

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

Test build #89105 has finished for PR 20984 at commit a77128f.

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

SparkQA · 2018-04-10T13:16:25Z

Test build #89103 has finished for PR 20984 at commit ac8d5b4.

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

viirya · 2018-04-10T13:31:41Z

retest this please.

SparkQA · 2018-04-10T16:02:28Z

Test build #89122 has finished for PR 20984 at commit a77128f.

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

hvanhovell · 2018-04-10T16:04:50Z

retest this please

SparkQA · 2018-04-10T18:32:30Z

Test build #89139 has finished for PR 20984 at commit a77128f.

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

hvanhovell · 2018-04-10T18:37:34Z

retest this please

SparkQA · 2018-04-10T21:20:46Z

Test build #89152 has finished for PR 20984 at commit a77128f.

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

viirya · 2018-04-10T22:14:19Z

retest this please.

SparkQA · 2018-04-10T22:19:59Z

Test build #89151 has finished for PR 20984 at commit a77128f.

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

SparkQA · 2018-04-11T02:03:58Z

Test build #89158 has finished for PR 20984 at commit a77128f.

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

viirya · 2018-04-12T12:39:18Z

ping @hvanhovell

dongjoon-hyun · 2018-04-12T16:18:09Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/ArrayData.scala

+    case BooleanType => (idx: Int) => arrayData.getBoolean(idx)
+    case ByteType => (idx: Int) => arrayData.getByte(idx)
+    case ShortType => (idx: Int) => arrayData.getShort(idx)
+    case IntegerType => (idx: Int) => arrayData.getInt(idx)


DateType and TimestampType?

I'd like to reuse the access getter in #20981 which covers DateType and TimestampType.

SparkQA · 2018-04-17T06:30:42Z

Test build #89425 has finished for PR 20984 at commit 77640c4.

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

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

Test build #89428 has finished for PR 20984 at commit 6e8a697.

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

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

Test build #89429 has finished for PR 20984 at commit 8b5de0f.

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

viirya · 2018-04-17T07:06:44Z

retest this please.

SparkQA · 2018-04-17T09:10:42Z

Test build #89434 has finished for PR 20984 at commit 8b5de0f.

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

viirya · 2018-04-17T09:12:57Z

retest this please.

SparkQA · 2018-04-17T12:44:22Z

Test build #89443 has finished for PR 20984 at commit 8b5de0f.

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

hvanhovell · 2018-04-17T13:08:56Z

Merging to master. Thanks!

viirya · 2018-04-17T13:19:17Z

Thanks! @hvanhovell. I will create a small follow-up based on the comment at #20984 (comment).

Add IndexedSeq wrapper for ArrayData.

27eddd7

kiszk reviewed Apr 7, 2018

View reviewed changes

Add test.

962d048

viirya force-pushed the SPARK-23875 branch from 4f9752f to 962d048 Compare April 9, 2018 13:28

viirya mentioned this pull request Apr 10, 2018

[SPARK-23873][SQL] Use accessors in interpreted LambdaVariable #20981

Closed

viirya commented Apr 10, 2018

View reviewed changes

Add UDT support.

ac8d5b4

viirya commented Apr 10, 2018

View reviewed changes

hvanhovell reviewed Apr 10, 2018

View reviewed changes

Add comment and fix code style.

a77128f

hvanhovell reviewed Apr 10, 2018

View reviewed changes

hvanhovell approved these changes Apr 10, 2018

View reviewed changes

kiszk mentioned this pull request Apr 11, 2018

[SPARK-23930][SQL] Add slice function #21040

Closed

dongjoon-hyun reviewed Apr 12, 2018

View reviewed changes

viirya added 3 commits April 17, 2018 02:42

Merge remote-tracking branch 'upstream/master' into SPARK-23875

878c995

Reuse accessor getter.

77640c4

Add tests for other types.

8b5de0f

viirya force-pushed the SPARK-23875 branch from 6e8a697 to 8b5de0f Compare April 17, 2018 05:27

asfgit closed this in 30ffb53 Apr 17, 2018

viirya deleted the SPARK-23875 branch December 27, 2023 18:35


		private val accessor: (Int) => Any = getAccessor(dataType)

		override def apply(idx: Int): T = if (0 <= idx && idx < arrayData.numElements()) {

[SPARK-23875][SQL] Add IndexedSeq wrapper for ArrayData #20984

[SPARK-23875][SQL] Add IndexedSeq wrapper for ArrayData #20984

Uh oh!

Conversation

viirya commented Apr 5, 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 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 9, 2018

Uh oh!

kiszk commented Apr 9, 2018

Uh oh!

SparkQA commented Apr 9, 2018

Uh oh!

SparkQA commented Apr 9, 2018

Uh oh!

viirya commented Apr 10, 2018

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

viirya commented Apr 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

hvanhovell left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 10, 2018

Uh oh!

SparkQA commented Apr 10, 2018

Uh oh!

viirya commented Apr 10, 2018

Uh oh!

SparkQA commented Apr 10, 2018

Uh oh!

hvanhovell commented Apr 10, 2018

Uh oh!

SparkQA commented Apr 10, 2018

Uh oh!

hvanhovell commented Apr 10, 2018

Uh oh!

SparkQA commented Apr 10, 2018

Uh oh!

viirya commented Apr 10, 2018

Uh oh!

SparkQA commented Apr 10, 2018

Uh oh!

SparkQA commented Apr 11, 2018

Uh oh!

viirya commented Apr 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 17, 2018

Uh oh!

viirya commented Apr 5, 2018 •

edited

Loading

viirya Apr 10, 2018 •

edited

Loading

hvanhovell Apr 10, 2018 •

edited

Loading