sqlsmith: Various usability and bug fixes. #120504
Conversation
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
|
|
This change is |
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? Thank you for updating your pull request. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
There was a problem hiding this comment.
Just two small comments.
Reviewed 5 of 8 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @miretskiy and @rytaft)
pkg/internal/sqlsmith/sqlsmith.go line 460 at r2 (raw file):
// EnableLimits causes the Smither to probabilistically emit LIMIT clauses. var EnableLimits = simpleOption("enable LIMIT", func(s *Smither) { s.disableLimits = true
Should this be false?
pkg/internal/sqlsmith/type.go line 62 at r2 (raw file):
switch t { case types.Box2D, types.Geography, types.Geometry, types.INet, types.PGLSN, types.RefCursor, types.TSQuery, types.TSVector:
suggestion: I think it would be better to construct this as an approval list rather than subtracting a disapproval list from all scalar types, because we're likely to keep adding types in the future and those types will most likely skew toward "fancy".
Yup.
|
|
bors r+ |
|
🔒 Permission denied Existing reviewers: click here to make miretskiy a reviewer |
|
@michae2 -- looks like I can't merge it; will you do the honors? |
There was a problem hiding this comment.
Yes! I will rebase it and then merge.
Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status:
This PR adds various command line flags and sqlsmith options to improve usability and correctness: * Add `--schema` command line argument to sqlsmith so that the running database instance is not required in order to populate existing table information. * Add `--prefix` command line argument which will add prefix to every statement/expression generated by sqlsmith (this can be used for example to generalte sqllogic format) * Add a new smither option `SimpleScalarTypes` which eschews "complex types" -- such as GEOMETRY, GEOGRAPHY, and other less common types. * Fix a bug where `DisableIndexHints` option was not respected (all tables had index flags). * Fix a bug where pretty printing (query or expression) did not respect `PostgresMode`, thus producing queries that are not compatible w/ postgres (e.g. they always included type annotations). Epic: None Release note: None
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
bors r=michae2 a=miretskiy |
This PR adds various command line flags and
sqlsmith options to improve usability and correctness:
--schemacommand line argument to sqlsmith so that the running database instance is not required in order to populate existing table information.--prefixcommand line argument which will add prefix to every statement/expression generated by sqlsmith (this can be used for example to generalte sqllogic format)SimpleScalarTypeswhich eschews "complex types" -- such as GEOMETRY, GEOGRAPHY, and other less common types.DisableIndexHintsoption was not respected (all tables had index flags).PostgresMode, thus producing queries that are not compatible w/ postgres (e.g. they always included type annotations).Epic: None
Release note: None