[SPARK-13321][SQL] Add nested union test cases #11361

viirya · 2016-02-25T03:45:23Z

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

What changes were proposed in this pull request?

Looks like the following SQL can be parsed now with new ANTLR4 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 just need to add test cases.

How was this patch tested?

New tests are added to PlanParserSuit and HiveQuerySuite.

Conflicts: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/CatalystQlSuite.scala

viirya · 2016-02-25T03:48:04Z

cc @rxin

I am not sure if HiveCompatibilitySuite.union16 was hanging from this. Because I copy the same queries from union16 to HiveQuerySuite and they are working locally. I will see how jenkins outputs from this change as it now should run HiveCompatibilitySuite.

SparkQA · 2016-02-25T05:51:50Z

Test build #51928 has finished for PR 11361 at commit 5ff5ac2.

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

viirya · 2016-02-25T05:57:11Z

@rxin Looks like HiveCompatibilitySuite.union16 doesn't hang from this. But it actually takes long time to finish that test ([info] - union16 (13 minutes, 21 seconds)). I don't know if it is different than previous run before this pr. Because this only change parser rule, I think it should not modify the time to run the union test.

rxin · 2016-02-25T06:03:47Z

Can you take a look why would 2 queries take 13 mins?

EXPLAIN
SELECT count(1) FROM (
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL

  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL

  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL

  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL

  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src) src;


SELECT count(1) FROM (
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL

  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL

  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL

  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL

  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src) src;

When I was running this, this was running in parser forever.

viirya · 2016-02-25T08:12:50Z

@rxin ok. I got why it takes so long to finish the test.

The original query:

SELECT count(1) FROM (
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL

  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL

  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL

  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL

  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src UNION ALL
  SELECT key, value FROM src) src;

will result an analyzed plan like:

