Skip to content

Conversation

@cloud-fan
Copy link
Contributor

What changes were proposed in this pull request?

This is a regression caused by #24150

select * from (from a select * select *) is supported in 2.4, and we should keep supporting it.

This PR merges the parser rule for single and multi select statements, as they are very similar.

How was this patch tested?

a new test case


insertStatement
: (ctes)? insertInto queryTerm queryOrganization #singleInsertQuery
| (ctes)? fromClause multiInsertQueryBody+ #multiInsertQuery
Copy link
Contributor Author

Choose a reason for hiding this comment

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

move the ctes? to the parent entry, to save duplicated code. This is a simplification of #24150

@cloud-fan
Copy link
Contributor Author

cc @dilipbiswal

queryNoWith
: queryTerm queryOrganization #noWithQuery
| fromClause selectStatement #queryWithFrom
| fromClause selectStatement+ #queryWithFrom
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should we name the label #queriesWithFrom ? I am fine if we stay as is :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, multi-select is still one query :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@cloud-fan Okay.. sounds good to me.

@dilipbiswal
Copy link
Contributor

@cloud-fan Thanks a LOT for fixing it.
LGTM

table("a").select(star()).insertInto("tbl1").union(
table("a").where('s < 10).select(star()).insertInto("tbl2")))
assertEqual(
"select * from (from a select * select *)",
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for adding this.

@SparkQA
Copy link

SparkQA commented Apr 11, 2019

Test build #104515 has finished for PR 24348 at commit b35cabf.

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

@cloud-fan
Copy link
Contributor Author

thanks, merging to master!

@cloud-fan cloud-fan closed this in 0407070 Apr 12, 2019
intercept(
"from a select * select * from x where a.s < 10",
"Multi-select queries cannot have a FROM clause in their individual SELECT statements")
"This select statement can not have FROM cause as its already specified upfront")
Copy link
Member

Choose a reason for hiding this comment

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

you know what? cannot is correct too and more usual :-).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

it's fine. I didn't mean we should fix now. just wanted to say it :-).

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