sql: TestRandomSyntaxGeneration fixes #98821
Conversation
|
This change is |
There was a problem hiding this comment.
Looks like the last commit is causing issues. I think it's finding other bugs and possibly the 30s timeout is not graceful so that tests always fail. Feel free to back that commit out and follow it up later if you want to merge the first two commits.
Reviewed 2 of 2 files at r1, 1 of 1 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @michae2)
-- commits line 32 at r2:
Can you add a parser test like this?
-- commits line 38 at r2:
Can you also add a parser test like this?
There was a problem hiding this comment.
Reviewable status:
Previously, mgartner (Marcus Gartner) wrote…
Can you add a parser test like this?
A test that shows the parser fails to parse this? Not sure I see the point, but I do agree a regression test would be nice, I'll look for a place to put a format regression test.
|
I removed the CI bit, I'll put that on ice until all the bugs are fixed. |
There was a problem hiding this comment.
Reviewable status:
Previously, cucaroach (Tommy Reilly) wrote…
A test that shows the parser fails to parse this? Not sure I see the point, but I do agree a regression test would be nice, I'll look for a place to put a format regression test.
Nevermind, I didn't realize the parse tests ARE hitting the Format path, tests for these two cases added.
|
bors r+ |
|
👎 Rejected by too few approved reviews |
|
oh, still needs a |
The RSG works by calling format on the AST's it generates so its good
at finding Format bugs.
Fix a missing separator in ShowBackupOptions. Example:
```
SHOW BACKUP 'family' IN ('string', 'placeholder', 'placeholder', 'placeholder', 'string', 'placeholder', 'string', 'placeholder') WITH incremental_location = 'nullif', privilegesdebug_dump_metadata_sst
```
Fix bad construction in ShowTenant. Example:
```
SHOW TENANT [B'10010'] WITH REPLICATION STATUS WITH CAPABILITIES
```
Epic: none
Release note: None
|
Yeah, I tried gen bazel and gen bnf and no dice. Oh wait I had some cruft preventing gen from working completely. Should be fixed now. I also rebased to pick up #98977 and now my 30s CI test seems to be working. I also ran a 5m one to be sure. I'll wait to merge until CI is green. |
|
Okay well stress on the rsg_tests found a bunch of issues so that's not ready for prime time. |
Previously COPY would allow a wide range of syntax in the COPY TO substatement. Now like PG we limit it to a few things. PG grammar is: ``` PreparableStmt: SelectStmt | InsertStmt | UpdateStmt | DeleteStmt | MergeStmt ``` And now we do something similar. This prevents the wheels from coming off when RSG generates EXPLAIN's in the substatement for instance. Release note: none Epic: none
|
bors r+ |
|
Build succeeded: |
sql: fix some TestRandomSyntaxGeneration bugs
The RSG works by calling format on the AST's it generates so its good
at finding Format bugs.
Fix a missing separator in ShowBackupOptions. Example:
Fix bad construction in ShowTenant. Example:
Epic: none
Release note: None
copy: fix copy grammar to match PG
Previously COPY would allow a wide range of syntax in the COPY
TO substatement. Now like PG we limit it to a few things.
PG grammar is:
And now we do something similar. This prevents the wheels from coming
off when RSG generates EXPLAIN's in the substatement for instance.
Release note: none
Epic: none