Skip to content

[SPARK-17590][SQL] Analyze CTE definitions at once and allow CTE subquery to define CTE - #15146

Closed
viirya wants to merge 3 commits into
apache:masterfrom
viirya:cte-analysis-once
Closed

[SPARK-17590][SQL] Analyze CTE definitions at once and allow CTE subquery to define CTE#15146
viirya wants to merge 3 commits into
apache:masterfrom
viirya:cte-analysis-once

Conversation

@viirya

@viiryaviirya commented Sep 19, 2016•
edited
Loading

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

We substitute logical plan with CTE definitions in the analyzer rule CTESubstitution. A CTE definition can be used in the logical plan for multiple times, and its analyzed logical plan should be the same. We should not analyze CTE definitions multiple times when they are reused in the query.

By analyzing CTE definitions before substitution, we can support defining CTE in subquery.

How was this patch tested?

Jenkins tests.

@viiryaviirya changed the title [SPARK-17590][SQL] Analyze CTE definitions at once[SPARK-17590][SQL] Analyze CTE definitions at once and allow CTE subquery to define CTESep 19, 2016
@viirya

viirya commented Sep 19, 2016

Copy link
Copy Markdown
MemberAuthor

cc @hvanhovell@cloud-fan

@SparkQA

SparkQA commented Sep 19, 2016

Copy link
Copy Markdown

Test build #65586 has finished for PR 15146 at commit 394495d.

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

@SparkQA

SparkQA commented Sep 19, 2016

Copy link
Copy Markdown

Test build #65588 has finished for PR 15146 at commit 2f8e79f.

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

cloud-fan
cloud-fan reviewed Sep 19, 2016
View reviewed changes
Comment threadsql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

namedQuery
: name=identifier AS? '(' queryNoWith ')'
: name=identifier AS? '(' query ')'

@cloud-fancloud-fanSep 19, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you explain more about this grammar change?

@viiryaviiryaSep 19, 2016

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Currently we only allow a query without With used in the CTE's named query. This change loose this limit to allow a query with With to be a named query. So we can define CTE in CTE subquery.

@viiryaviiryaSep 19, 2016•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

BTW, query is defined as "ctes? queryNoWith".

cloud-fan
cloud-fan reviewed Sep 19, 2016
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala Outdated

test("define CTE in CTE subquery") {
checkAnswer(
sql("with t2 as (with t1 as (select 1 as b, 2 as c) select b, c from t1) " +

@cloud-fancloud-fanSep 19, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we use multi-line string with proper ident here? this test is almost unreadable...

@hvanhovellhvanhovellSep 19, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1

@cloud-fan

cloud-fan commented Sep 19, 2016

Copy link
Copy Markdown
Contributor

Seems good, but not quite sure, cc @hvanhovell to confirm

hvanhovell
hvanhovell approved these changes Sep 19, 2016
View reviewed changes

@hvanhovellhvanhovell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Comment threadsql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala Outdated

test("define CTE in CTE subquery") {
checkAnswer(
sql("with t2 as (with t1 as (select 1 as b, 2 as c) select b, c from t1) " +

@hvanhovellhvanhovellSep 19, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1

@hvanhovell

hvanhovell commented Sep 19, 2016

Copy link
Copy Markdown
Contributor

@viirya does this improve performance?

@viirya

viirya commented Sep 20, 2016

Copy link
Copy Markdown
MemberAuthor

@hvanhovell This is for analyzer change and adds CTE in CTE feature. I don't expect there is performance improvement.

@viirya

viirya commented Sep 20, 2016

Copy link
Copy Markdown
MemberAuthor

I guess if using the same analyzed plan increases the chance to reuse exchange, then it may improve the performance. Anyway, it is not the purpose of this change. Because the analyzed subquery plan will be changed largely in optimization later, we cannot guarantee this.

@SparkQA

SparkQA commented Sep 20, 2016

Copy link
Copy Markdown

Test build #65630 has finished for PR 15146 at commit baf239b.

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

@hvanhovell

hvanhovell commented Sep 21, 2016

Copy link
Copy Markdown
Contributor

Merging to master. Thanks!

@viirya

viirya commented Sep 22, 2016

Copy link
Copy Markdown
MemberAuthor

@hvanhovell@cloud-fan Thanks!

@gbloisigbloisi mentioned this pull request Feb 10, 2019
Closed
@viirya
viirya deleted the cte-analysis-once branch December 27, 2023 18:19
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cloud-fancloud-fancloud-fan left review comments
@hvanhovellhvanhovellhvanhovell approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@viirya@SparkQA@cloud-fan@hvanhovell