Skip to content

Conversation

@viirya
Copy link
Member

@viirya viirya commented Feb 15, 2016

JIRA: https://issues.apache.org/jira/browse/SPARK-13321

The following SQL can not be parsed with current parser:

SELECT  `u_1`.`id` FROM (((SELECT  `t0`.`id` FROM `default`.`t0`) UNION ALL (SELECT  `t0`.`id` FROM `default`.`t0`)) UNION ALL (SELECT  `t0`.`id` FROM `default`.`t0`)) AS u_1

We should fix it.

|
selectStatement[topLevel]
|
(LPAREN selectStatement[true]) => nestedSetOpSelectStatement[topLevel]
Copy link
Member Author

Choose a reason for hiding this comment

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

Passing true or false here doesn't really matter. This topLevel argument works only in setOpSelectStatement rule. We will pass in the real topLevel argument into the rule later.

@SparkQA
Copy link

SparkQA commented Feb 15, 2016

Test build #51295 has finished for PR 11204 at commit 93bd51e.

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

}

test("nesting UNION") {
parser.parsePlan("SELECT `u_1`.`id` FROM (((SELECT `t0`.`id` FROM `default`.`t0`) " +
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you test the actual plan?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. I will update this.

@SparkQA
Copy link

SparkQA commented Feb 15, 2016

Test build #51310 has finished for PR 11204 at commit f462997.

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

@SparkQA
Copy link

SparkQA commented Feb 15, 2016

Test build #51314 has finished for PR 11204 at commit 190b326.

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


comparePlans(parsedSame, expected)

val parsed2 = parser.parsePlan(
Copy link
Member Author

Choose a reason for hiding this comment

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

Recursively nested UNION.

Conflicts:
	sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/CatalystQlSuite.scala
@viirya
Copy link
Member Author

viirya commented Feb 21, 2016

@hvanhovell Do you have other comments on this PR?

@SparkQA
Copy link

SparkQA commented Feb 21, 2016

Test build #51630 has finished for PR 11204 at commit d024fff.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • abstract class SubqueryExpression extends LeafExpression
    • case class ScalarSubquery(
    • case class Subquery(name: String, child: SparkPlan) extends UnaryNode
    • case class ScalarSubquery(

@hvanhovell
Copy link
Contributor

LGTM

@viirya
Copy link
Member Author

viirya commented Feb 22, 2016

cc @rxin

@rxin
Copy link
Contributor

rxin commented Feb 22, 2016

Thanks - merging in master.

@asfgit asfgit closed this in 55d6fdf Feb 22, 2016
@rxin
Copy link
Contributor

rxin commented Feb 24, 2016

Note: I reverted this patch because HiveCompatibilitySuite.union16 was hanging from this.

Somehow we weren't running test cases in HiveCompatibilitySuite anymore for pull requests.

@gatorsmile
Copy link
Member

@rxin Do you need me to submit a PR to disable the following test case? This test case requires the nested Union support. Thanks!

test("three-child union") {
checkHiveQl(
"""
|SELECT id FROM parquet_t0
|UNION ALL SELECT id FROM parquet_t0
|UNION ALL SELECT id FROM parquet_t0
""".stripMargin)
}

@rxin
Copy link
Contributor

rxin commented Feb 24, 2016

Please do.

@rxin
Copy link
Contributor

rxin commented Feb 24, 2016

@viirya when you fixed this issue, please make sure you add a unit test aside from Hive compat test.

@gatorsmile
Copy link
Member

Just submit a PR: #11352. Thanks! @rxin

@viirya
Copy link
Member Author

viirya commented Feb 25, 2016

@rxin ok, I will look into this today.

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.

5 participants