sql/parser: require parens around resource group options#170976
Conversation
|
😎 Merged successfully - details. |
|
This change is |
|
Seems reasonable to me. I originally tried to make the non-parens syntax work to match the outcome brief. But I think consistency with other syntax is more reasonable. Thoughts on whether we should be using this arbitrary option list as opposed to making Weight_CPU and friends keywords? |
The grammar for `with_resource_group_options` previously accepted both `WITH <opt> = <val>, ...` and `WITH (<opt> = <val>, ...)`. Because `BUCKET_COUNT` is in the lexer's `WITH_LA` lookahead set, supporting the unparenthesized form required four grammar alternatives (the `WITH` and `WITH_LA` variants of each), and the generated BNF surfaced this as duplicate `'WITH' ...` lines in `stmt_block.bnf`. PR cockroachdb#170894 made the `Format` methods always emit the parenthesized form, so the unparenthesized grammar paths are no longer needed for the parse/format round-trip. Drop them and require parentheses, matching CRDB convention for arbitrary KV-style option lists (`CREATE TABLE` storage params, `BACKUP ... WITH OPTIONS (...)`, etc.). Resolves: cockroachdb#170788 Release note: None
I think we should keep it as an arbitrary option list, similar to what we do for table storage parameters. The option name validation can still happen at planning time. When we've tried making option names keywords, like for BUCKET_COUNT or some BACKUP options, it generally just adds overhead and causes headaches without much benefit. |
rafiss
requested review from
michae2 and
stevendanna
and removed request for
a team
3 participants
The grammar for
with_resource_group_optionspreviously accepted bothWITH <opt> = <val>, ...andWITH (<opt> = <val>, ...).Because
BUCKET_COUNTis in the lexer'sWITH_LAlookahead set, supporting the unparenthesized form required four grammar alternatives (theWITHandWITH_LAvariants of each), and the generated BNF surfaced this as duplicate'WITH' ...lines instmt_block.bnf.PR #170894 made the
Formatmethods always emit the parenthesized form, so the unparenthesized grammar paths are no longer needed for the parse/format round-trip. Drop them and require parentheses, matching CRDB convention for arbitrary KV-style option lists (CREATE TABLEstorage params,BACKUP ... WITH OPTIONS (...), etc.).Resolves: #170788
Release note: None