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.

Sign up for GitHub

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

Jump to bottom

sql: automatic way to ensure new sql keyword is added to bare_label_keywords list #84309

Closed
chengxiong-ruan opened this issue Jul 12, 2022 · 4 comments · Fixed by #97878
Closed

sql: automatic way to ensure new sql keyword is added to bare_label_keywords list #84309

chengxiong-ruan opened this issue Jul 12, 2022 · 4 comments · Fixed by #97878
Labels
A-sql-syntax Issues strictly related to the SQL grammar, with no semantic aspect C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@chengxiong-ruan
Copy link
Contributor

chengxiong-ruan commented Jul 12, 2022 •
edited by cockroach-jira-scripts
Loading

Describe the solution you'd like
Adding new keyword could break user queries which use column labels without AS. We added the bare_label_keywords list to allow those keywords to be used as column labels. However, it's easy to forget to add the keyword. We need to an automatic way to validate all newly added keywords are added to this list.

We could auto generate code to validate that, for example we're already doing similar thing when generating this list. The hard part could be how do we know a keyword is newly added? One alternative is to add all keywords (including existing ones) to this list, and the automatic check just need to make sure all keywords are added.

Jira issue: CRDB-17582
@chengxiong-ruan chengxiong-ruan added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-sql-syntax Issues strictly related to the SQL grammar, with no semantic aspect labels Jul 12, 2022
@chengxiong-ruan chengxiong-ruan self-assigned this Jul 12, 2022
@blathers-crl blathers-crl bot added the T-sql-schema-deprecated Use T-sql-foundations instead label Jul 12, 2022
@chengxiong-ruan
Copy link
Contributor Author

I don't know which project I should add to this issue...so I just attached my team.

@chengxiong-ruan chengxiong-ruan mentioned this issue Jul 12, 2022
Merged
@rafiss
Copy link
Collaborator

rafiss commented Jul 26, 2022

Thanks for the issue. What's the impact of forgetting to add the keyword to bare_label_keywords?

@chengxiong-ruan
Copy link
Contributor Author

@rafiss adding new keyword without adding to bare_label_keywords could break user query if the query use column label without AS. Before bare_label_keyword was added, any new keyword could break user queries because it was expecting an IDENT token, but if you used a keyword as the column label, the parser would fail to get an IDENT and error out :(
I think it could be quite rare... but I encountered one case when I was adding the new keyword INPUT which broke this test.

@chengxiong-ruan chengxiong-ruan removed their assignment Sep 22, 2022
@michae2
Copy link
Collaborator

michae2 commented Feb 9, 2023

Maybe some kind of test that automatically tries to parse SELECT 1 <keyword> with every (unreserved) keyword would suffice?

@ZhouXing19 ZhouXing19 mentioned this issue Feb 13, 2023
Merged
ZhouXing19 added a commit to ZhouXing19/cockroach that referenced this issue Feb 13, 2023
@ZhouXing19
parser: add QUERY to bare_label_keyword
9814778 
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.
ZhouXing19 added a commit to ZhouXing19/cockroach that referenced this issue Feb 14, 2023
@ZhouXing19
parser: add QUERY to bare_label_keyword
db011d3 
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.
craig bot pushed a commit that referenced this issue Feb 15, 2023
@ZhouXing19 @RaduBerinde @andyyang890
Merge #96295 #97041 #97134 #97149
3c4d3ef 
96295: changefeedccl: Rename and remove CDC functions r=miretskiy a=miretskiy

This PR renames and removes CDC specific functions, while maintaining backward compatability.

* `cdc_is_delete()` function removed.  It is replaced
  with `event_op()` function which returns a string describing
  the type of event.  If the changefeed is running with `diff`
  option, then this function returns `insert`, `update`, or
  `delete`.  If changefeed is running without the `diff` option,
  we can't tell an update from insert, so this function returns
  `upsert` or `delete`.
* `cdc_mvcc_timestamp()` function removed.  This information can be accessed 
    via cockroach standard system column `crdb_internal_mvcc_timestamp`.  
    The same timestamp column is avaiable in the previous row state 
    `(cdc_prev).crdb_internal_mvcc_timestamp`
* `cdc_updated_timestamp()` function renamed as `event_schema_timestamp()`

Fixes #92482
Epic: CRDB-17161

Release note (enterprise change): Deprecate and replace some of the changefeed specific functions made available in preview mode in 22.2
release.   While these functions are deprecated, old changefeed
transformations should continue to function properly.
Customers are encouraged to closely monitor their changefeeds
created during upgrade.  While effort was made to maintain backward
compatibility, the updated changefeed transformation may produce
slightly different output (different column names, etc).

97041: parser: add `QUERY` to `bare_label_keyword` r=ZhouXing19 a=ZhouXing19

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.

97134: storage: include version in benchmark fixture directory r=RaduBerinde a=RaduBerinde

This benchmark is sometimes failing in CI and I cannot reproduce locally. The error is consistent with a leftover fixture from an older version (which shouldn't happen because CI should be cleaning up the repo).

This change adds the version to the fixture name so that this kind of cross-version problem cannot occur. This is a good idea regardless of the CI issue.

Informs #97061

Release note: None
Epic: none

97149: sql: add missing STORING clause in system.privileges user ID migration r=rafiss a=andyyang890

This patch fixes a discrepancy between the bootstrap schema and user ID
migration for the system.privileges table.

Part of #87079

Release note: None

Co-authored-by: Yevgeniy Miretskiy <[email protected]>
Co-authored-by: Jane Xing <[email protected]>
Co-authored-by: Radu Berinde <[email protected]>
Co-authored-by: Andy Yang <[email protected]>
@blathers-crl blathers-crl bot mentioned this issue Feb 15, 2023
Merged
blathers-crl bot pushed a commit that referenced this issue Feb 15, 2023
@ZhouXing19
parser: add QUERY to bare_label_keyword
acfbf02 
As revealed by #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 #84309

Release Note(bug fix): fixed the syntax error for `SELECT ... QUERY` (without AS)
statement.
@rafiss rafiss mentioned this issue Mar 1, 2023
Merged
@craig craig bot closed this as completed in 3036617 Mar 2, 2023
@exalate-issue-sync exalate-issue-sync bot added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) and removed T-sql-schema-deprecated Use T-sql-foundations instead labels May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-sql-syntax Issues strictly related to the SQL grammar, with no semantic aspect C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

parser: add test to check that bare_label_keywords is updated rafiss/cockroach
3 participants
@rafiss @chengxiong-ruan @michae2