Skip to content

Conversation

@nicktobey
Copy link
Contributor

@nicktobey nicktobey commented Feb 26, 2024

Fixes #2344

This PR also adds a test framework for testing MySQL error codes, since this doesn't appear to currently be tested. This should make adding tests for other error codes easy.

Some of the MySQL error codes that I expected us to use (such as 1050: ERTableExists) have zero usages in GMS or Dolt. We're probably returning 1105: ERUnknownError for these.

Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

Test looks good, thanks for adding.

Main comment is that any user-facing error should be in the sql package. Probably all the error types you moved are in fact user-facing and should be moved there. We tend to put them all in errors.go. There might be a couple internal errors in there (namely "not supported" type errors that should be caught in analysis and never make it to plan time), and these should probably live where you found them. The rest should go in sql/errors.go.

@nicktobey
Copy link
Contributor Author

nicktobey commented Feb 26, 2024

Test looks good, thanks for adding.

Main comment is that any user-facing error should be in the sql package. Probably all the error types you moved are in fact user-facing and should be moved there. We tend to put them all in errors.go. There might be a couple internal errors in there (namely "not supported" type errors that should be caught in analysis and never make it to plan time), and these should probably live where you found them. The rest should go in sql/errors.go.

Makes sense. I'll undo the package refactor and just move errors from their current packages to sql as we discover that they're user-facing.

@nicktobey nicktobey merged commit 0f1cfb7 into main Feb 27, 2024
@nicktobey nicktobey deleted the nicktobey/mysql-errors branch February 27, 2024 21:44
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.

Server error message compatibility when doing INSERT

2 participants