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/parser: add sequence_name_list and view_name_list #86229

Merged
merged 1 commit into from
Aug 17, 2022

Conversation

ajwerner
Copy link
Contributor

@ajwerner ajwerner commented Aug 16, 2022

This will hopefully make for better diagrams.

Fixes #86059

Release justification: docs only change

Release note: None

@ajwerner ajwerner requested a review from a team as a code owner August 16, 2022 15:37
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ajwerner ajwerner requested a review from a user August 16, 2022 15:37
@ajwerner ajwerner force-pushed the ajwerner/sequence-name-list branch from a7929c0 to d9122b7 Compare August 16, 2022 19:00
@ajwerner ajwerner changed the title sql/parser: add sequence_name_list element for DROP SEQUENCE sql/parser: add sequence_name_list and view_name_list Aug 16, 2022
@ajwerner
Copy link
Contributor Author

I'm not certain that this is an improvement. Maybe it is. Who's our parser guru?

@ajwerner
Copy link
Contributor Author

What I do not understand is why the view and sequence types get nice list names in their diagrams, but the DROP TABLE statement doesn't.

@ajwerner
Copy link
Contributor Author

ajwerner commented Aug 16, 2022

Oh, I figured it out -- it's here

inline: []string{"opt_drop_behavior", "table_name_list"},

I don't know what we want -- should we just not inline table_name_list? Was it nice to inline it? Should I make a list variety for each of the subtypes and then inline all of them? That's probably the most consistent: but also the most ugly.

@ghost
Copy link

ghost commented Aug 16, 2022

Oh, I figured it out -- it's here

inline: []string{"opt_drop_behavior", "table_name_list"},

I don't know what we want -- should we just not inline table_name_list? Was it nice to inline it? Should I make a list variety for each of the subtypes and then inline all of them? That's probably the most consistent: but also the most ugly.

Choosing to inline something is at writer discretion. See https://cockroachlabs.atlassian.net/wiki/spaces/ED/pages/1134166645/SQL+Grammar+Documentation#Add-or-update-a-SQL-diagram I don't know why those choices were made, as it was before my time. You can revert that if you feel it's appropriate.

This will hopefully make for better diagrams.

Fixes cockroachdb#86059

Release note: None
@ajwerner ajwerner force-pushed the ajwerner/sequence-name-list branch from d9122b7 to 45e777b Compare August 16, 2022 19:21
@ajwerner
Copy link
Contributor Author

I've removed the inlining. Whether this is an improvement, I don't know. I'll let you @stbof decide or advise on further refinement.

@ghost
Copy link

ghost commented Aug 16, 2022

I've removed the inlining. Whether this is an improvement, I don't know. I'll let you @stbof decide or advise on further refinement.

I'll build the updated diagrams and see how they look.

@ajwerner
Copy link
Contributor Author

TFTR!

bors r=stbof

@craig
Copy link
Contributor

craig bot commented Aug 17, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 17, 2022

Build succeeded:

@craig craig bot merged commit 79ef820 into cockroachdb:master Aug 17, 2022
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.

Update sql.y to reference sequence_name instead of table_name in DROP SEQUENCE
2 participants