[SPARK-24313][SQL] Fix collection operations' interpreted evaluation for complex types #21361

mgaido91 · 2018-05-18T09:20:54Z

What changes were proposed in this pull request?

The interpreted evaluation of several collection operations works only for simple datatypes. For complex data types, for instance, array_contains it returns always false. The list of the affected functions is array_contains, array_position, element_at and GetMapValue.

The PR fixes the behavior for all the datatypes.

How was this patch tested?

added UT

…lex types

mgaido91 · 2018-05-18T09:21:26Z

cc @cloud-fan @kiszk @ueshin

viirya · 2018-05-18T09:33:34Z

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

  override def dataType: DataType = BooleanType

+  @transient private lazy val ordering: Ordering[Any] =
+    TypeUtils.getInterpretedOrdering(right.dataType)


Then in checkInputDataTypes we should check if there is Ordering for right.dataType. Otherwise for example MapType will throw a match error.

yes, thanks

viirya · 2018-05-18T09:34:11Z

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

      if (v == null) {
        hasNull = true
-      } else if (v == value) {
+      } else if (ordering.equiv(v, value)) {


Previously does this work for Map? No?

MapType is not supported in comparison, even =

ueshin · 2018-05-18T09:50:19Z

ArrayPosition has the same problem? cc @kiszk

kiszk · 2018-05-18T09:55:01Z

Great catch, thanks.
@ueshin, ~~I will work for ArrayPosition~~.

mgaido91 · 2018-05-18T09:55:53Z

yes @ueshin , I can fix ArrayPosition in this PR if you want

kiszk · 2018-05-18T10:11:15Z

@mgaido91 Thank you very much. Can I ask you to fix ArrayPosition in this PR, too?

mgaido91 · 2018-05-18T10:14:27Z

sure, thanks @kiszk. Sorry, I saw your comment only now, probably we were writing at the same time :)

ueshin · 2018-05-18T10:20:29Z

We should also fix GetMapValueUtil.getValueEval()?

mgaido91 · 2018-05-18T11:06:58Z

yes @ueshin , great catch! I am fixing it too.

SparkQA · 2018-05-18T13:01:17Z

Test build #90785 has finished for PR 21361 at commit 35370cd.

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

