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

sqlsmith: fix flaky TestGenerateParse #66743

Merged
merged 1 commit into from
Jun 24, 2021

Conversation

mgartner
Copy link
Collaborator

This commit fixes the sqlsmith.TestGenerateParse test that has been
flaking since #66599 when primary index stored columns were added to the
output of SHOW INDEXES.

sqlsmith uses SHOW INDEXES to build an in-memory representation of
indexes to help generate interesting randomized queries. It is not
equipped to handle rowid primary indexes, so rows output from
SHOW INDEXES with a column_name of rowid are filtered out. Stored
columns of primary indexes, now included in SHOW INDEXES, are not
filtered out, causing sqlsmith to build an in-memory representation of
rowid indexes that have only stored columns and no indexed columns.
This breaks the assumption within sqlsmith that each index should have
at least one indexed column, causing panics.

This commit fixes the issue by removing indexes with no indexed columns
from sqlsmith's in-memory index map.

Fixes #66723

Release note: None

@mgartner mgartner requested review from postamar and a team June 22, 2021 22:39
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@mgartner
Copy link
Collaborator Author

I'll update this PR to unskip TestGenerateParse once #66736 is merged.

This commit fixes the `sqlsmith.TestGenerateParse` test that has been
flaking since cockroachdb#66599 when primary index stored columns were added to the
output of `SHOW INDEXES`.

sqlsmith uses `SHOW INDEXES` to build an in-memory representation of
indexes to help generate interesting randomized queries. It is not
equipped to handle `rowid` primary indexes, so rows output from
`SHOW INDEXES` with a `column_name` of `rowid` are filtered out. Stored
columns of primary indexes, now included in `SHOW INDEXES`, are not
filtered out, causing sqlsmith to build an in-memory representation of
`rowid` indexes that have only stored columns and no indexed columns.
This breaks the assumption within sqlsmith that each index should have
at least one indexed column, causing panics.

This commit fixes the issue by removing indexes with no indexed columns
from sqlsmith's in-memory index map.

Fixes cockroachdb#66723

Release note: None
@mgartner mgartner force-pushed the 66723-fix-sqlsmith-flake branch from c455fba to 9a24944 Compare June 23, 2021 02:02
@mgartner
Copy link
Collaborator Author

I'll update this PR to unskip TestGenerateParse once #66736 is merged.

Done.

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

:lgtm:

bors r=ajwerner

Reviewed 3 of 3 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @postamar)

@craig
Copy link
Contributor

craig bot commented Jun 24, 2021

Build succeeded:

@craig craig bot merged commit 663a44a into cockroachdb:master Jun 24, 2021
@mgartner mgartner deleted the 66723-fix-sqlsmith-flake branch June 29, 2021 16:18
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.

internal/sqlsmith: "index out of range" panic in makeMergeJoinExprwith
3 participants