Skip to content

[SPARK-16192][SQL] Add type checks in CollectSet#13892

Closed
maropu wants to merge 3 commits intoapache:masterfrom
maropu:SPARK-16192
Closed

[SPARK-16192][SQL] Add type checks in CollectSet#13892
maropu wants to merge 3 commits intoapache:masterfrom
maropu:SPARK-16192

Conversation

@maropu
Copy link
Member

@maropu maropu commented Jun 24, 2016

What changes were proposed in this pull request?

CollectSet cannot have map-typed data because MapTypeData does not implement equals.
So, this pr is to add type checks in CheckAnalysis.

How was this patch tested?

Added tests to check failures when we found map-typed data in CollectSet.

@SparkQA
Copy link

SparkQA commented Jun 24, 2016

Test build #61170 has finished for PR 13892 at commit 5110474.

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

@maropu
Copy link
Member Author

maropu commented Jun 24, 2016

@cloud-fan Could you check this?

@hvanhovell
Copy link
Contributor

@maropu could you implement checkInputDataTypes and do the validation in that method?

failAnalysis(s"grouping_id() can only be used with GroupingSets/Cube/Rollup")
failAnalysis("grouping_id() can only be used with GroupingSets/Cube/Rollup")

case c: CollectSet if c.child.dataType.isInstanceOf[MapType] =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be recursive. The child could be a struct/array containing a map.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, my bad. I'll fix this.

@maropu maropu changed the title [SPARK-16192][SQL] Add type checks in CheckAnalysis [SPARK-16192][SQL] Add type checks in CollectSet Jun 25, 2016
"another aggregate function." :: Nil)
}

test("we should fail analysis when we find map type data in collect_set") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets remove this test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay

@SparkQA
Copy link

SparkQA commented Jun 25, 2016

Test build #61211 has finished for PR 13892 at commit 73dc6e7.

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

@hvanhovell
Copy link
Contributor

LGTM - pending jenkins

@SparkQA
Copy link

SparkQA commented Jun 25, 2016

Test build #61212 has finished for PR 13892 at commit 681b993.

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

@SparkQA
Copy link

SparkQA commented Jun 25, 2016

Test build #61213 has finished for PR 13892 at commit dd7e233.

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

@SparkQA
Copy link

SparkQA commented Jun 25, 2016

Test build #61216 has finished for PR 13892 at commit 4924984.

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

@SparkQA
Copy link

SparkQA commented Jun 25, 2016

Test build #61217 has finished for PR 13892 at commit 1a445c8.

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

@hvanhovell
Copy link
Contributor

Merging to master/2.0. Thanks!

asfgit pushed a commit that referenced this pull request Jun 25, 2016
## What changes were proposed in this pull request?
`CollectSet` cannot have map-typed data because MapTypeData does not implement `equals`.
So, this pr is to add type checks in `CheckAnalysis`.

## How was this patch tested?
Added tests to check failures when we found map-typed data in `CollectSet`.

Author: Takeshi YAMAMURO <linguin.m.s@gmail.com>

Closes #13892 from maropu/SPARK-16192.

(cherry picked from commit d2e44d7)
Signed-off-by: Herman van Hovell <hvanhovell@databricks.com>
@asfgit asfgit closed this in d2e44d7 Jun 25, 2016
@maropu maropu deleted the SPARK-16192 branch July 5, 2017 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants