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

mgaido91 · 2018-04-11T12:28:01Z

What changes were proposed in this pull request?

The PR add the slice function. The behavior of the function is based on Presto's one.

The function slices an array according to the requested start index and length.

How was this patch tested?

added UTs

kiszk · 2018-04-11T14:17:58Z

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

+         |} else {
+         |  $values = new Object[$resLength];
+         |  for (int $i = 0; $i < $resLength; $i ++) {
+         |    $values[$i] = ${CodeGenerator.getValue(x, elementType, s"$i + $startIdx")};


May this assignment cause performance degradation due to boxing if array element type is primitive (e.g. float)?

I though about that too, but I am not sure there is a better solution: this approach is used both in CreateArray and GenerateSafeProjection. And there is a TODO for specialized versions of GenericArrayData able to deal with primitive types without boxing.

Probably we can try and fix this TODO in another PR/JIRA. What do you think?

I see. If we postpone specialization, is it necessary to generate Java code for now? The generated code seems to do the same thing in nullSafeEval. WDYT?

I think it can be helpful: they are not really doing the same thing anyway. Moreover, this is the way also CreateArray and GenerateSafeProjection work, so for coherency I think this is the right thing to do. What do you think?

For the future, I agree that this is the right way to generate Java code since we can avoid boxing.

On the other hand, you are proposing to postpone specialization. In eval and generated code, GenericArrayData is generated by using Object[].
I may misunderstand for coherency since I may not find the target of the coherency in the thread.

My target of coherency was the CreateArray operator and the code generated in GenerateSafeProjection.

I might miss something, but seems like CreateArray is using different ways to codegen for primitive arrays and the others, and I guess GenerateSafeProjection is using Object[] on purpose to create GenericArrayData to be "safe" (avoid using UnsafeXxx).
I think we should modify this codegen to avoid boxing. WDYT?

Btw, we need to null check here for an array of primitive type contains null anyway?

You are right, I am not sure why I missed it...maybe I checked outdated code. Sorry, I am fixing it, thanks.

SparkQA · 2018-04-11T16:41:38Z

Test build #89194 has finished for PR 21040 at commit 5cbbf7a.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Slice(x: Expression, start: Expression, length: Expression)

kiszk · 2018-04-11T17:24:17Z

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

+      return new GenericArrayData(Array.empty[AnyRef])
+    }
+    val elementType = x.dataType.asInstanceOf[ArrayType].elementType
+    val data = arr.toArray[AnyRef](elementType)


This PR #20984 can make slice better.

shall we wait for that PR to get in?

I think it would be good since we can avoid the whole array copy if that PR will be merged near future.
@viirya What do you think?

I think #20984 should be merged soon.

gatorsmile · 2018-04-13T06:58:50Z

cc @ueshin

ueshin · 2018-04-19T10:25:22Z

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

+ */
+// scalastyle:off line.size.limit
+@ExpressionDescription(
+  usage = "_FUNC_(a1, a2) - Subsets array x starting from index start (or starting from the end if start is negative) with the specified length.",


_FUNC_(x, start, length) instead of _FUNC_(a1, a2)?

ueshin · 2018-04-19T10:26:46Z

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

+
+  override def nullable: Boolean = children.exists(_.nullable)
+
+  override def foldable: Boolean = children.forall(_.foldable)


We don't need nullable and foldable here because these are the same as defined in TernaryExpression.

ueshin · 2018-04-19T10:28:07Z

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

+    }
+    if (lengthInt < 0) {
+      throw new RuntimeException(s"Unexpected value for length in function $prettyName: " +
+        s"length must be greater than or equal to 0.")


nit: unnecessary s.

ueshin · 2018-04-19T10:32:53Z

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

+    }
+    // this can happen if start is negative and its absolute value is greater than the
+    // number of elements in the array
+    if (startIndex < 0) {


We should also skip when startIndex >= arr.numElements() to avoid unnecessary convert arr.toArray?

ueshin · 2018-04-19T10:35:42Z

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

+    val arr = xVal.asInstanceOf[ArrayData]
+    val startIndex = if (startInt == 0) {
+      throw new RuntimeException(
+        s"Unexpected value for start in function $prettyName:  SQL array indices start at 1.")


nit: remove an extra space between $prettyName: and SQL.

ueshin · 2018-04-19T10:44:22Z

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

+    checkEvaluation(Slice(a0, Literal.create(null, IntegerType), Literal(2)), null)
+    checkEvaluation(Slice(a0, Literal(2), Literal.create(null, IntegerType)), null)
+    checkEvaluation(Slice(Literal.create(null, ArrayType(IntegerType)), Literal(1), Literal(2)),
+      null)


Can you add a case for something like Slice(a0, Literal(10), Literal(1))?

And also can you add a case for nullable primitive array like Slice(Seq(1, 2, null, 4), 2, 3)?

ueshin · 2018-04-19T10:52:19Z

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala

  }

+  protected def checkExceptionInExpression[T <: Throwable : ClassTag](
+      expression: Expression,


expression: => Expression to be consistent with the overloaded one, just in case?

SparkQA · 2018-04-20T18:20:05Z

Test build #89640 has finished for PR 21040 at commit b94d067.

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

kiszk · 2018-04-20T19:17:48Z

retest this please

SparkQA · 2018-04-20T23:13:17Z

Test build #89657 has finished for PR 21040 at commit b94d067.

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

mgaido91 · 2018-04-21T08:06:52Z

any more comments @kiszk @ueshin ?

ueshin

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

ueshin · 2018-04-26T02:40:32Z

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

+         |} else {
+         |  $values = new Object[$resLength];
+         |  for (int $i = 0; $i < $resLength; $i ++) {
+         |    $values[$i] = ${CodeGenerator.getValue(x, elementType, s"$i + $startIdx")};


I might miss something, but seems like CreateArray is using different ways to codegen for primitive arrays and the others, and I guess GenerateSafeProjection is using Object[] on purpose to create GenericArrayData to be "safe" (avoid using UnsafeXxx).
I think we should modify this codegen to avoid boxing. WDYT?

Btw, we need to null check here for an array of primitive type contains null anyway?

ueshin · 2018-04-26T02:40:36Z

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

+    checkEvaluation(Slice(a0, Literal.create(null, IntegerType), Literal(2)), null)
+    checkEvaluation(Slice(a0, Literal(2), Literal.create(null, IntegerType)), null)
+    checkEvaluation(Slice(Literal.create(null, ArrayType(IntegerType)), Literal(1), Literal(2)),
+      null)


And also can you add a case for nullable primitive array like Slice(Seq(1, 2, null, 4), 2, 3)?

SparkQA · 2018-04-27T17:05:10Z

Test build #89923 has finished for PR 21040 at commit 72ed607.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ArrayJoin(
case class Flatten(child: Expression) extends UnaryExpression
case class MonthsBetween(
trait QueryPlanConstraints extends ConstraintHelper
trait ConstraintHelper
case class CachedRDDBuilder(
case class InMemoryRelation(
case class WriteToContinuousDataSource(
case class WriteToContinuousDataSourceExec(writer: StreamWriter, query: SparkPlan)

ueshin

LGTM except for a nit.

ueshin · 2018-04-30T05:45:12Z

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

+                        ev: ExprCode,
+                        inputArray: String,
+                        startIdx: String,
+                        resLength: String): String = {


nit: indent

SparkQA · 2018-04-30T11:39:32Z

Test build #89977 has finished for PR 21040 at commit 9d65570.

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

mgaido91 · 2018-04-30T14:05:42Z

retest this please

HyukjinKwon

LGTM too

ueshin · 2018-05-01T03:10:49Z

Jenkins, retest this please.

kiszk · 2018-05-01T07:21:24Z

retest this please

ueshin · 2018-05-01T17:36:59Z

Jenkins, retest this please.

SparkQA · 2018-05-01T21:17:36Z

Test build #89989 has finished for PR 21040 at commit 9d65570.

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

mgaido91 · 2018-05-01T21:19:58Z

retest this please

SparkQA · 2018-05-01T23:20:50Z

Test build #90005 has finished for PR 21040 at commit 9d65570.

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

kiszk · 2018-05-04T14:19:53Z

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

+         |  UnsafeArrayData.calculateHeaderPortionInBytes($resLength) +
+         |  ${classOf[ByteArrayMethods].getName}.roundNumberOfBytesToNearestWord(
+         |    ${elementType.defaultSize} * $resLength);
+         |byte[] $bytesArray = new byte[$sizeInBytes];


What happens if sizeInBytes is larger than Integer.MAX_VALUE? For example, 0x7000_0000 long elements. In this case, GenericArrayData or long[] can hold these elements. WDYT?

In other places (eg Concat) in such a case we just throw a runtime exception. What about following the same pattern here?

I am not even sure we have to add such a check actually, since here we can only reduce the size of an already existing array... Anyway probably it is ok to add an additional sanity check. WDYT?

I am curious about the following two cases.

In UnsafeArray, long[] may be used. Its size is 0x8000_0000 * 4. On the other hand, the size is the allocated byte[] is up to 0x8000_0000.

If GenericArray, which includes a lot of (e.g. 0x7F00_0000) Long or Double elements, is passed to this operation, the expected allocation size is more than 0x8000_0000.

While these cases reduce the size of an existing array, does the result array fit into byte[]? WDYT?

I added the same check which is performed in Concat and Flatten. If we want to support also larger arrays of primitives, we probably best have another PR which address the issue on all the functions affected (this one, Concat and Flatten), especially considering that the issue is much more likely to happen in the other two cases. Do you agree?

SparkQA · 2018-05-04T16:04:47Z

Test build #90202 has finished for PR 21040 at commit e2eb21e.

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

SparkQA · 2018-05-04T16:26:12Z

Test build #90196 has finished for PR 21040 at commit 9f0deec.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class ImplicitTypeCasts(conf: SQLConf) extends TypeCoercionRule
case class StringToTimestampWithoutTimezone(child: Expression, timeZoneId: Option[String] = None)

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

Test build #90203 has finished for PR 21040 at commit 07604e0.

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

ueshin · 2018-05-07T07:57:06Z

Thanks! merging to master.

[SPARK-23930][SQL] Add slice function

5cbbf7a

kiszk reviewed Apr 11, 2018

View reviewed changes

ueshin reviewed Apr 19, 2018

View reviewed changes

mgaido91 added 3 commits April 20, 2018 17:52

fix typo

367aaf2

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

f2784f1

review comments

b94d067

ueshin reviewed Apr 26, 2018

View reviewed changes

mgaido91 added 2 commits April 27, 2018 14:53

specialize codegen for primitive types

dc6cb60

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

72ed607

ueshin reviewed Apr 30, 2018

View reviewed changes

fix indent

9d65570

HyukjinKwon approved these changes May 1, 2018

View reviewed changes

kiszk mentioned this pull request May 4, 2018

[SPARK-23921][SQL] Add array_sort function #21021

Closed

Merge branch 'master' into SPARK-23930

9f0deec

kiszk reviewed May 4, 2018

View reviewed changes

add checks for size greater than maxint

e2eb21e

fix scalastyle

07604e0

asfgit closed this in e35ad3c May 7, 2018


		override def nullable: Boolean = children.exists(_.nullable)

		override def foldable: Boolean = children.forall(_.foldable)

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

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

Uh oh!

Conversation

mgaido91 commented Apr 11, 2018

What changes were proposed in this pull request?

How was this patch tested?

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!

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

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 11, 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!

gatorsmile 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 20, 2018

Uh oh!

kiszk commented Apr 20, 2018

Uh oh!

SparkQA commented Apr 20, 2018

Uh oh!

mgaido91 commented Apr 21, 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!

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

SparkQA commented Apr 30, 2018

Uh oh!

mgaido91 Apr 12, 2018 •

edited

Loading