-
Notifications
You must be signed in to change notification settings - Fork 869
MySQL Support #225
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
MySQL Support #225
Conversation
* initial commit of sql schema validation * adds barebones support for most basic use-case * improves parsing and generation of params * adds decent tests for mysql * improves param implementation - adds tests - adds support for named params - adds support for Limit expressions * adds support for decimal types * resolves some testing issues * generalizes imports * adds null type tests * improves go struct generation * adds command line call to mysql generate * improves support for insert queries and adds tests * improves support for enum types * refactors config to function param * patchs pkg settings arguments * simplifies and improves param parsing * param implementation and test patches * patch placeholder default table * properly handles "select * " expressions * initial commit of sql schema validation * adds barebones support for most basic use-case * improves parsing and generation of params * adds decent tests for mysql * improves param implementation - adds tests - adds support for named params - adds support for Limit expressions * adds support for decimal types * resolves some testing issues * generalizes imports * adds null type tests * improves go struct generation * adds command line call to mysql generate * improves support for insert queries and adds tests * improves support for enum types * refactors config to function param * patchs pkg settings arguments * simplifies and improves param parsing * param implementation and test patches * patch placeholder default table * properly handles "select * " expressions
Amazing. I'm going to try this out. |
🏆 🏆 🏆 @cmoog thank you so much for this pull request! I didn't think MySQL support was going to land any time soon and I've been proven wrong. I'm still on Christmas / NYE vacation, so I won't get a chance to review this until January 2nd. I just wanted to let you know that I've seen the PR and will be reviewing it soon.
Agreed! There's another open PR which I will wait to merge until this lands. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cmoog Thanks again for taking the time and effort to make this first pass at MySQL support. Since this is the first time sqlc will support two different databases, I'd like to make sure the various packages fit together well for future databases.
sqlc generate
should support different databases. Instead of having a different command, let's extend the package syntax to have a database
field.
{
"version": "1",
"packages": [
{
"name": "foo",
"database": "postgres",
"path": "internal/foo",
"queries": "./psql/query/",
"schema": "./psql/schema/"
},
{
"name": "bar",
"database": "mysql",
"path": "internal/bar",
"queries": "./mysql/query/",
"schema": "./mysql/schema/"
}
]
}
The interface between parsing and code generation isn't clear right now, I'm hoping we can clean that up. I'm going to play around with the code locally before making any more comments.
@@ -159,8 +180,15 @@ type GoQuery struct { | |||
Arg GoQueryValue | |||
} | |||
|
|||
func (r Result) UsesType(typ string) bool { | |||
for _, strct := range r.Structs() { | |||
type Generateable interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As sqlc adds support for more database engines, I'd like this to b a concrete type instead of an interface. The Query
struct generated by the parser should be general enough to use across engines. I understand that's not the case today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point. Although generatable
worked as a POC, I do think it's appropriate to pay careful consideration to where the line of abstraction should lie. Good thinking with generalizing Query
to be usable by multiple engines.
@cmoog I've changed the pull request's base branch, it now merges into the If you'd rather we work on this another way, please let me know. It's easy to do this differently. |
@kyleconroy Sounds good to me. Thanks for taking a look at the PR. Let me know how you'd like to proceed with improving the interface between parsing and code generation. When you think it's appropriate, I'd be happy to make the necessary changes to the |
I've added a set of TODO items to #230. I don't think the mysql package will need to change too much before being merged in. The interface between parsing and code generation is a work in progress, so there's no point in letting this code sit around before it gets figured out. |
This PR contains a near feature-complete implementation of MySQL support. Although it's missing a few features, I do think it is wise to merge to prevent more significant merge conflicts down the line.
Most notably, the refactoring of the
dinosql/gen.go
file into parser agnostic Go code generation functions could potentially cause difficulty merging if we wait until MySQL support is fully stable.Much of the remaining implementation work involves supporting all types and functions of the MySQL spec completely. This should not drastically alter the structure of the current implementation.
Critiques and contributions are welcome!
Fixes #150