[SPARK-17982][SQL] SQLBuilder should wrap the generated SQL with parenthesis for LIMIT #15546

dongjoon-hyun · 2016-10-19T03:43:12Z

What changes were proposed in this pull request?

Currently, SQLBuilder handles LIMIT by always adding LIMIT at the end of the generated subSQL. It makes RuntimeExceptions like the following. This PR adds a parenthesis always except SubqueryAlias is used together with LIMIT.

Before

scala> sql("CREATE TABLE tbl(id INT)")
scala> sql("CREATE VIEW v1(id2) AS SELECT id FROM tbl LIMIT 2")
java.lang.RuntimeException: Failed to analyze the canonicalized SQL: ...

After

scala> sql("CREATE TABLE tbl(id INT)")
scala> sql("CREATE VIEW v1(id2) AS SELECT id FROM tbl LIMIT 2")
scala> sql("SELECT id2 FROM v1")
res4: org.apache.spark.sql.DataFrame = [id2: int]

Fixed cases in this PR

The following two cases are the detail query plans having problematic SQL generations.

SELECT * FROM (SELECT id FROM tbl LIMIT 2)

Please note that FROM SELECT part of the generated SQL in the below. When we don't use '()' for limit, this fails.

# Original logical plan:
Project [id#1]
+- GlobalLimit 2
   +- LocalLimit 2
      +- Project [id#1]
         +- MetastoreRelation default, tbl

# Canonicalized logical plan:
Project [gen_attr_0#1 AS id#4]
+- SubqueryAlias tbl
   +- Project [gen_attr_0#1]
      +- GlobalLimit 2
         +- LocalLimit 2
            +- Project [gen_attr_0#1]
               +- SubqueryAlias gen_subquery_0
                  +- Project [id#1 AS gen_attr_0#1]
                     +- SQLTable default, tbl, [id#1]

# Generated SQL:
SELECT `gen_attr_0` AS `id` FROM (SELECT `gen_attr_0` FROM SELECT `gen_attr_0` FROM (SELECT `id` AS `gen_attr_0` FROM `default`.`tbl`) AS gen_subquery_0 LIMIT 2) AS tbl

SELECT * FROM (SELECT id FROM tbl TABLESAMPLE (2 ROWS))

Please note that ((~~~) AS gen_subquery_0 LIMIT 2) in the below. When we use '()' for limit on SubqueryAlias, this fails.

# Original logical plan:
Project [id#1]
+- Project [id#1]
   +- GlobalLimit 2
      +- LocalLimit 2
         +- MetastoreRelation default, tbl

# Canonicalized logical plan:
Project [gen_attr_0#1 AS id#4]
+- SubqueryAlias tbl
   +- Project [gen_attr_0#1]
      +- GlobalLimit 2
         +- LocalLimit 2
            +- SubqueryAlias gen_subquery_0
               +- Project [id#1 AS gen_attr_0#1]
                  +- SQLTable default, tbl, [id#1]

# Generated SQL:
SELECT `gen_attr_0` AS `id` FROM (SELECT `gen_attr_0` FROM ((SELECT `id` AS `gen_attr_0` FROM `default`.`tbl`) AS gen_subquery_0 LIMIT 2)) AS tbl

How was this patch tested?

Pass the Jenkins test with a newly added test case.

gatorsmile · 2016-10-19T03:45:58Z

Wrong JIRA number. : )

dongjoon-hyun · 2016-10-19T03:46:55Z

Ooops. Thank you for fixing me! @gatorsmile

SparkQA · 2016-10-19T05:13:15Z

Test build #67166 has finished for PR 15546 at commit 2af55c8.

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

SparkQA · 2016-10-19T05:21:02Z

Test build #67165 has finished for PR 15546 at commit 85c0686.

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

SparkQA · 2016-10-19T07:19:10Z

Test build #67172 has finished for PR 15546 at commit 105c36a.

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

rxin · 2016-10-19T07:52:33Z

sql/core/src/main/scala/org/apache/spark/sql/catalyst/SQLBuilder.scala

shouldn't we create a test in the sql generation test there?

dongjoon-hyun · 2016-10-19T08:09:15Z

Thank you for review, @rxin .
Here, CREATE VIEW is not supported by SQLGeneration test suite.

CREATE VIEW v1(id2) AS SELECT id FROM tbl LIMIT 2

The case is a view with column names and limit.

rxin · 2016-10-19T08:17:56Z

but you can construct a query with limit to make sure the output has ( ) no?

dongjoon-hyun · 2016-10-19T08:22:38Z

Sorry, but what do you mean by the output has ( ) no??

dongjoon-hyun · 2016-10-19T08:27:52Z

Ah. I see. ( ) means parenthesis, literally. I'll replace CREATE VIEW test case with SQL Generation suite. BTW, at the last commit, the PR is changed to add parenthesis if the child of Limit is Project.

dongjoon-hyun · 2016-10-19T09:33:01Z

sql/hive/src/test/resources/sqlgen/limit.sql

Without this PR, this test case fails because it generate the following query. Note that the third line.

SELECT `gen_attr_0` AS `id` FROM (SELECT `gen_attr_0` FROM SELECT `gen_attr_0` FROM (SELECT `id` AS `gen_attr_0`, `name` AS `gen_attr_1` FROM `default`.`tbl`) AS gen_subquery_0 LIMIT 2) AS tbl

dongjoon-hyun · 2016-10-19T09:34:37Z

sql/hive/src/test/scala/org/apache/spark/sql/catalyst/LogicalPlanToSQLSuite.scala

This should be absolute path. It seems to be changed by accidentally by the following PR.

8a6bbe0#diff-d3bb909d073a402405e6089cd9ee17efR48

Up to now, SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "hive/test-only *LogicalPlanToSQLSuite" didn't update the golden files.

Hi, @rxin .
IMO, this bug on LogicalPlanToSQLSuite is needed to be fixed soon. If this PR is needed to be reviewed more, should I make another PR containing that only?

dongjoon-hyun · 2016-10-19T09:37:02Z

@rxin . I moved the test case and fixed a bug in LogicalPlanToSQLSuite.

SparkQA · 2016-10-19T10:16:19Z

Test build #67178 has finished for PR 15546 at commit 637499b.

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

dongjoon-hyun · 2016-10-19T11:05:58Z

The current running build has one testcase failure, when schema inference is turned on, should read partition data. It seems to be irrelevant.

dongjoon-hyun · 2016-10-19T11:06:05Z

Retest this please.

SparkQA · 2016-10-19T11:10:42Z

Test build #67185 has finished for PR 15546 at commit 05d9475.

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

SparkQA · 2016-10-19T13:11:27Z

Test build #67190 has finished for PR 15546 at commit 05d9475.

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

rxin · 2016-10-20T02:37:51Z

sql/core/src/main/scala/org/apache/spark/sql/catalyst/SQLBuilder.scala

is it always safe to just add limit to any plan?

Yes. Mostly. During testing, I realized that the previous code is designed to add "LIMIT" string without parenthesis to handle the most cases. And, Spark do not allow double parenthesis.

ORDER BY: Limit(_, Sort)

GROUP BY: Limit(_, Aggr)
...

Project is the only observed cases in CREAE VIEW or SELECT * FROM (SELECT ... LIMIT ..).

I'll change the title of PR.

dongjoon-hyun · 2016-10-20T07:21:49Z

@rxin .
I'll add more testcases for this and figure out the correct answer for your question.

is it always safe to just add limit to any plan?

rxin · 2016-11-01T20:50:37Z

@dongjoon-hyun any update on this?

dongjoon-hyun · 2016-11-01T20:56:49Z

Sorry for delay, @rxin . I'm still working on, and will update in a few days.

rxin · 2016-11-01T21:01:23Z

@dongjoon-hyun we are going to cut the release branch today (or tomorrow) so it would be good to get this in asap.

dongjoon-hyun · 2016-11-01T21:04:00Z

Oh, 2.1-rc1 today? I see.

rxin · 2016-11-01T21:10:05Z

Not rc, but the branch cut.

dongjoon-hyun · 2016-11-01T21:11:47Z

Ah, I see. It's feature freeze. Thank you for informing me.

SparkQA · 2016-11-02T09:29:01Z

Test build #67968 has finished for PR 15546 at commit 813e41f.

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

gatorsmile · 2016-11-06T06:20:53Z

@dongjoon-hyun I have a general comment about the SQL generation fixes. Not sure how you fix the issue. Normally, when we did it, we need to see the analyzed SQL plan. In this case, it becomes a little complex, we called addSubqueryIfNeeded in toSQL.

Could you try to show reviewers the failed plans with the added SubqueryAlias? Then, we can discuss whether the fix coverage and test case coverage.

dongjoon-hyun · 2016-11-06T07:05:44Z

Thank you for review, @gatorsmile . I'll add the failed plans, too.

dongjoon-hyun · 2016-11-06T11:11:56Z

Hi, @gatorsmile . The PR is updated according to the advice.

Remove the goldenSQLPath from this PR.
Update the PR description about the target fixed cases and the detailed plans, which includes the SubqueryAlias cases.

SparkQA · 2016-11-06T13:24:17Z

Test build #68239 has finished for PR 15546 at commit a81434f.

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

dongjoon-hyun · 2016-11-10T06:33:23Z

Hi, @gatorsmile .
Could you review again?

…nthesis for LIMIT

dongjoon-hyun · 2016-11-10T09:17:59Z

During updating the PR, I rebased and squashed to resolve conflict.

SparkQA · 2016-11-10T10:13:30Z

Test build #68458 has finished for PR 15546 at commit e76a675.

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

dongjoon-hyun · 2016-11-10T16:25:39Z

The three failures seems to be irrelevant.

[info] - randomized aggregation test - [typed, with partial + safe] - with grouping keys - with non-empty input *** FAILED *** (1 second, 419 milliseconds)

dongjoon-hyun · 2016-11-10T16:25:45Z

Retest this please.

gatorsmile · 2016-11-10T17:50:45Z

Will review this PR tonight. Thanks!

dongjoon-hyun · 2016-11-10T18:12:37Z

Thank you, @gatorsmile !

SparkQA · 2016-11-10T18:40:46Z

Test build #68480 has finished for PR 15546 at commit e76a675.

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

SparkQA · 2016-11-10T20:20:59Z

Test build #68484 has finished for PR 15546 at commit 604414e.

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

dongjoon-hyun · 2016-11-11T11:48:49Z

Could you review again, @gatorsmile ?

gatorsmile · 2016-11-11T17:42:23Z

LGTM

gatorsmile · 2016-11-11T17:44:19Z

retest this please

dongjoon-hyun · 2016-11-11T18:09:12Z

Thank you, @gatorsmile .

SparkQA · 2016-11-11T20:03:22Z

Test build #68528 has finished for PR 15546 at commit 604414e.

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

…nthesis for LIMIT ## What changes were proposed in this pull request? Currently, `SQLBuilder` handles `LIMIT` by always adding `LIMIT` at the end of the generated subSQL. It makes `RuntimeException`s like the following. This PR adds a parenthesis always except `SubqueryAlias` is used together with `LIMIT`. **Before** ``` scala scala> sql("CREATE TABLE tbl(id INT)") scala> sql("CREATE VIEW v1(id2) AS SELECT id FROM tbl LIMIT 2") java.lang.RuntimeException: Failed to analyze the canonicalized SQL: ... ``` **After** ``` scala scala> sql("CREATE TABLE tbl(id INT)") scala> sql("CREATE VIEW v1(id2) AS SELECT id FROM tbl LIMIT 2") scala> sql("SELECT id2 FROM v1") res4: org.apache.spark.sql.DataFrame = [id2: int] ``` **Fixed cases in this PR** The following two cases are the detail query plans having problematic SQL generations. 1. `SELECT * FROM (SELECT id FROM tbl LIMIT 2)` Please note that **FROM SELECT** part of the generated SQL in the below. When we don't use '()' for limit, this fails. ```scala # Original logical plan: Project [id#1] +- GlobalLimit 2 +- LocalLimit 2 +- Project [id#1] +- MetastoreRelation default, tbl # Canonicalized logical plan: Project [gen_attr_0#1 AS id#4] +- SubqueryAlias tbl +- Project [gen_attr_0#1] +- GlobalLimit 2 +- LocalLimit 2 +- Project [gen_attr_0#1] +- SubqueryAlias gen_subquery_0 +- Project [id#1 AS gen_attr_0#1] +- SQLTable default, tbl, [id#1] # Generated SQL: SELECT `gen_attr_0` AS `id` FROM (SELECT `gen_attr_0` FROM SELECT `gen_attr_0` FROM (SELECT `id` AS `gen_attr_0` FROM `default`.`tbl`) AS gen_subquery_0 LIMIT 2) AS tbl ``` 2. `SELECT * FROM (SELECT id FROM tbl TABLESAMPLE (2 ROWS))` Please note that **((~~~) AS gen_subquery_0 LIMIT 2)** in the below. When we use '()' for limit on `SubqueryAlias`, this fails. ```scala # Original logical plan: Project [id#1] +- Project [id#1] +- GlobalLimit 2 +- LocalLimit 2 +- MetastoreRelation default, tbl # Canonicalized logical plan: Project [gen_attr_0#1 AS id#4] +- SubqueryAlias tbl +- Project [gen_attr_0#1] +- GlobalLimit 2 +- LocalLimit 2 +- SubqueryAlias gen_subquery_0 +- Project [id#1 AS gen_attr_0#1] +- SQLTable default, tbl, [id#1] # Generated SQL: SELECT `gen_attr_0` AS `id` FROM (SELECT `gen_attr_0` FROM ((SELECT `id` AS `gen_attr_0` FROM `default`.`tbl`) AS gen_subquery_0 LIMIT 2)) AS tbl ``` ## How was this patch tested? Pass the Jenkins test with a newly added test case. Author: Dongjoon Hyun <dongjoon@apache.org> Closes #15546 from dongjoon-hyun/SPARK-17982. (cherry picked from commit d42bb7c) Signed-off-by: gatorsmile <gatorsmile@gmail.com>

gatorsmile · 2016-11-11T21:30:07Z

Thanks! Merging to master/2.1.

gatorsmile · 2016-11-11T21:30:56Z

Could you please backport it to 2.0? It sounds the JIRA opener needs it in Spark 2.0 branch.

dongjoon-hyun · 2016-11-11T22:17:09Z

Thank you, @gatorsmile .
I'll make a backport for 2.0, too.

dongjoon-hyun · 2016-11-11T22:17:46Z

Thank you for review, @rxin and @cloud-fan , too.

…nthesis for LIMIT ## What changes were proposed in this pull request? Currently, `SQLBuilder` handles `LIMIT` by always adding `LIMIT` at the end of the generated subSQL. It makes `RuntimeException`s like the following. This PR adds a parenthesis always except `SubqueryAlias` is used together with `LIMIT`. **Before** ``` scala scala> sql("CREATE TABLE tbl(id INT)") scala> sql("CREATE VIEW v1(id2) AS SELECT id FROM tbl LIMIT 2") java.lang.RuntimeException: Failed to analyze the canonicalized SQL: ... ``` **After** ``` scala scala> sql("CREATE TABLE tbl(id INT)") scala> sql("CREATE VIEW v1(id2) AS SELECT id FROM tbl LIMIT 2") scala> sql("SELECT id2 FROM v1") res4: org.apache.spark.sql.DataFrame = [id2: int] ``` **Fixed cases in this PR** The following two cases are the detail query plans having problematic SQL generations. 1. `SELECT * FROM (SELECT id FROM tbl LIMIT 2)` Please note that **FROM SELECT** part of the generated SQL in the below. When we don't use '()' for limit, this fails. ```scala # Original logical plan: Project [id#1] +- GlobalLimit 2 +- LocalLimit 2 +- Project [id#1] +- MetastoreRelation default, tbl # Canonicalized logical plan: Project [gen_attr_0#1 AS id#4] +- SubqueryAlias tbl +- Project [gen_attr_0#1] +- GlobalLimit 2 +- LocalLimit 2 +- Project [gen_attr_0#1] +- SubqueryAlias gen_subquery_0 +- Project [id#1 AS gen_attr_0#1] +- SQLTable default, tbl, [id#1] # Generated SQL: SELECT `gen_attr_0` AS `id` FROM (SELECT `gen_attr_0` FROM SELECT `gen_attr_0` FROM (SELECT `id` AS `gen_attr_0` FROM `default`.`tbl`) AS gen_subquery_0 LIMIT 2) AS tbl ``` 2. `SELECT * FROM (SELECT id FROM tbl TABLESAMPLE (2 ROWS))` Please note that **((~~~) AS gen_subquery_0 LIMIT 2)** in the below. When we use '()' for limit on `SubqueryAlias`, this fails. ```scala # Original logical plan: Project [id#1] +- Project [id#1] +- GlobalLimit 2 +- LocalLimit 2 +- MetastoreRelation default, tbl # Canonicalized logical plan: Project [gen_attr_0#1 AS id#4] +- SubqueryAlias tbl +- Project [gen_attr_0#1] +- GlobalLimit 2 +- LocalLimit 2 +- SubqueryAlias gen_subquery_0 +- Project [id#1 AS gen_attr_0#1] +- SQLTable default, tbl, [id#1] # Generated SQL: SELECT `gen_attr_0` AS `id` FROM (SELECT `gen_attr_0` FROM ((SELECT `id` AS `gen_attr_0` FROM `default`.`tbl`) AS gen_subquery_0 LIMIT 2)) AS tbl ``` ## How was this patch tested? Pass the Jenkins test with a newly added test case. Author: Dongjoon Hyun <dongjoon@apache.org> Closes apache#15546 from dongjoon-hyun/SPARK-17982.

dongjoon-hyun changed the title ~~[SPARK-17892][SQL] SQLBuilder should wrap the generated SQL with parenthesis for LIMIT~~ [SPARK-17982][SQL] SQLBuilder should wrap the generated SQL with parenthesis for LIMIT Oct 19, 2016

rxin reviewed Oct 19, 2016

View reviewed changes

dongjoon-hyun commented Oct 19, 2016

View reviewed changes

rxin reviewed Oct 20, 2016

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-17982][SQL] SQLBuilder should wrap the generated SQL with parenthesis for LIMIT~~ [SPARK-17982][SQL] SQLBuilder should wrap the generated SQL for Limit with Project in () Oct 20, 2016

dongjoon-hyun changed the title ~~[SPARK-17982][SQL] SQLBuilder should wrap the generated SQL for Limit with Project in ()~~ [SPARK-17982][SQL][WIP] SQLBuilder should wrap the generated SQL for Limit with Project in () Oct 20, 2016

dongjoon-hyun mentioned this pull request Nov 6, 2016

[SPARK-18292][SQL] LogicalPlanToSQLSuite should not use resource dependent path for golden file generation #15789

Closed

[SPARK-17982][SQL] SQLBuilder should wrap the generated SQL with pare…

e76a675

…nthesis for LIMIT

Add comments.

604414e

asfgit closed this in d42bb7c Nov 11, 2016

dongjoon-hyun mentioned this pull request Nov 11, 2016

[SPARK-17982][SQL][BACKPORT-2.0] SQLBuilder should wrap the generated SQL with parenthesis for LIMIT #15856

Closed

dongjoon-hyun deleted the SPARK-17982 branch January 7, 2019 07:03

[SPARK-17982][SQL] SQLBuilder should wrap the generated SQL with parenthesis for LIMIT #15546

[SPARK-17982][SQL] SQLBuilder should wrap the generated SQL with parenthesis for LIMIT #15546

Uh oh!

Conversation

dongjoon-hyun commented Oct 19, 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!

gatorsmile commented Oct 19, 2016

Uh oh!

dongjoon-hyun commented Oct 19, 2016

Uh oh!

SparkQA commented Oct 19, 2016

Uh oh!

SparkQA commented Oct 19, 2016

Uh oh!

SparkQA commented Oct 19, 2016

Uh oh!

rxin Oct 19, 2016

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Oct 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rxin commented Oct 19, 2016

Uh oh!

dongjoon-hyun commented Oct 19, 2016

Uh oh!

dongjoon-hyun commented Oct 19, 2016

Uh oh!

dongjoon-hyun Oct 19, 2016

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Oct 19, 2016

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Oct 19, 2016

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Oct 19, 2016

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Oct 19, 2016

Uh oh!

SparkQA commented Oct 19, 2016

Uh oh!

dongjoon-hyun commented Oct 19, 2016

Uh oh!

dongjoon-hyun commented Oct 19, 2016

Uh oh!

SparkQA commented Oct 19, 2016

Uh oh!

SparkQA commented Oct 19, 2016

Uh oh!

rxin Oct 20, 2016

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Oct 20, 2016 • 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!

dongjoon-hyun Oct 20, 2016

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Oct 20, 2016

Uh oh!

rxin commented Nov 1, 2016

Uh oh!

dongjoon-hyun commented Nov 1, 2016

Uh oh!

rxin commented Nov 1, 2016

Uh oh!

dongjoon-hyun commented Nov 1, 2016

Uh oh!

rxin commented Nov 1, 2016

Uh oh!

dongjoon-hyun commented Nov 1, 2016

Uh oh!

SparkQA commented Nov 2, 2016

dongjoon-hyun commented Oct 19, 2016 •

edited

Loading

dongjoon-hyun commented Oct 19, 2016 •

edited

Loading

dongjoon-hyun Oct 20, 2016 •

edited

Loading