[SPARK-18519][SQL] map type can not be used in EqualTo #15956

cloud-fan · 2016-11-21T06:28:01Z

What changes were proposed in this pull request?

Technically map type is not orderable, but can be used in equality comparison. However, due to the limitation of the current implementation, map type can't be used in equality comparison so that it can't be join key or grouping key.

This PR makes this limitation explicit, to avoid wrong result.

How was this patch tested?

updated tests.

cloud-fan · 2016-11-21T06:28:57Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

This doesn't make sense, BinaryType is comparable, we should accept it as join key.

We should add a few tests with binary keys, that can also be done in a follow-up. The parser supports binary literals, so we can use SQLQueryTestSuite for this.

I already added one: https://github.com/apache/spark/pull/15956/files#diff-999f50ed13c690eaec243e3b3446af08R468 , yea we should add more end-to-end tests in SQLQueryTestSuite in follow-up

cloud-fan · 2016-11-21T06:29:34Z

cc @yhuai @hvanhovell @gatorsmile

SparkQA · 2016-11-21T06:32:39Z

Test build #68920 has started for PR 15956 at commit a7b6e42.

rxin · 2016-11-21T07:09:54Z

So technically I think map type should be able to be used in equality comparison, but not orderable, so I'm not sure if this is correct.

Spark SQL currently has a restriction that we just don't allow map types to be used in equality comparison at all, but I don't want to structure code in a way that would make it harder to support that in the future.

rxin · 2016-11-21T07:10:54Z

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

isn't this "wrong"? map type is not supported here is it?

We need this to satisfy BinaryOperator, the type checking logic is defined by both BinaryOperator and checkInputDataTypes here. Shall we make BinaryComparison not extends BinaryOperator and duplicate its type checking logic here?

cloud-fan · 2016-11-21T08:21:59Z

Spark SQL currently has a restriction that we just don't allow map types to be used in equality comparison at all, but I don't want to structure code in a way that would make it harder to support that in the future.

If we wanna support equality comparison for map type in the future, we can just update the type checking logic for BinaryComparison. For now I think it's better to forbid users to do this, to avoid confusing behaviors, e.g. SELECT MAP(1,2) = MAP(1,2) returns false.

SparkQA · 2016-11-21T10:02:04Z

Test build #68932 has finished for PR 15956 at commit b95e25f.

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

SparkQA · 2016-11-21T14:51:59Z

Test build #68938 has finished for PR 15956 at commit fe6818b.

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

SparkQA · 2016-11-21T17:59:30Z

Test build #68940 has finished for PR 15956 at commit c3275ed.

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

rxin · 2016-11-21T19:07:05Z

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

why can't this be expressed? isn't the old ordered a way to express it?

because TypeCollection.Ordered doesn't cover all the cases, e.g. ArrayType and StructType are orderable.

hmm, Hive only supports atomic type, shall we follow it?

FYI, postgres supports binary comparison on array type.

gatorsmile · 2016-11-21T20:11:04Z

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

Semantically, this is different from the previous lilmits:

override def inputType: AbstractDataType = TypeCollection.Ordered

We add the support of StructType, ArrayType and UDT. It sounds like no test case coverage. Could you please add them?

let me minimize the size of this PR, I'll keep GreaterThan, LessThan, etc. unchanged, and only update EqualTo, EqualNullSafe to forbid map types.

hvanhovell · 2016-11-22T13:55:13Z

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

