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

parser: add QUERY to bare_label_keyword #97041

Merged
merged 1 commit into from
Feb 15, 2023

Conversation

ZhouXing19
Copy link
Collaborator

@ZhouXing19 ZhouXing19 commented Feb 13, 2023

As revealed by #84309, some keywords not added to bare_label_keywords in sql.y make the SELECT ... <keyword> statement error out, which is not compatible with postgres. This commit is to add the QUERY keyword per a support ticket. We're not adding the whole unreserved_keyword list here as having some of them in bare_label_keyword, such as DAY, brings reduce errors.

Postgres:

postgres=# select substring('stringstringstring',1,10) query;
   query
------------
 stringstri
(1 row)

CRDB:

root@localhost:26257/defaultdb> select substring('stringstringstring',1,10)  query;
invalid syntax: statement ignored: at or near "query": syntax error
SQLSTATE: 42601
DETAIL: source SQL:
select substring('stringstringstring',1,10)  query
                                             ^

informs #84309

Release Note(bug fix): fixed the syntax error for SELECT ... QUERY (without AS) statement.

@ZhouXing19 ZhouXing19 requested a review from a team as a code owner February 13, 2023 15:18
@blathers-crl
Copy link

blathers-crl bot commented Feb 13, 2023

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

Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

the code lgtm! just had a request to comment

@@ -16374,6 +16374,7 @@ bare_label_keywords:
| INVOKER
| LEAKPROOF
| PARALLEL
| QUERY
Copy link
Collaborator

Choose a reason for hiding this comment

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

we discussed offline, but could you adjust the comment a bit for bare_label_keywords and the other rules above and below? i think the information is a bit outdated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just chatted with @chengxiong-ruan and have it updated. Basically we still want a keyword to be only in one of those "_keyword" lists (note, bare_labeled_keywords is not one of them), but newly added keyword should always be added to bare_labeled_keywords as well.

As revealed by cockroachdb#84309, some keywords not added to `bare_label_keywords` in
`sql.y` will make the `SELECT ... <keyword>` statement error out, which is not
compatible with postgres. This commit is to add the `QUERY` keyword per a
support ticket. We're not adding the whole `unreserved_keyword` list here as
having some of them in `bare_label_keyword`, such as `DAY`, bring `reduce`
errors.

Postgres:

```
postgres=# select substring('stringstringstring',1,10) query;
   query
------------
 stringstri
(1 row)
```

CRDB:

```
root@localhost:26257/defaultdb> select substring('stringstringstring',1,10)  query;
invalid syntax: statement ignored: at or near "query": syntax error
SQLSTATE: 42601
DETAIL: source SQL:
select substring('stringstringstring',1,10)  query
                                             ^
```

informs cockroachdb#84309

Release Note(bug fix): fixed the syntax error for `SELECT ... QUERY` (without AS)
statement.
Copy link
Contributor

@chengxiong-ruan chengxiong-ruan left a comment

Choose a reason for hiding this comment

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

LGTM!

@ZhouXing19
Copy link
Collaborator Author

Failed test in the CI is irrelevant.

Thanks!

bors r+

@craig
Copy link
Contributor

craig bot commented Feb 15, 2023

Build succeeded:

@craig craig bot merged commit 3c4d3ef into cockroachdb:master Feb 15, 2023
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