cloud-fan · 2018-05-18T13:08:13Z

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

        "Arguments must be an array followed by a value of same type as the array members")
    } else {
-      TypeCheckResult.TypeCheckSuccess
+      if (RowOrdering.isOrderable(right.dataType)) {


we can call TypeUtils.checkForOrderingExpr

cloud-fan · 2018-05-18T13:09:28Z

good catch! this is a long-standing bug...

SparkQA · 2018-05-18T14:55:15Z

Test build #90788 has finished for PR 21361 at commit b7d7249.

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

SparkQA · 2018-05-18T15:46:24Z

Test build #90790 has finished for PR 21361 at commit a819a97.

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

SparkQA · 2018-05-18T17:06:46Z

Test build #90794 has finished for PR 21361 at commit 71916d9.

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

cloud-fan · 2018-05-19T08:44:45Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/complexTypesSuite.scala

+  test("SPARK-24313: support complex types as map keys") {
+    val mb0 = Literal.create(
+      Map(Array[Byte](1, 2) -> "1", Array[Byte](3, 4) -> null, Array[Byte](2, 1) -> "2"),
+      MapType(BinaryType, StringType))


shall we test ArrayTypeto reflect the test name?

I changed the test name here and the comment for ElementAt because I found an issue in using arrays as keys for map. When I do that, I get:

Caused by: java.lang.ClassCastException: scala.collection.immutable.$colon$colon cannot be cast to org.apache.spark.sql.catalyst.util.ArrayData

I will investigate this further, but I think this is a separate issue and should be addressed in another PR, what do you think?

cloud-fan · 2018-05-19T08:44:54Z

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

+    // test complex types as keys
+    val mb0 = Literal.create(
+      Map(Array[Byte](1, 2) -> "1", Array[Byte](3, 4) -> null, Array[Byte](2, 1) -> "2"),
+      MapType(BinaryType, StringType))


cloud-fan · 2018-05-19T08:45:16Z

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

+      ArrayType(BinaryType))
+    val b3 = Literal.create(Seq[Array[Byte]](null, Array[Byte](1, 2)),
+      ArrayType(BinaryType))
+    val be = Literal.create(Array[Byte](1, 2), BinaryType)


ditto, binary type is not complex type

SparkQA · 2018-05-20T15:02:31Z

Test build #90857 has finished for PR 21361 at commit c74ddd7.

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

kiszk · 2018-05-20T15:21:15Z

retest this please

SparkQA · 2018-05-20T19:07:58Z

Test build #90863 has finished for PR 21361 at commit c74ddd7.

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

cloud-fan · 2018-05-21T08:29:46Z

LGTM, can we add an end-to-end test case for GetMapValue? This bug can be triggered with constant folding.

mgaido91 · 2018-05-21T08:35:20Z

@cloud-fan sorry but I am not sure I got it. May you please provide me some more details about the end-to-end test case for GetMapValue you want me to add? Thanks.

cloud-fan · 2018-05-21T09:14:15Z

e.g.

val binaryLiteral = lit(Array[Byte](1.toByte))
spark.range(1).select(map(binaryLiteral, lit(1)).getItem(Array[Byte](1.toByte))).show
+--------------------+
|map(X'01', 1)[X'01']|
+--------------------+
|                null|
+--------------------+

we should use checkAnswer in the test instead of show

mgaido91 · 2018-05-21T13:56:22Z

thank you for your kind explanation @cloud-fan. I added the UT you suggested. Thanks.

cloud-fan · 2018-05-21T14:04:43Z

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

    assert(complexData.filter(complexData("m").getItem("1") === 1).count() == 1)
    assert(complexData.filter(complexData("s").getField("key") === 1).count() == 1)
+
+    // SPARK-24313: access binary keys


can we create a new test case? I think a correctness bug deserves an individual end-to-end test case.

SparkQA · 2018-05-21T17:38:37Z

Test build #90900 has finished for PR 21361 at commit 3f8624b.

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

SparkQA · 2018-05-21T18:07:57Z

Test build #90903 has finished for PR 21361 at commit 6315775.

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

cloud-fan · 2018-05-22T13:09:21Z

thanks, merging to master!

@mgaido91 can you send a new PR for 2.3? thanks!

mgaido91 · 2018-05-22T14:48:40Z

sure @cloud-fan, thanks. I will create it in the next days. Thank you.

…ed evaluation for complex types The interpreted evaluation of several collection operations works only for simple datatypes. For complex data types, for instance, `array_contains` it returns always `false`. The list of the affected functions is `array_contains`, `array_position`, `element_at` and `GetMapValue`. The PR fixes the behavior for all the datatypes. added UT Author: Marco Gaido <[email protected]> Closes apache#21361 from mgaido91/SPARK-24313.

gatorsmile · 2018-05-27T05:23:59Z

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

-      } else {
-        TypeCheckResult.TypeCheckFailure(s"${elementType.simpleString} cannot be used in comparison.")
-      }
+      TypeUtils.checkForOrderingExpr(elementType, s"function $prettyName")


Also a general suggestion. For these refactoring, we should do it in a separate PR.

ok, I'll keep this in mind for the future, thanks.

gatorsmile · 2018-05-27T05:28:59Z

Thanks for fixing this!

[SPARK-24313][SQL] Fix array_contains interpreted evaluation for comp…

35370cd

…lex types

viirya reviewed May 18, 2018

View reviewed changes

ueshin mentioned this pull request May 18, 2018

[SPARK-23920][SQL]add array_remove to remove all elements that equal element from array #21069

Closed

add orderable check

8c9553f

mgaido91 changed the title ~~[SPARK-24313][SQL] Fix array_contains interpreted evaluation for complex types~~ [SPARK-24313][SQL] Fix array_contains/array_position interpreted evaluation for complex types May 18, 2018

fix array position

b7d7249

mgaido91 changed the title ~~[SPARK-24313][SQL] Fix array_contains/array_position interpreted evaluation for complex types~~ [SPARK-24313][SQL] Fix collection operations' interpreted evaluation for complex types May 18, 2018

fix element_at and GetValueMap

a819a97

cloud-fan reviewed May 18, 2018

View reviewed changes

address comments

71916d9

kiszk mentioned this pull request May 18, 2018

[SPARK-23912][SQL]add array_distinct #21050

Closed

cloud-fan reviewed May 19, 2018

View reviewed changes

address comments

c74ddd7

add end to end test

3f8624b

cloud-fan reviewed May 21, 2018

View reviewed changes

address comment

6315775

asfgit closed this in d3d1807 May 22, 2018

gatorsmile reviewed May 27, 2018

View reviewed changes

[SPARK-24313][SQL] Fix collection operations' interpreted evaluation for complex types #21361

[SPARK-24313][SQL] Fix collection operations' interpreted evaluation for complex types #21361

Uh oh!

Conversation

mgaido91 commented May 18, 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!

mgaido91 commented May 18, 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!

ueshin commented May 18, 2018

Uh oh!

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

Uh oh!

Uh oh!

mgaido91 commented May 18, 2018

Uh oh!

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

Uh oh!

Uh oh!

mgaido91 commented May 18, 2018

Uh oh!

ueshin commented May 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgaido91 commented May 18, 2018

Uh oh!

SparkQA commented May 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented May 18, 2018

Uh oh!

SparkQA commented May 18, 2018

Uh oh!

SparkQA commented May 18, 2018

Uh oh!

SparkQA commented May 18, 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!

SparkQA commented May 20, 2018

Uh oh!

kiszk commented May 20, 2018

Uh oh!

SparkQA commented May 20, 2018

Uh oh!

cloud-fan commented May 21, 2018

Uh oh!

mgaido91 commented May 21, 2018

Uh oh!

cloud-fan commented May 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgaido91 commented May 21, 2018

Uh oh!

cloud-fan May 21, 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 May 21, 2018

Uh oh!

SparkQA commented May 21, 2018

Uh oh!

cloud-fan commented May 22, 2018

mgaido91 commented May 18, 2018 •

edited

Loading

kiszk commented May 18, 2018 •

edited

Loading

kiszk commented May 18, 2018 •

edited

Loading

ueshin commented May 18, 2018 •

edited

Loading

cloud-fan commented May 21, 2018 •

edited

Loading

cloud-fan May 21, 2018 •

edited

Loading