Skip to content

[SPARK-16672][SQL] SQLBuilder should not raise exceptions on EXISTS queries #14307

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
dongjoon-hyun wants to merge 4 commits into from
Closed

[SPARK-16672][SQL] SQLBuilder should not raise exceptions on EXISTS queries #14307

dongjoon-hyun wants to merge 4 commits into from

Conversation

@dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented Jul 21, 2016

What changes were proposed in this pull request?

Currently, SQLBuilder raises empty.reduceLeft exceptions on unoptimized EXISTS queries. We had better prevent this.

scala> sql("CREATE TABLE t1(a int)")
scala> val df = sql("select * from t1 b where exists (select * from t1 a)")
scala> new org.apache.spark.sql.catalyst.SQLBuilder(df).toSQL
java.lang.UnsupportedOperationException: empty.reduceLeft

How was this patch tested?

Pass the Jenkins tests with a new test suite.

@SparkQA
Copy link

SparkQA commented Jul 21, 2016

Test build #62689 has finished for PR 14307 at commit 7baab5e.

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

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Jul 22, 2016

Hi, @hvanhovell .
Could you review this PR about SQLBuilder?

@jaceklaskowski
Copy link
Contributor

jaceklaskowski commented Jul 22, 2016

I didn't even know such a SQL query is possible in SQL :) Lots to learn still.

rxin
rxin reviewed Jul 23, 2016
View reviewed changes
sql/hive/src/test/scala/org/apache/spark/sql/hive/SQLBuilderSuite.scala Outdated
Copy link
Contributor

@rxin rxin Jul 23, 2016

Choose a reason for hiding this comment

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

LogicalPlanToSQLSuite?

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Jul 23, 2016

Choose a reason for hiding this comment

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

Oh, I see.

@SparkQA
Copy link

SparkQA commented Jul 23, 2016

Test build #62756 has finished for PR 14307 at commit 70f5401.

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

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Jul 25, 2016

Rebased.

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Jul 25, 2016

Hi, @hvanhovell .
Could you review this PR?

hvanhovell
hvanhovell reviewed Jul 25, 2016
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/catalyst/SQLBuilder.scala Outdated
Copy link
Contributor

@hvanhovell hvanhovell Jul 25, 2016

Choose a reason for hiding this comment

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

Minor: it is a little more readable if we move the addSubqueryIfNeeded(query) out of the if...else... and into a separate variable.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Jul 25, 2016

Choose a reason for hiding this comment

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

Oh, right. I'll fix soon.

@hvanhovell
Copy link
Contributor

hvanhovell commented Jul 25, 2016

One minor comment. Looks pretty good.

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Jul 25, 2016

Thank you for review, @hvanhovell .
I updated that.

@hvanhovell
Copy link
Contributor

hvanhovell commented Jul 25, 2016

LGMT, pending jenkins

@SparkQA
Copy link

SparkQA commented Jul 25, 2016

Test build #62815 has finished for PR 14307 at commit 3d8dbbd.

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

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Jul 25, 2016

It's strange . Jenkins does not trigger the test for the last commit.

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Jul 25, 2016

Oh, it became a maintenance mode. :(

@SparkQA
Copy link

SparkQA commented Jul 25, 2016

Test build #62820 has finished for PR 14307 at commit 7c7b58e.

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

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Jul 25, 2016

Hi, @hvanhovell .
Could you merge this PR? :)

rxin
rxin reviewed Jul 25, 2016
View reviewed changes
sql/hive/src/test/scala/org/apache/spark/sql/catalyst/LogicalPlanToSQLSuite.scala Outdated
Copy link
Contributor

@rxin rxin Jul 25, 2016

Choose a reason for hiding this comment

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

it'd be good to test the generated sql.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Jul 25, 2016

Choose a reason for hiding this comment

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

Sure. No problem.

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Jul 25, 2016

Hi, @rxin .
The PR is updated to use checkSQL.

@SparkQA
Copy link

SparkQA commented Jul 25, 2016

Test build #62828 has finished for PR 14307 at commit be2085a.

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

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Jul 26, 2016

Hi, @rxin .
Could you merge this PR please?
Since this PR uses checkSQL, it will conflict with Generate stable SQLs in SQLBuilder(#14257)
After merging this, I will rebase to the master and regenerate the answer in #14257.

@rxin
Copy link
Contributor

rxin commented Jul 26, 2016

Merging in master/2.0.

asfgit pushed a commit that referenced this pull request Jul 26, 2016
@dongjoon-hyun @rxin
[SPARK-16672][SQL] SQLBuilder should not raise exceptions on EXISTS q…
aeb6d5c 
…ueries

## What changes were proposed in this pull request?

Currently, `SQLBuilder` raises `empty.reduceLeft` exceptions on *unoptimized* `EXISTS` queries. We had better prevent this.
```scala
scala> sql("CREATE TABLE t1(a int)")
scala> val df = sql("select * from t1 b where exists (select * from t1 a)")
scala> new org.apache.spark.sql.catalyst.SQLBuilder(df).toSQL
java.lang.UnsupportedOperationException: empty.reduceLeft
```

## How was this patch tested?

Pass the Jenkins tests with a new test suite.

Author: Dongjoon Hyun <dongjoon@apache.org>

Closes #14307 from dongjoon-hyun/SPARK-16672.

(cherry picked from commit 8a8d26f)
Signed-off-by: Reynold Xin <rxin@databricks.com>
@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Jul 26, 2016

Thank you so much for review and merging, @rxin , @hvanhovell , and @jaceklaskowski !

@asfgit asfgit closed this in 8a8d26f Jul 26, 2016
@dongjoon-hyun dongjoon-hyun deleted the SPARK-16672 branch August 14, 2016 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dongjoon-hyun @SparkQA @jaceklaskowski @hvanhovell @rxin