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: compat: Support 'COMMENT ON TABLE foo is 'bar' parsing #21063

Merged
merged 1 commit into from
Dec 29, 2017

Conversation

ZacharyThomas
Copy link

Many ORMs use Postgres' COMMENT statement to label database objects. This commit
handles the grammar changes necessary to support the COMMENT syntax for
Columns and Tables. The implementaiton of COMMENT is left for a future PR.

Release note (sql change): allow parsing of COMMENT ON syntax

This touches on #19472

@ZacharyThomas ZacharyThomas requested a review from a team December 27, 2017 16:12
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@CLAassistant
Copy link

CLAassistant commented Dec 27, 2017

CLA assistant check
All committers have signed the CLA.

@knz
Copy link
Contributor

knz commented Dec 27, 2017

Thanks! since the code is still empty (and would cause an error) I would recommend leaving out the comments that provide contextual help.


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


pkg/sql/parser/sql.y, line 1653 at r1 (raw file):

  COMMENT ON TABLE any_name IS comment_text
  {
    return unimplemented(sqllex, "COMMENT ON TABLE unimplemented")

use unimplementedWithIssue() and link to the issue you're touching.


pkg/sql/parser/sql.y, line 1661 at r1 (raw file):

comment_text:
			SCONST							{ $$ = $1 }

We use spaces not tabs.


Comments from Reviewable

@knz
Copy link
Contributor

knz commented Dec 27, 2017

also use the text /* SKIP DOC */ for them (see the other syntax bits that already do that)


Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


Comments from Reviewable

@knz
Copy link
Contributor

knz commented Dec 27, 2017

Thank you!
This really looks good. Although since your entire PR is about just one concern, I would recommend you squash all the commits into one before we can merge this.


Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks pending.


pkg/sql/parser/sql.y, line 1647 at r2 (raw file):

comment_stmt:
  /* SKIP DOC */

This comment must be placed inside the action, ie. between the braces { ... }


pkg/sql/parser/sql.y, line 1652 at r2 (raw file):

    return unimplementedWithIssue(sqllex, 19472)
  }
  /* SKIP DOC */

ditto


Comments from Reviewable

@knz
Copy link
Contributor

knz commented Dec 28, 2017

@mjibson this PR is failing CI due to some incorrect bnf file. Can you advise Zachary how to re-generate this file? I cannot seem to find any documentation about it.


Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


Comments from Reviewable

@knz
Copy link
Contributor

knz commented Dec 28, 2017

@ZacharyThomas your PR seems ready to me but CI is saying something is wrong. Unfortunately this is a new lint error which I am not familiar with. My colleague who introduced this check earlier is out on vacation and will only be back next Wednesday. Are you ok investigating this on your own or waiting until next week?

@knz
Copy link
Contributor

knz commented Dec 28, 2017

Filed the cause of the CI error as #21081.


Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


Comments from Reviewable

@knz
Copy link
Contributor

knz commented Dec 28, 2017

@ZacharyThomas my colleague Jordan explained what you need to do here:

  make generate PKG=./docs/...

then commit the changed files. Then we can merge your change.

Thanks!

Many ORMs use Postgres' COMMENT statement to label database objects. This commit
handles the grammar changes necessary to support the COMMENT syntax for
Columns and Tables. The implementaiton of COMMENT is left for a future PR.

Release note (sql change): allow parsing of COMMENT ON syntax
@ZacharyThomas
Copy link
Author

I was super confused so thanks for that! Hopefully this solves it.

@knz
Copy link
Contributor

knz commented Dec 29, 2017

yes, perfect. Thank you again!

@knz knz merged commit 1a3cb64 into cockroachdb:master Dec 29, 2017
@ZacharyThomas
Copy link
Author

TFTR!

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.

4 participants