+    }
+  }
+
+  private def hasMapType(dt: DataType): Boolean = dt match {


dt.existsRecursively(_.isInstanceOf[MapType])?

hvanhovell · 2016-11-22T13:56:15Z

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


  override def inputType: AbstractDataType = AnyDataType

+  override def checkInputDataTypes(): TypeCheckResult = {


Minor: Can we deduplicate this?

well it's only duplicated twice and will be removed after #15970 , I think it should be fine.

hvanhovell

A few minor things, otherwise LGTM

SparkQA · 2016-11-22T14:41:41Z

Test build #68997 has finished for PR 15956 at commit b5a50f4.

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

hvanhovell · 2016-11-22T14:51:56Z

LGTM pending Jenkins

SparkQA · 2016-11-22T16:57:46Z

Test build #69002 has finished for PR 15956 at commit dff0b08.

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

hvanhovell · 2016-11-22T17:15:45Z

LGTM - merging to master/2.1. Thanks!

## What changes were proposed in this pull request? Technically map type is not orderable, but can be used in equality comparison. However, due to the limitation of the current implementation, map type can't be used in equality comparison so that it can't be join key or grouping key. This PR makes this limitation explicit, to avoid wrong result. ## How was this patch tested? updated tests. Author: Wenchen Fan <[email protected]> Closes #15956 from cloud-fan/map-type. (cherry picked from commit bb152cd) Signed-off-by: Herman van Hovell <[email protected]>

hvanhovell · 2016-11-22T17:18:57Z

@cloud-fan I cannot merge this to 2.0. Can you make a backport if we need one?

cloud-fan · 2016-11-23T05:57:53Z

Yea, I'll backport

## What changes were proposed in this pull request? Technically map type is not orderable, but can be used in equality comparison. However, due to the limitation of the current implementation, map type can't be used in equality comparison so that it can't be join key or grouping key. This PR makes this limitation explicit, to avoid wrong result. backport #15956 to 2.0 ## How was this patch tested? updated tests Author: Wenchen Fan <[email protected]> Closes #15988 from cloud-fan/map-type.

## What changes were proposed in this pull request? Technically map type is not orderable, but can be used in equality comparison. However, due to the limitation of the current implementation, map type can't be used in equality comparison so that it can't be join key or grouping key. This PR makes this limitation explicit, to avoid wrong result. ## How was this patch tested? updated tests. Author: Wenchen Fan <[email protected]> Closes apache#15956 from cloud-fan/map-type.

cloud-fan commented Nov 21, 2016

View reviewed changes

cloud-fan mentioned this pull request Nov 21, 2016

[SPARK-18053][SQL] compare unsafe and safe complex-type values correctly #15929

Closed

rxin reviewed Nov 21, 2016

View reviewed changes

cloud-fan force-pushed the map-type branch from b95e25f to fe6818b Compare November 21, 2016 13:59

cloud-fan force-pushed the map-type branch from fe6818b to c3275ed Compare November 21, 2016 15:41

rxin reviewed Nov 21, 2016

View reviewed changes

gatorsmile reviewed Nov 21, 2016

View reviewed changes

cloud-fan changed the title ~~[SPARK-18519][SQL] map type can not be used in binary comparison~~ [SPARK-18519][SQL] map type can not be used in EqualTo Nov 22, 2016

map type can not be used in EqualTo

b5a50f4

cloud-fan force-pushed the map-type branch from c3275ed to b5a50f4 Compare November 22, 2016 12:20

hvanhovell reviewed Nov 22, 2016

View reviewed changes

address comments

dff0b08

asfgit closed this in bb152cd Nov 22, 2016

cloud-fan mentioned this pull request Nov 23, 2016

[SPARK-18519][SQL][BRANCH-2.0] map type can not be used in EqualTo #15988

Closed


		override def inputType: AbstractDataType = AnyDataType

		override def checkInputDataTypes(): TypeCheckResult = {

[SPARK-18519][SQL] map type can not be used in EqualTo #15956

[SPARK-18519][SQL] map type can not be used in EqualTo #15956

Uh oh!

Conversation

cloud-fan commented Nov 21, 2016 • 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!

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!

cloud-fan commented Nov 21, 2016

Uh oh!

SparkQA commented Nov 21, 2016

Uh oh!

rxin commented Nov 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Nov 21, 2016

Uh oh!

SparkQA commented Nov 21, 2016

Uh oh!

SparkQA commented Nov 21, 2016

Uh oh!

SparkQA commented Nov 21, 2016

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!

hvanhovell left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 22, 2016

Uh oh!

hvanhovell commented Nov 22, 2016

Uh oh!

SparkQA commented Nov 22, 2016

Uh oh!

hvanhovell commented Nov 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hvanhovell commented Nov 22, 2016

Uh oh!

cloud-fan commented Nov 23, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cloud-fan commented Nov 21, 2016 •

edited

Loading

hvanhovell commented Nov 22, 2016 •

edited

Loading