[SPARK-16771][SQL] WITH clause should not fall into infinite loop. by dongjoon-hyun · Pull Request #14397 · apache/spark

dongjoon-hyun · 2016-07-29T05:00:45Z

What changes were proposed in this pull request?

This PR changes the CTE resolving rule to use only forward-declared tables in order to prevent infinite loops. More specifically, new logic is like the following.

Resolve CTEs in WITH clauses first before replacing the main SQL body.
When resolving CTEs, only forward-declared CTEs or base tables are referenced.
- Self-referencing is not allowed any more.
- Cross-referencing is not allowed any more.

Reported Error Scenarios

scala> sql("WITH t AS (SELECT 1 FROM t) SELECT * FROM t")
java.lang.StackOverflowError
...
scala> sql("WITH t1 AS (SELECT * FROM t2), t2 AS (SELECT 2 FROM t1) SELECT * FROM t1, t2")
java.lang.StackOverflowError
...

Note that t, t1, and t2 are not declared in database. Spark falls into infinite loops before resolving table names.

How was this patch tested?

Pass the Jenkins tests with new two testcases.

SparkQA · 2016-07-29T06:50:22Z

Test build #62995 has finished for PR 14397 at commit 5bca528.

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

dongjoon-hyun · 2016-07-29T15:20:11Z

Hi, @rxin .
Could you review this PR?

hvanhovell · 2016-07-31T22:26:42Z

@dongjoon-hyun I think this has merit. I do have one question, what do other databases do? Like postgresql, mysql, sqlserver and others?

hvanhovell · 2016-07-31T22:31:23Z

This integrates some of the functionality of the ResolveRelations rule, but not all (for instance file based datasources). Shouldn't we be consistent and (perhaps) integrate these rules?

dongjoon-hyun · 2016-08-01T02:14:54Z

Thank you for review, @hvanhovell .

For the recursive CTE queries, traditional DBMS supports optional RECURSIVE keyword which computing closures, i.e., it iterates the subquery until the result converge. For example, PostgreSQL, MSSQL, SQLite supports WITH RECURSIVE syntax officially.

For the overlap of ResolveRelations rule, this rule calls lookupRelation to check the name conflict between the base relations and CTEs. As a side effect, it resolves the relations, too. Here is the reasons.

To check the conflict, this is the only simplest way.
For the performance, this patch only call one lookupRelation for UnresolvedRelation.
- ResolveRelations rule do not try resolve again and will not complain about that.
The remaining general and exceptional rules like file-based datasource should be in ResolveRelations. We can add more datasources there, not here.

For Hive, the recursive queries and RECURSIVE syntax are not supported.

hvanhovell · 2016-08-01T15:28:22Z

@dongjoon-hyun

New behavior versus existing systems

I was not talking about recursive CTE's (which can be very useful in some cases). We are changing the behavior of the Analyzer, and this might surprise users. I would like to know if there is a common approach to this among other systems; so we can justify the change in behavior.

Resolve Relations

The change you propose is almost subsuming the ResolveRelations rule. I think it is correct to to do this. However I am not a big fan of duplicating the behavior in two rules. Do you think there is way to get around this? Could we for instance centralize the logic in ResolveRelations?

dongjoon-hyun · 2016-08-01T20:13:00Z

Oh, sorry for misunderstanding, @hvanhovell . I think we can integrate ResolveRelation and this one. I'll be back after some testing in this evening. Also for the common approach, I will bring some references, too.

dongjoon-hyun · 2016-08-02T06:27:51Z

Hi, @hvanhovell . It seems not clearly documented, so I did some comparisons.

First of all, in the main SQL body, CTE query names are used first. However, the table resolution in the main SQL body is also dependent on the CTE subqueries. The general Analyzer approach seems to

use forward-declared(not self) base table or CTE name
- Especially, in WITH clauses, it depends on CTE orders.
prevent the execution of cyclic queries by raising exceptions.
- Hive/Oracle raise exceptions. PostgreSQL uses the base tables if they exist.

The root cause of previous Spark problems is using self or backward-declared CTE for table name resolutions. I will try to revise this PR according to the above rules. Please comment about the above rule.

dongjoon-hyun · 2016-08-02T06:49:55Z

The above approach also can remove the duplicated scope issue between CTESubstitution and ResolveRelation.

dongjoon-hyun · 2016-08-08T18:03:56Z

Hi, @hvanhovell .
Sorry for late update. I updated the PR description and code.
Could you review this PR again?

SparkQA · 2016-08-08T19:46:12Z

Test build #63371 has finished for PR 14397 at commit b10ee10.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class With(child: LogicalPlan, cteRelations: Seq[(String, SubqueryAlias)]) extends UnaryNode

dongjoon-hyun · 2016-08-09T22:02:35Z

Could you review this PR again, @hvanhovell ?
Last time, I did a huge mistake, but now I think I fixed correct according to your advice.

dongjoon-hyun · 2016-08-09T22:04:54Z

Here, the CTEs of WITH clause are resolved sequentially before applying to main SQL body.

hvanhovell · 2016-08-09T22:09:37Z

@dongjoon-hyun I'll take a look in the morning (CET)

dongjoon-hyun · 2016-08-09T22:12:07Z

Thank you so much! Then, see you later. :)

hvanhovell · 2016-08-10T09:11:25Z

relations is a map. The iteration sequence of a map does not need the be the ordered in which elements were added (Use a map larger than 4 to see this in action, e.g.: Seq.tabulate(5)(i => ('a' + i).toChar.toString -> i).toMap).
So I think we need to change the data type in With to accommodate this.

So, I changed it to Seq. Now, relations of With is a sequence.

Never mind you have already done that :)

