[SPARK-23922][SQL] Add arrays_overlap function #21028

mgaido91 · 2018-04-10T15:06:21Z

What changes were proposed in this pull request?

The PR adds the function arrays_overlap. This function returns true if the input arrays contain a non-null common element; if not, it returns null if any of the arrays contains a null element, false otherwise.

How was this patch tested?

added UTs

SparkQA · 2018-04-10T15:10:25Z

Test build #89130 has finished for PR 21028 at commit e5ebdad.

This patch fails Python style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ArraysOverlap(left: Expression, right: Expression)

gatorsmile · 2018-04-10T15:34:29Z

cc @ueshin

SparkQA · 2018-04-10T18:54:45Z

Test build #89132 has finished for PR 21028 at commit 682bc73.

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

SparkQA · 2018-04-17T14:45:06Z

Test build #89450 has finished for PR 21028 at commit 876cd93.

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

ueshin · 2018-04-19T09:10:43Z

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

+ * Checks if the two arrays contain at least one common element.
+ */
+@ExpressionDescription(
+  usage = "_FUNC_(a1, a2) - Returns true if a1 contains at least an element present also in a2.",


Can you add a note for null handling?

ueshin · 2018-04-19T09:14:43Z

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

+
+  override def dataType: DataType = BooleanType
+
+  override def inputTypes: Seq[AbstractDataType] = left.dataType match {


Seq(ArrayType, ArrayType)?

no, because in that way we would loose the information about the elementType

There are similar functions, such as array_union(#21061), array_intersect(#21102), array_except(#21103), and maybe concat(#20858) which is slightly different though, to handle two (or more) arrays with the same element type.
I think we should use the same way to specify and check input types.

I'd like to discuss the best way for it here or somewhere else.
cc @kiszk @mn-mikke Do you have any suggestions?
Also cc @gatorsmile @cloud-fan

If common functions for a method, which accepts two array with the same type, are provided as trait, it would be fine.

We can create a new trait, which will first make sure all its children are array type, and then make sure all its children are same type after implicit type cast(make sure other databases also do implicit type cast for these functions).

Then update TypeCoercion rule to handle this trait.

yea if implicit type cast is not allowed for these functions.

Just a quick note, there is a dedicated type coercion rule for concat functions. So if there was the trait you described, we could remove the rule.

@mn-mikke I am not sure, since it is quite a strange case, since it allows also string and byte. I am not sure we can do this with implicit type cast.

@mgaido91 Sorry, I should have been more explicit. I've been referring to the below case that I added into FunctionArgumentConversion due to enabling type coercion of array types.

case c @ Concat(children) if children.forall(c => ArrayType.acceptsType(c.dataType)) && !haveSameType(children) => val types = children.map(_.dataType) findWiderCommonType(types) match { case Some(finalDataType) => Concat(children.map(Cast(_, finalDataType))) case None => c }

implicit type cast is allowed in Presto. I am pushing here a proposal of trait, let me know what you think about it. Thanks.

ueshin · 2018-04-19T09:28:40Z

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

+    checkEvaluation(ArraysOverlap(a5, a6), true)
+    checkEvaluation(ArraysOverlap(a5, a7), null)
+    checkEvaluation(ArraysOverlap(a6, a7), false)
+  }


Can you add cases for one of the two arguments is null and ArraysOverlap(Seq(null), Seq(null))?

ueshin · 2018-04-19T09:35:47Z

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

+          )
+        }
+      )
+    } else {


We can skip if the right is containsNull == false?

ueshin · 2018-04-19T09:38:53Z

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

+         |      }
+         |    }
+         |  }
+         |} else {


SparkQA · 2018-04-20T19:37:02Z

Test build #89639 has finished for PR 21028 at commit c895707.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class Summarizer(object):
class SummaryBuilder(JavaWrapper):
case class Reverse(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
case class ArrayPosition(left: Expression, right: Expression)
case class ElementAt(left: Expression, right: Expression) extends GetMapValueUtil
case class Concat(children: Seq[Expression]) extends Expression
abstract class GetMapValueUtil extends BinaryExpression with ImplicitCastInputTypes
case class GetMapValue(child: Expression, key: Expression)
class ArrayDataIndexedSeq[T](arrayData: ArrayData, dataType: DataType) extends IndexedSeq[T]

mgaido91 · 2018-04-21T08:07:41Z

anymore comments @ueshin ?

SparkQA · 2018-04-27T15:35:04Z

Test build #89926 has finished for PR 21028 at commit 1dbcd0c.

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

mn-mikke · 2018-04-27T15:54:59Z

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

+ * Base trait for [[BinaryExpression]]s with two arrays of the same element type and implicit
+ * casting.
+ */
+trait BinaryArrayExpressionWithImplicitCast extends BinaryExpression


The ImplicitCastInputTypes trait is able to work with any number of children. Would it be possible to implement this trait to behave in the same way?

It's possible indeed. Though, as far as I know there is no use case for a function with a different number of children, so I am not sure if it makes sense to generalize it. @cloud-fan @kiszk @ueshin WDYT?

As @ueshin pointed out here, concat is also a use case that has a different number of children. Am I wrong?

@kiszk you are not wrong, but Concat is a very specific case, since it supports also Strings and Binarys, so it would anyway require a specific implementation.

I see, I would like to hear other opinions

SparkQA · 2018-04-27T16:04:00Z

Test build #89930 has finished for PR 21028 at commit 076fc69.

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

SparkQA · 2018-04-27T16:48:39Z

Test build #89932 has finished for PR 21028 at commit eafca0f.

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

SparkQA · 2018-04-27T20:46:02Z

Test build #89935 has finished for PR 21028 at commit 5925104.

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

kiszk · 2018-04-28T02:06:29Z

retest this please

SparkQA · 2018-04-28T05:49:13Z

Test build #89949 has finished for PR 21028 at commit 5925104.

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

mn-mikke · 2018-05-02T17:35:31Z

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

+       true
+  """, since = "2.4.0")
+// scalastyle:off line.size.limit
+case class ArraysOverlap(left: Expression, right: Expression)


Shouldn't you override prettyName to a value following the conventions?

override def prettyName: String = "arrays_overlap"

done, thanks!

cloud-fan · 2018-05-03T12:28:11Z

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

+  }
+
+  override def checkInputDataTypes(): TypeCheckResult = {
+    TypeCoercion.findWiderTypeForTwo(left.dataType, right.dataType) match {


We should not call findWiderTypeForTwo here. Instead we should directly check if the 2 inputs are array type and have the same element type, so that it's safe to say this expression is resolved.

with your suggestion, if we have two arrays with different element types (but one can be casted to the other), we would throw an exception. Instead, with this approach that use case is valid and we perform an implicit cast to the wider type.

we would throw an exception

No, the type coercion rule will be run again and again until this expression is resolved, or hit fixed point and fail.

sorry, my bad, I got confused. I am fixing this, thanks.

SparkQA · 2018-05-03T14:04:29Z

Test build #90117 has finished for PR 21028 at commit 2a1121c.

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

SparkQA · 2018-05-11T13:28:47Z

Test build #90510 has finished for PR 21028 at commit 3dd724b.

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

SparkQA · 2018-05-11T20:22:27Z

Test build #90518 has finished for PR 21028 at commit e36a5d7.

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

cloud-fan · 2018-05-14T12:18:56Z

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

+   */
+  private def fastEval(arr1: ArrayData, arr2: ArrayData): Any = {
+    var hasNull = false
+    val (bigger, smaller, biggerDt) = if (arr1.numElements() > arr2.numElements()) {


the biggerDt is not used

cloud-fan · 2018-05-14T12:22:30Z

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

+      index: String,
+      code: String,
+      isNullCode: String): String = {
+    if (inputTypes.exists(_.asInstanceOf[ArrayType].containsNull)) {


shouldn't this depend on whether the input array arrayVar contains null?

unfortunately we don't know which one we have here (the left or the rigth) as arrayVar, since we don't know which one is the smaller/bigger and this can change record to record. So we can skip the null check only if both them don't contain null.

i see, makes sense!

cloud-fan · 2018-05-14T12:28:57Z

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

+      s"${ev.isNull} = true;")
+    s"""
+       |for (int $i = 0; $i < $bigger.numElements(); $i ++) {
+       |$isInSmaller


cloud-fan · 2018-05-14T12:30:04Z

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

+      s"""
+         |for (int $j = 0; $j < $smaller.numElements(); $j ++) {
+         |  $compareValues
+         |  if (${ev.value}) {


do we need this if?

it was in the wrong place, thanks, nice catch!

cloud-fan · 2018-05-14T15:13:06Z

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

+         |}
+         |if ($smaller.numElements() > 0) {
+         |  $comparisonCode
+         |}


if smaller is empty, we should return false. However, the code here depends on the initial value of ev.value and ev.isNull, which, according to nullSafeCodeGen, depends on nullable.

Yes, but ev.isNull anyway is it initiated to false, unless one of the input is null. And in that case we don't even reach this point because we just return null.

ev.value is initiated always to the defaultValue which is false. So when we arrive here we are sure that they are both false.

ah i see, sorry I misread the code in nullSafeCodeGen

np, thanks for checking it. Always better one check more than one less.

cloud-fan · 2018-05-14T15:19:42Z

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

+         |if (${ctx.genEqual(elementType, getFromSmaller, getFromBigger)}) {
+         |  ${ev.isNull} = false;
+         |  ${ev.value} = true;
+         |  break;


so we wanna break 2 loops here, that's why we generate 2 breaks. it might be cleaner to generate

for (int $i = 0; $i < $bigger.numElements() && !${ev.value}; $i ++) { for (int $j = 0; $j < $smaller.numElements() && !${ev.value}; $j ++) { ... } }

SparkQA · 2018-05-14T17:37:22Z

Test build #90592 has finished for PR 21028 at commit 49d9372.

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

SparkQA · 2018-05-15T12:11:26Z

Test build #90635 has finished for PR 21028 at commit 227437b.

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

ueshin · 2018-05-16T10:05:43Z

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

+   */
+  private def bruteForceEval(arr1: ArrayData, arr2: ArrayData): Any = {
+    var hasNull = false
+    if (arr1.numElements() > 0) {


We should also compare the numElements here and check the array is empty for the smaller one? Otherwise the result is different if the arr1 is not empty and contains null and arr2 is empty?

thanks, I added also some test cases for this

SparkQA · 2018-05-16T14:58:52Z

Test build #90679 has finished for PR 21028 at commit 2e9e024.

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

cloud-fan · 2018-05-17T05:02:09Z

retest this please

cloud-fan · 2018-05-17T05:09:35Z

LGTM

SparkQA · 2018-05-17T05:09:50Z

Test build #90711 has finished for PR 21028 at commit 2e9e024.

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

cloud-fan · 2018-05-17T05:10:10Z

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

+  override def inputTypes: Seq[AbstractDataType] = {
+    (left.dataType, right.dataType) match {
+      case (ArrayType(e1, hasNull1), ArrayType(e2, hasNull2)) =>
+        TypeCoercion.findTightestCommonType(e1, e2, caseSensitive) match {


this should fail the build now

SparkQA · 2018-05-17T11:55:35Z

Test build #90719 has finished for PR 21028 at commit 56c59ae.

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

cloud-fan · 2018-05-17T12:45:53Z

thanks, merging to master!

… collection expressions. ## What changes were proposed in this pull request? The PR tries to avoid serialization of private fields of already added collection functions and follows up on comments in [SPARK-23922](#21028) and [SPARK-23935](#21236) ## How was this patch tested? Run tests from: - CollectionExpressionSuite.scala - DataFrameFunctionsSuite.scala Author: Marek Novotny <[email protected]> Closes #21352 from mn-mikke/SPARK-24305.

[SPARK-23922][SQL] Add arrays_overlap function

e5ebdad

fix python style

682bc73

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

876cd93

ueshin reviewed Apr 19, 2018

View reviewed changes

mgaido91 added 2 commits April 20, 2018 17:38

review comments

88e09b3

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

c895707

mgaido91 added 3 commits April 27, 2018 16:36

introduce BinaryArrayExpressionWithImplicitCast

65b7d6d

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

f9a1ecf

fix type check

1dbcd0c

fix scalastyle

076fc69

mn-mikke reviewed Apr 27, 2018

View reviewed changes

fix build error

eafca0f

fix

5925104

mn-mikke reviewed May 2, 2018

View reviewed changes

address comments

2a1121c

cloud-fan reviewed May 3, 2018

View reviewed changes

use sets instead of nested loops

bf81e4a

review comments

3dd724b

mgaido91 added 2 commits May 11, 2018 18:23

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

f7089f5

fix compilation error

e36a5d7

cloud-fan reviewed May 14, 2018

View reviewed changes

address comments

49d9372

cloud-fan reviewed May 14, 2018

View reviewed changes

address comment

227437b

ueshin reviewed May 16, 2018

View reviewed changes

fix null handling with complex types

2e9e024

cloud-fan reviewed May 17, 2018

View reviewed changes

mgaido91 added 2 commits May 17, 2018 10:21

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

92730a1

fix build

56c59ae

mn-mikke mentioned this pull request May 17, 2018

[SPARK-24305][SQL][FOLLOWUP] Avoid serialization of private fields in collection expressions. #21352

Closed

asfgit closed this in 69350aa May 17, 2018


		override def dataType: DataType = BooleanType

		override def inputTypes: Seq[AbstractDataType] = left.dataType match {

[SPARK-23922][SQL] Add arrays_overlap function #21028

[SPARK-23922][SQL] Add arrays_overlap function #21028

Conversation

mgaido91 commented Apr 10, 2018

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Apr 10, 2018

gatorsmile commented Apr 10, 2018

SparkQA commented Apr 10, 2018

SparkQA commented Apr 17, 2018

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

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

mgaido91 commented Apr 21, 2018

SparkQA commented Apr 27, 2018

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 27, 2018

SparkQA commented Apr 27, 2018

SparkQA commented Apr 27, 2018

kiszk commented Apr 28, 2018

SparkQA commented Apr 28, 2018

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 May 3, 2018

SparkQA commented May 11, 2018

SparkQA commented May 11, 2018

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

cloud-fan May 14, 2018 • edited Loading

Choose a reason for hiding this comment

mgaido91 May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgaido91 May 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented May 14, 2018

SparkQA commented May 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented May 16, 2018

cloud-fan commented May 17, 2018

cloud-fan commented May 17, 2018

SparkQA commented May 17, 2018

Choose a reason for hiding this comment

SparkQA commented May 17, 2018

cloud-fan commented May 17, 2018

cloud-fan May 14, 2018 •

edited

Loading

mgaido91 May 14, 2018 •

edited

Loading

mgaido91 May 15, 2018 •

edited

Loading