[SPARK-28002][SQL] Support WITH clause column aliases #24842
Conversation
|
@dongjoon-hyun, @gatorsmile this is another feature that PostgreSQL does support. |
|
ok to test |
|
LGTM pending Jenkins, thanks! |
|
In fact, I was thinking, instead of adding test cases in the The changes in this PR are purely about the parsing phase. Running full-blown SQL queries for testing parsing changes is slow and unnecessary. The Spark PR builder already takes a long time to finish, would be better to cut the cost whenever possible. |
|
Test build #106393 has finished for PR 24842 at commit
|
|
Thanks @liancheng for the review. I've moved analysis error test cases to BTW there is another improvement PR regarding |
|
Test build #106405 has finished for PR 24842 at commit
|
|
@peter-toth can you still add a test to the plan parser suite? |
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisSuite.scala
Outdated
Show resolved
Hide resolved
|
Thank you for ping me, @peter-toth . +1 for the above test case comments. And, this PR also looks good to me. |
There was a problem hiding this comment.
Oh, please move this to cte.sql. That is a perfect place for this.
cc @gatorsmile
@hvanhovell sure, I added a test to |
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisSuite.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisSuite.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala
Outdated
Show resolved
Hide resolved
|
Test build #106438 has finished for PR 24842 at commit
|
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala
Outdated
Show resolved
Hide resolved
|
Test build #106456 has finished for PR 24842 at commit
|
|
Test build #106458 has finished for PR 24842 at commit
|
|
retest this please |
| CROSS JOIN CTE1 t2; | ||
|
|
||
| -- CTE with column alias | ||
| WITH t(x) AS (SELECT 1) |
There was a problem hiding this comment.
Could you add more test cases? For example, can a with clause in a subquery shadow a with clause in an enclosing query with the same name? Another example, use with clauses in a subquery expression?
There was a problem hiding this comment.
I wanted to focus on column aliases in this PR, but I have another open here which focuses on nested WITH clauses: #24831
There was a problem hiding this comment.
Yep. For the nested WITH, #24831 would be a better place to add them.
After merging this, let's proceed as @gatorsmile suggested, @peter-toth .
| interceptParseException(parsePlan)(sqlCommand, messages: _*) | ||
|
|
||
| private def cte(plan: LogicalPlan, namedPlans: (String, LogicalPlan)*): With = { | ||
| private def cte(plan: LogicalPlan, namedPlans: (String, (LogicalPlan, Seq[String]))*): With = { |
There was a problem hiding this comment.
Thank you for consolidating both cte functions into one!
There was a problem hiding this comment.
+1, LGTM.
@gatorsmile . Could you do the final review and sign-off this? For the nested WITH, I hope we can do that in his another PRs.
|
Retest this please. |
|
Test build #106523 has finished for PR 24842 at commit
|
|
Retest this please. |
|
Test build #106533 has finished for PR 24842 at commit
|
|
Merged to master. Thank you so much, @peter-toth , @liancheng, @hvanhovell , @gatorsmile ! @peter-toth . Please proceed to #24831 and #24860 . |
## What changes were proposed in this pull request? This PR adds support of column aliasing in a CTE so this query becomes valid: ``` WITH t(x) AS (SELECT 1) SELECT * FROM t WHERE x = 1 ``` ## How was this patch tested? Added new UTs. Closes apache#24842 from peter-toth/SPARK-28002. Authored-by: Peter Toth <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>
|
Thanks @dongjoon-hyun @liancheng @gatorsmile @hvanhovell for the review. I have prepared #24831 for review. |
|
Thanks. No problem, @peter-toth ! |
|
|
||
| namedQuery | ||
| : name=identifier AS? '(' query ')' | ||
| : name=identifier (columnAliases=identifierList)? AS? '(' query ')' |
There was a problem hiding this comment.
Duplicate names within a single CTE definition are not allowed.
| -- !query 7 | ||
| DROP VIEW IF EXISTS t | ||
| WITH t(x) AS (SELECT 1) | ||
| SELECT * FROM t WHERE x = 1 |
There was a problem hiding this comment.
Almost all the test cases are using one column in the CTE definition. Can you try your best to improve the current test coverage in this new syntax?
|
Sorry for my late response. @peter-toth @dongjoon-hyun Could you submit a follow-up PR to improve it? |
|
@gatorsmile @dongjoon-hyun I opened #24949 to add some new test cases. Please let me know if you want more cases. Please note that I'm also working on #24860 and it will add many new tests that cover WITH column aliases. |
6 participants
What changes were proposed in this pull request?
This PR adds support of column aliasing in a CTE so this query becomes valid:
How was this patch tested?
Added new UTs.