Skip to content

[SPARK-17753][SQL] Allow a complex expression as the input a value based case statement #15322

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
hvanhovell wants to merge 2 commits into from
Closed

[SPARK-17753][SQL] Allow a complex expression as the input a value based case statement #15322

hvanhovell wants to merge 2 commits into from

Conversation

@hvanhovell
Copy link
Contributor

@hvanhovell hvanhovell commented Oct 2, 2016

What changes were proposed in this pull request?

We currently only allow relatively simple expressions as the input for a value based case statement. Expressions like case (a > 1) or (b = 2) when true then 1 when false then 0 end currently fail. This PR adds support for such expressions.

How was this patch tested?

Added a test to the ExpressionParserSuite.

@hvanhovell
Copy link
Contributor Author

hvanhovell commented Oct 2, 2016

cc @dongjoon-hyun

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Oct 2, 2016

Thank you for pinging me!

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Oct 2, 2016 •
edited
Loading

@hvanhovell , it looks great.

Most of cases are passed. I'm wondering if we can support the following, too?

sql("SELECT CASE 'a'='a' WHEN TRUE THEN 1 END").show

Since the followings are passed, the above one is a minor one.

sql("SELECT CASE ('a'='a') WHEN TRUE THEN 1 END").show
sql("SELECT CASE 1=1 WHEN TRUE THEN 1 END").show
sql("SELECT CASE 1='a' WHEN TRUE THEN 1 END").show

@SparkQA
Copy link

SparkQA commented Oct 2, 2016

Test build #66232 has finished for PR 15322 at commit b25c849.

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

@hvanhovell
Copy link
Contributor Author

hvanhovell commented Oct 2, 2016

@dongjoon-hyun I had to move a few rules around to get your example working. It was hitting the typeConstructor rule instead of the simpleCase rule; the lesson here is that generic rules should normally get lower priority then specific ones (if they both apply).

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Oct 2, 2016

Ah, that was the reason. Thank you so much, @hvanhovell .

@SparkQA
Copy link

SparkQA commented Oct 2, 2016

Test build #66246 has finished for PR 15322 at commit b6b3910.

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

viirya
viirya reviewed Oct 3, 2016
View reviewed changes
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/ExpressionParserSuite.scala
CaseKeyWhen('a, Seq(1, 'b, 2, 'c, 'd)))
assertEqual("case (a or b) when true then c when false then d else e end",
CaseKeyWhen('a || 'b, Seq(true, 'c, false, 'd, 'e)))
assertEqual("case 'a'='a' when true then 1 end",
Copy link
Member

@viirya viirya Oct 3, 2016

Choose a reason for hiding this comment

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

nit: seems 'a'='a' is a valueExpression? So it should be passed even without this change.

Copy link
Contributor Author

@hvanhovell hvanhovell Oct 3, 2016

Choose a reason for hiding this comment

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

Yeah, I thought the same thing. The issue here is that case 'a' also matches the typeConstructor rule which was triggered before the simpleCase rule. So I also fixed this by moving the rules around a bit more.

Copy link
Member

@viirya viirya Oct 3, 2016

Choose a reason for hiding this comment

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

oh, I see. Currently the first element in typeConstructor is an identifier. Is it too loose?

@viirya
Copy link
Member

viirya commented Oct 3, 2016

LGTM except a minor comment.

@hvanhovell
Copy link
Contributor Author

hvanhovell commented Oct 4, 2016

Merging to master/2.0. Thanks for the reviews!

asfgit pushed a commit that referenced this pull request Oct 4, 2016
@hvanhovell
[SPARK-17753][SQL] Allow a complex expression as the input a value ba…
5843932 
…sed case statement

## What changes were proposed in this pull request?
We currently only allow relatively simple expressions as the input for a value based case statement. Expressions like `case (a > 1) or (b = 2) when true then 1 when false then 0 end` currently fail. This PR adds support for such expressions.

## How was this patch tested?
Added a test to the ExpressionParserSuite.

Author: Herman van Hovell <hvanhovell@databricks.com>

Closes #15322 from hvanhovell/SPARK-17753.

(cherry picked from commit 2bbecde)
Signed-off-by: Herman van Hovell <hvanhovell@databricks.com>
@asfgit asfgit closed this in 2bbecde Oct 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@viirya viirya viirya left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hvanhovell @dongjoon-hyun @SparkQA @viirya