Skip to content

sql/parser: remove no longer used keywords #147386

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

Merged
merged 2 commits into from
May 29, 2025

Conversation

yuzefovich
Copy link
Member

@yuzefovich yuzefovich commented May 28, 2025

plpgsql/parser: rename bazel target name for the package

For some reason, Go library bazel target for the PLpgSQL parser package
was named plpgparser. This creates a problem with an attempt to add
a test that lives in parser (as opposed to parser_test), so rename
the target to parser which follows the convention elsewhere. (I
eventually decided against adding a test but thought it'd be nice to
clean things up.)

sql/parser: remove no longer used keywords

This commit removes a handful of unreserved keywords that are no longer
used. Additionally, it refactors existing reserved_keywords.awk into
a shell script that is run via a unit test to assert that no longer used
keywords get removed promptly. (I tried applying the same script to
PLpgSQL and JSONPath parsers, and it found nothing for the latter and
produced many false positives for the former, so I decided to not
include the corresponding test.)

Epic: None
Release note: None

Copy link

blathers-crl bot commented May 28, 2025

It looks like your PR touches SQL parser code but doesn't add or edit parser tests. Please make sure you add or edit parser tests if you edit the parser.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@yuzefovich yuzefovich force-pushed the parser-unused-keywords branch from e7100fc to d576148 Compare May 28, 2025 01:12
For some reason, Go library bazel target for the PLpgSQL parser package
was named `plpgparser`. This creates a problem with an attempt to add
a test that lives in `parser` (as opposed to `parser_test`), so rename
the target to `parser` which follows the convention elsewhere. (I
eventually decided against adding a test but thought it'd be nice to
clean things up.)

Release note: None
This commit removes a handful of unreserved keywords that are no longer
used. Additionally, it refactors existing `reserved_keywords.awk` into
a shell script that is run via a unit test to assert that no longer used
keywords get removed promptly. (I tried applying the same script to
PLpgSQL and JSONPath parsers, and it found nothing for the latter and
produced many false positives for the former, so I decided to not
include the corresponding test.)

Release note: None
@yuzefovich yuzefovich force-pushed the parser-unused-keywords branch from d576148 to f50a1b4 Compare May 28, 2025 03:13
@yuzefovich yuzefovich changed the title sql: add testing for unused keywords in grammars sql/parser: remove no longer used keywords May 28, 2025
@yuzefovich yuzefovich marked this pull request as ready for review May 28, 2025 16:13
@yuzefovich yuzefovich requested review from a team as code owners May 28, 2025 16:13
@yuzefovich yuzefovich requested review from mw5h and DrewKimball and removed request for a team and mw5h May 28, 2025 16:13
Copy link
Collaborator

@DrewKimball DrewKimball left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 10 of 10 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)

@yuzefovich
Copy link
Member Author

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented May 29, 2025

@craig craig bot merged commit a96b522 into cockroachdb:master May 29, 2025
22 checks passed
@yuzefovich yuzefovich deleted the parser-unused-keywords branch May 29, 2025 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants