-
Notifications
You must be signed in to change notification settings - Fork 375
Add CockroachDB Support #359
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
base: main
Are you sure you want to change the base?
Conversation
nelsam
left a comment
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.
I would also like to see tests for the cockroachdb dialect.
| IfTableNotExists(command, schema, table string) string | ||
|
|
||
| // The command to create a new database/schema | ||
| CreateSchemaCommand() string |
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.
Unfortunately, this would force a major version bump, because custom dialects would no longer implement the updated Dialect type. Can we do this as an optional type that dialects are not required to implement (and then use the old logic as a fallback)? Maybe something like type NonstandardSchemaDialect interface { CreateSchemaCommand() string }?
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.
Excellent idea. I'd like to change it slightly, though, since even a NonstandardSchemaDialect could vary amongst custom implementations. Maybe a type SchemaCreator() interface { CreateSchemaCommand() string } instead?
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.
I think SchemaCreator is a little misleading, since all other dialects will still be able to create schemas. What we're providing is a way for dialects to inform gorp that they are unable to execute create schema operations per the SQL standard.
For those reasons, I still think something along the lines of "nonstandard" should be part of the name. It is accurate - these are dialects that cannot handle the SQL standard for create schema, so they should be labeled as such. NonstandardSchemaCreator sounds fine to me, too.
Also, we might want to pass the schema name in to CreateSchemaCommand so that dialects have more power over the resulting SQL command.
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.
I like the sound of that. I'll get that implemented and try to sort out testing after the holidays 👍
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.
@zikes Hey, just checking in; have you had any more time to work on this?
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.
I haven't, sorry. It's something I'd still be interested in implementing sometime, but it's been less of a priority for me lately.
|
Hi folks, just wondering if anyone has time to revisit this? |
Because CockroachDB does not support the
create schemasyntax, an addition to theDialectinterface was necessary.