Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: add parsing for interleaved & partitioning with CREATE TABLE AS #20943

Closed
wants to merge 1 commit into from

Conversation

danhhz
Copy link
Contributor

@danhhz danhhz commented Dec 20, 2017

CREATE TABLE a (b) INTERLEAVE IN PARENT ... AS ...
CREATE TABLE a (b) PARTITION BY ... AS ...

The interleave/partition clauses could also plausably go after the AS,
but I felt like this was more parallel to CREATE TABLE where they go
right after the column defs. (Plus there's all sorts of grammar
conflicts with putting it after the AS).

Implementation is blocked on #20940, but in the meantime, this will
return a friendly error message.

Also cleans up some unnecessary function parameters that I noticed were
being passed around.

For #20178

Release note: None

    CREATE TABLE a (b) INTERLEAVE IN PARENT ... AS ...
    CREATE TABLE a (b) PARTITION BY ... AS ...

The interleave/partition clauses could also plausably go after the AS,
but I felt like this was more parallel to CREATE TABLE where they go
right after the column defs. (Plus there's all sorts of grammar
conflicts with putting it after the AS).

Implementation is blocked on cockroachdb#20940, but in the meantime, this will
return a friendly error message.

Also cleans up some unnecessary function parameters that I noticed were
being passed around.

For cockroachdb#20178

Release note: None
@danhhz danhhz requested review from jordanlewis, benesch and a team December 20, 2017 19:00
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz
Copy link
Contributor

knz commented Dec 20, 2017

Code apparently OK but ... I don't like the direction.

Has this been thought through properly? CREATE TABLE AS was never meant to be a "serious" feature, because it does not allow one to specify the PK of the newly created table (nor any other constraint / index). Without the ability to specify the PK, I don't see how we can define PARTITION by or INTERLEAVE IN PARENT properly.


Reviewed 9 of 9 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@jordanlewis
Copy link
Member

#20940 tracks adding the ability to specify the PK of the new table.

@danhhz
Copy link
Contributor Author

danhhz commented Dec 20, 2017

@knz Yeah merging this depends on us planning to add the ability to specify PK for CREATE TABLE AS, which is tracked in #20940. I'm not sure how controversial that is and I have no real opinion on it. If we decide we shouldn't add the ability to specify a PK when using CREATE TABLE AS, then I will happily close this PR unmerged.

@bdarnell
Copy link
Contributor

CREATE TABLE AS was never meant to be a "serious" feature

FWIW, it's in the SQL standard. I've found it most useful in conjunction with temporary tables.

@knz
Copy link
Contributor

knz commented Dec 21, 2017

I'll be fully OK with this PR once we change the statement to also support PK/index definitions (a change which I also support). I wasn't aware of the separate issue, thanks Jordan for the link.

@danhhz
Copy link
Contributor Author

danhhz commented Dec 21, 2017

Well I don't think that's happening soon. I was trying to return a better error than a syntax error, but I don't really care that much, so closing this.

@danhhz danhhz closed this Dec 21, 2017
@danhhz danhhz deleted the partition_createas branch December 21, 2017 16:19
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