This does not replace With in all cases. Could you remove the relations.nonEmpty guard?

If WITH without relations, I thought we can skip all since there is nothing to be replaced.

Yeah you are right about that. But the current code does not remove the With node if there are no relations.

hvanhovell · 2016-08-10T09:21:09Z

@dongjoon-hyun this is looking very promising. I left two small comments.

dongjoon-hyun · 2016-08-10T09:21:56Z

Thank you, @hvanhovell !

hvanhovell · 2016-08-10T11:14:34Z

resolved :+ r._1 -> ResolveRelations(substituteCTE(r._2, resolved))?
You could also deconstruct the r tuple for easier reading.

hvanhovell · 2016-08-10T11:49:41Z

LGTM pending Jenkins.

SparkQA · 2016-08-10T12:34:29Z

Test build #63526 has finished for PR 14397 at commit d896afc.

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

SparkQA · 2016-08-10T13:30:49Z

Test build #63529 has finished for PR 14397 at commit f3a2cd4.

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

dongjoon-hyun · 2016-08-11T04:11:43Z

Rebased just to resolve conflicts.

SparkQA · 2016-08-11T06:12:58Z

Test build #63577 has finished for PR 14397 at commit 624bb3d.

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

SparkQA · 2016-08-11T06:13:39Z

Test build #63578 has finished for PR 14397 at commit 178813e.

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

rxin · 2016-08-11T06:23:33Z

can you create a test in SQLQueryTestSuite instead?

Sure. I'll move this.

Ur, @rxin .
SQLQueryTestSuite seems not to support exceptions cases yet.

In this PR, exceptions are important and should be checked.

dongjoon-hyun · 2016-08-11T07:39:40Z

Oh, now Exception is supported at 0db373a. Thanks, @rxin . I will try again.

dongjoon-hyun · 2016-08-11T08:05:47Z

Hi, @rxin .
Now the testcases are moved into sql-tests with SQLQueryTestSuite.

SparkQA · 2016-08-11T10:13:21Z

Test build #63595 has finished for PR 14397 at commit 7f74ec7.

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

dongjoon-hyun · 2016-08-11T20:59:07Z

Hi, @rxin . I moved the testcase into new suite.
Please let me know if there is more thing to do.
New SQL test suite is great!

rxin · 2016-08-11T21:11:21Z

@@ -0,0 +1,14 @@
+create temporary view t as select * from values 0, 1, 2 as t(id);


maybe CTE instead of "with" ?

Here, t and t2 is base tables.
The followings are used CTEs and to check if base table or previous CTE is used correctly.

Ah, I see. You mean with.sql into CTE.sql.
I see. No problem.

I will use cte.sql instead of with.sql.

dongjoon-hyun · 2016-08-11T21:20:58Z

Thank you, @rxin . It's updated.

SparkQA · 2016-08-11T23:29:35Z

Test build #63637 has finished for PR 14397 at commit f1f4a83.

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

dongjoon-hyun · 2016-08-12T04:53:10Z

Hi, @hvanhovell .
Could you review this again? After the last time you've seen, only the testcase is moved into a new testsuite.

hvanhovell · 2016-08-12T17:04:01Z

LGTM (again) - merging to master. Thanks!

dongjoon-hyun · 2016-08-12T17:07:37Z

Thank you, @hvanhovell !

hvanhovell reviewed Jul 31, 2016
View reviewed changes

dongjoon-hyun reviewed Aug 9, 2016
View reviewed changes

hvanhovell reviewed Aug 10, 2016
View reviewed changes

rxin reviewed Aug 11, 2016
View reviewed changes

dongjoon-hyun added 5 commits August 11, 2016 00:41

[SPARK-16771][SQL] WITH clause should not fall into infinite loop.
64b81fc

Address comments.
788e136

WITH clause should be removed.
fd27b04

Remove testcase removed from master.
ba45b1b

Move testcase into sql-tests
7f74ec7

rxin reviewed Aug 11, 2016
View reviewed changes

Rename files.
f1f4a83

asfgit closed this in 2a10513Aug 12, 2016

dongjoon-hyun deleted the SPARK-16771-TREENODE branch January 17, 2018 09:41

		@@ -0,0 +1,14 @@
		create temporary view t as select * from values 0, 1, 2 as t(id);

Uh oh!

Conversation

dongjoon-hyun commented Jul 29, 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!

SparkQA commented Jul 29, 2016

Uh oh!

dongjoon-hyun commented Jul 29, 2016

Uh oh!

hvanhovell commented Jul 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Aug 1, 2016

Uh oh!

hvanhovell commented Aug 1, 2016

New behavior versus existing systems

Resolve Relations

Uh oh!

dongjoon-hyun commented Aug 1, 2016

Uh oh!

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

Uh oh!

Uh oh!

dongjoon-hyun commented Aug 2, 2016

Uh oh!

dongjoon-hyun commented Aug 8, 2016

Uh oh!

SparkQA commented Aug 8, 2016

Uh oh!

dongjoon-hyun commented Aug 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvanhovell commented Aug 9, 2016

Uh oh!

dongjoon-hyun commented Aug 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvanhovell commented Aug 10, 2016

Uh oh!

dongjoon-hyun commented Aug 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvanhovell commented Aug 10, 2016

Uh oh!

SparkQA commented Aug 10, 2016

Uh oh!

SparkQA commented Aug 10, 2016

Uh oh!

dongjoon-hyun commented Aug 11, 2016

Uh oh!

SparkQA commented Aug 11, 2016

Uh oh!

SparkQA commented Aug 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jul 29, 2016•
edited
Loading

dongjoon-hyun commented Aug 2, 2016•
edited
Loading

dongjoon-hyunAug 11, 2016•
edited
Loading