== Analyzed Logical Plan ==
count(1): bigint
Aggregate [(count(1),mode=Complete,isDistinct=false) AS count(1)#393L]
+- SubqueryAlias src
   +- Union
      :- Union
      :  :- Union
      :  :  :- Union
      :  :  :  :- Union
      :  :  :  :  :- Union
      :  :  :  :  :  :- Union
      :  :  :  :  :  :  :- Union
      :  :  :  :  :  :  :  :- Union
      :  :  :  :  :  :  :  :  :- Union
      :  :  :  :  :  :  :  :  :  :- Union
      :  :  :  :  :  :  :  :  :  :  :- Union
      :  :  :  :  :  :  :  :  :  :  :  :- Union
      :  :  :  :  :  :  :  :  :  :  :  :  :- Union
      :  :  :  :  :  :  :  :  :  :  :  :  :  :- Union
      :  :  :  :  :  :  :  :  :  :  :  :  :  :  :- Union
      :  :  :  :  :  :  :  :  :  :  :  :  :  :  :  :- Union
      :  :  :  :  :  :  :  :  :  :  :  :  :  :  :  :  :- Union
      :  :  :  :  :  :  :  :  :  :  :  :  :  :  :  :  :  :- Union
      :  :  :  :  :  :  :  :  :  :  :  :  :  :  :  :  :  :  :- Union
      :  :  :  :  :  :  :  :  :  :  :  :  :  :  :  :  :  :  :  :- Union
      :  :  :  :  :  :  :  :  :  :  :  :  :  :  :  :  :  :  :  :  :- Union
      ...(skip)

In HiveComparisonTest, we will try to use SQLBuilder to convert analyzed plan back to sql query.

Because PR #11195 adds a () to wrap sql queries for union's children, it will generate a deeply nested sql query for union16 query:

SELECT count(1) AS `count(1)` FROM (((((((((((((((((((((((((SELECT `src`.`key`, `src`.`value` FROM `default`.`src`) UNION ALL (SELECT `src`.`key`, `src`.`value` FROM `default`.`src`)) UNION ALL (SELECT `src`.`key`, `src`.`value` FROM `default`.`src`)) UNION ALL (SELECT `src`.`key`, `src`.`value` FROM `default`.`src`)) UNION ALL (SELECT `src`.`key`, `src`.`value` FROM `default`.`src`)) UNION ALL (SELECT `src`.`key`, `src`.`value` FROM `default`.`src`)) UNION ALL (SELECT `src`.`key`, `src`.`value` FROM `default`.`src`)) UNION ALL (SELECT `src`.`key`, `src`.`value` FROM `default`.`src`)) UNION ALL (SELECT `src`.`key`, `src`.`value` FROM `default`.`src`)) UNION ALL (SELECT `src`.`key`, `src`.`value` FROM `default`.`src`)) UNION ALL (SELECT `src`.`key`, `src`.`value` FROM `default`.`src`)) UNION ALL (SELECT `src`.`key`, `src`.`value` FROM `default`.`src`)) UNION ALL (SELECT `src`.`key`, `src`.`value` FROM `default`.`src`)) UNION ALL (SELECT `src`.`key`, `src`.`value` FROM `default`.`src`)) UNION ALL (SELECT `src`.`key`, `src`.`value` FROM `default`.`src`)) UNION ALL (SELECT `src`.`key`, `src`.`value` FROM `default`.`src`)) UNION ALL (SELECT `src`.`key`, `src`.`value` FROM `default`.`src`)) UNION ALL (SELECT `src`.`key`, `src`.`value` FROM `default`.`src`)) UNION ALL (SELECT `src`.`key`, `src`.`value` FROM `default`.`src`)) UNION ALL (SELECT `src`.`key`, `src`.`value` FROM `default`.`src`)) UNION ALL (SELECT `src`.`key`, `src`.`value` FROM `default`.`src`)) UNION ALL (SELECT `src`.`key`, `src`.`value` FROM `default`.`src`)) UNION ALL (SELECT `src`.`key`, `src`.`value` FROM `default`.`src`)) UNION ALL (SELECT `src`.`key`, `src`.`value` FROM `default`.`src`)) UNION ALL (SELECT `src`.`key`, `src`.`value` FROM `default`.`src`)) AS src

Basically the parser processes nested union query with a recursive approach, to parse such deeply nested query cost much time. That is why union16 takes so long to finish.

If we remove the () from the sql queries for union's children in SQLBuilder, the generated sql query would be:

SELECT count(1) AS `count(1)` FROM (SELECT `src`.`key`, `src`.`value` FROM `default`.`src` UNION ALL SELECT `src`.`key`, `src`.`value` FROM `default`.`src` UNION ALL SELECT `src`.`key`, `src`.`value` FROM `default`.`src` UNION ALL SELECT `src`.`key`, `src`.`value` FROM `default`.`src` UNION ALL SELECT `src`.`key`, `src`.`value` FROM `default`.`src` UNION ALL SELECT `src`.`key`, `src`.`value` FROM `default`.`src` UNION ALL SELECT `src`.`key`, `src`.`value` FROM `default`.`src` UNION ALL SELECT `src`.`key`, `src`.`value` FROM `default`.`src` UNION ALL SELECT `src`.`key`, `src`.`value` FROM `default`.`src` UNION ALL SELECT `src`.`key`, `src`.`value` FROM `default`.`src` UNION ALL SELECT `src`.`key`, `src`.`value` FROM `default`.`src` UNION ALL SELECT `src`.`key`, `src`.`value` FROM `default`.`src` UNION ALL SELECT `src`.`key`, `src`.`value` FROM `default`.`src` UNION ALL SELECT `src`.`key`, `src`.`value` FROM `default`.`src` UNION ALL SELECT `src`.`key`, `src`.`value` FROM `default`.`src` UNION ALL SELECT `src`.`key`, `src`.`value` FROM `default`.`src` UNION ALL SELECT `src`.`key`, `src`.`value` FROM `default`.`src` UNION ALL SELECT `src`.`key`, `src`.`value` FROM `default`.`src` UNION ALL SELECT `src`.`key`, `src`.`value` FROM `default`.`src` UNION ALL SELECT `src`.`key`, `src`.`value` FROM `default`.`src` UNION ALL SELECT `src`.`key`, `src`.`value` FROM `default`.`src` UNION ALL SELECT `src`.`key`, `src`.`value` FROM `default`.`src` UNION ALL SELECT `src`.`key`, `src`.`value` FROM `default`.`src` UNION ALL SELECT `src`.`key`, `src`.`value` FROM `default`.`src` UNION ALL SELECT `src`.`key`, `src`.`value` FROM `default`.`src`) AS src

Then the union16 can normally finish under this patch.

viirya · 2016-02-25T08:14:43Z

@rxin I don't think we should convert union plan back to nested sql query. I would like to remove the () from SQLBuilder for union. What do you think?

rxin · 2016-02-25T08:58:00Z

Any other thing we can do for this perf problem? It's only 25 levels of nesting. It seems strange to me that the parser would take mins to parse this ... It's hard for me to believe it's just because of some recursion. Is there some exponential complexity here?

cc @hvanhovell

viirya · 2016-02-25T09:50:56Z

ok. I will continue to see if we can improve the performance of parsing nested union.

hvanhovell · 2016-02-26T06:42:37Z

@viirya I am currently working ANTLR4 based version of the parsers (see my repo for a few initial commits). It is basically a port of the presto parser. I need another week or so to get most of the HQL functionality working. Perhaps we should wait with this until the new parser is ready.

(edited)

viirya · 2016-02-26T06:54:51Z

@hvanhovell Great to see your initial work. It looks promising. I think this can wait until the new parser. Besides, are we going to retire ANTLR3 used now?

hvanhovell · 2016-02-26T08:05:16Z

@viirya I really don't see any reason to keep ANTLR3 around after we migrate the parser.

Conflicts: sql/catalyst/src/main/antlr3/org/apache/spark/sql/catalyst/parser/SparkSqlParser.g sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/CatalystQlSuite.scala

viirya · 2016-04-01T07:05:32Z

cc @hvanhovell @rxin Because new ANTLR4 parser seems can support this syntax. I updated this to add test cases only. Please take a look. Thanks!

hvanhovell · 2016-04-01T07:37:35Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala

+       |(SELECT  `t0`.`id` FROM `default`.`t0`)) AS u_1
+      """.stripMargin)
+
+    val expected = Project(


Minor: @viirya could you update this use the DSL and assertEqual equals? It makes this a bit easier to read.

BTW this test is very similar to the following test case: https://github.com/apache/spark/blob/master/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala#L384-L392

hmm. indeed. If so, I think I can close this pr now.

@hvanhovell Is new ANTLR4 parser natively to solve this?

@viirya the new parser handles nested queries a lot better. This is mainly due to ANTLR4's better parsing algorithms.

SparkQA · 2016-04-01T08:59:28Z

Test build #54689 has finished for PR 11361 at commit 6775020.

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

viirya added 7 commits February 15, 2016 06:04

Support nested UNION in parser.

93bd51e

Test actual parsed plan.

f462997

Support recursively nested one.

190b326

Merge remote-tracking branch 'upstream/master' into nested-union

a00261d

Merge remote-tracking branch 'upstream/master' into nested-union

d024fff

Conflicts: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/CatalystQlSuite.scala

Merge remote-tracking branch 'upstream/master' into nested-union

1e34c44

Add unit test.

5ff5ac2

Merge remote-tracking branch 'upstream/master' into nested-union

c616664

viirya added 2 commits April 1, 2016 06:33

Merge remote-tracking branch 'upstream/master' into nested-union

4d93e9e

Conflicts: sql/catalyst/src/main/antlr3/org/apache/spark/sql/catalyst/parser/SparkSqlParser.g sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/CatalystQlSuite.scala

Add nested union test cases.

6775020

viirya changed the title ~~[SPARK-13321][SQL] Support nested UNION in parser~~ [SPARK-13321][SQL] Add nested union test cases Apr 1, 2016

hvanhovell reviewed Apr 1, 2016
View reviewed changes

viirya closed this Apr 1, 2016

viirya deleted the nested-union branch December 27, 2023 18:33

[SPARK-13321][SQL] Add nested union test cases #11361

[SPARK-13321][SQL] Add nested union test cases #11361

Uh oh!

Conversation

viirya commented Feb 25, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

viirya commented Feb 25, 2016

Uh oh!

SparkQA commented Feb 25, 2016

Uh oh!

viirya commented Feb 25, 2016

Uh oh!

rxin commented Feb 25, 2016

Uh oh!

viirya commented Feb 25, 2016

Uh oh!

viirya commented Feb 25, 2016

Uh oh!

rxin commented Feb 25, 2016

Uh oh!

viirya commented Feb 25, 2016

Uh oh!

hvanhovell commented Feb 26, 2016

Uh oh!

viirya commented Feb 26, 2016

Uh oh!

hvanhovell commented Feb 26, 2016

Uh oh!

viirya commented Apr 1, 2016

Uh oh!

hvanhovell Apr 1, 2016

Choose a reason for hiding this comment

Uh oh!

viirya Apr 1, 2016

Choose a reason for hiding this comment

Uh oh!

viirya Apr 1, 2016

Choose a reason for hiding this comment

Uh oh!

hvanhovell Apr 1, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 1, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants