Vinai/create database #298
Conversation
sql/catalog.go
Outdated
| @@ -81,6 +81,13 @@ func (c *Catalog) AddDatabase(db Database) { | |||
| c.mu.Unlock() | |||
| } | |||
|
|
|||
| // DropDatabase removes a database to the catalog. | |||
There was a problem hiding this comment.
Call this Remove instead of Drop. Drop would be for integrators to implement (which isn't supported yet)
sql/catalog.go
Outdated
| @@ -120,6 +127,10 @@ func (d Databases) Database(name string) (Database, error) { | |||
| return nil, ErrDatabaseNotFound.New(name) | |||
| } | |||
|
|
|||
| if len(name) == 0 { | |||
| return nil, ErrNoDatabaseSelected.New() | |||
There was a problem hiding this comment.
This isn't the right error here, should just be ErrDatabaseNotFound as above
There was a problem hiding this comment.
And in fact you don't need this check specifically, it's already handled by the logic below
There was a problem hiding this comment.
So the problem I was having is that mysql should return a database not set error when you drop the database you are currently using. This adds that functionality. Lmk if that seems like a hacky solution to you.
There was a problem hiding this comment.
I'll add a test to sql-server in dolt to show this.
sql/catalog.go
Outdated
|
|
||
| if idx != -1 { | ||
| dbs := *d | ||
| dbs[idx] = dbs[len(dbs)-1] |
There was a problem hiding this comment.
Check out this guide for a shorter way to remove an element from a slice
sql/errors.go
Outdated
| @@ -126,4 +126,10 @@ var ( | |||
|
|
|||
| // ErrTruncateReferencedFromForeignKey is returned when a table is referenced in a foreign key and TRUNCATE is called on it. | |||
| ErrTruncateReferencedFromForeignKey = errors.NewKind("cannot truncate table %s as it is referenced in foreign key %s on table %s") | |||
|
|
|||
| // ErrDatabaseExists is returned when a CREATE DATABASE is called on a table that already exists. | |||
| ErrDatabaseExists = errors.NewKind("can't create database %s; database exists") | |||
sql/errors.go
Outdated
| ErrDatabaseExists = errors.NewKind("can't create database %s; database exists") | ||
|
|
||
| // ErrDatabaseDoesntExists is returned when a DROP DATABASE is callend when a table is dropped that doesn't exist. | ||
| ErrDatabaseDoesntExists = errors.NewKind("can't drop database %s; database doesn't exist") |
There was a problem hiding this comment.
ErrCannotDropDatabaseDoesntExist
sql/plan/dbddl.go
Outdated
| return NillaryWithChildren(c, children...) | ||
| } | ||
|
|
||
| func NewCreateDatabase(dbName string, ifNotExists bool, collate string, charset string) *CreateDB { |
There was a problem hiding this comment.
collation (same with field names
There was a problem hiding this comment.
On second thought, don't even take collation and charset since we don't and can't do anything with them.
| } | ||
| } | ||
|
|
||
| type DropDB struct { |
There was a problem hiding this comment.
Also needs a comment about what this does
sql/plan/dbddl.go
Outdated
| } | ||
|
|
||
| func (d DropDB) Schema() sql.Schema { | ||
| return nil |
There was a problem hiding this comment.
Both of these should return sql.OkResultSchema and report one row modified from RowIter()
sql/plan/dbddl.go
Outdated
|
|
||
| func (d DropDB) RowIter(ctx *sql.Context, row sql.Row) (sql.RowIter, error) { | ||
| exists := d.Catalog.HasDB(d.dbName) | ||
| if d.IfExists && !exists { |
There was a problem hiding this comment.
Same comment about structuring this logic
sql/plan/dbddl.go
Outdated
| return NillaryWithChildren(d, children...) | ||
| } | ||
|
|
||
| func NewDropDatabase(dbName string, ifExists bool, collate string, charset string) *DropDB { |
There was a problem hiding this comment.
Same comment about collation and charset
| require.Error(t, err) | ||
|
|
||
| // TODO: Deal with handling this error. | ||
| //AssertErr(t, e, harness, "SHOW TABLES", sql.ErrNoDatabaseSelected) |
There was a problem hiding this comment.
@zachmu Are you fine with me leaving this as an issue an moving on? Getting this render correctly seems to be a pain to get right and is not an important feature imo.
There was a problem hiding this comment.
Yeah I think that's fine for now
| require.Error(t, err) | ||
|
|
||
| // TODO: Deal with handling this error. | ||
| //AssertErr(t, e, harness, "SHOW TABLES", sql.ErrNoDatabaseSelected) |
There was a problem hiding this comment.
Yeah I think that's fine for now
enginetest/enginetests.go
Outdated
| //AssertErr(t, e, harness, "SHOW TABLES", sql.ErrNoDatabaseSelected) | ||
| }) | ||
|
|
||
| t.Run("DROP DATABASE works on newly created tables.", func(t *testing.T) { |
enginetest/enginetests.go
Outdated
| AssertErr(t, e, harness, "USE testdb", sql.ErrDatabaseNotFound) | ||
| }) | ||
|
|
||
| t.Run("DROP SCHEMA works on newly created tables.", func(t *testing.T) { |
| AssertErr(t, e, harness, "USE testdb", sql.ErrDatabaseNotFound) | ||
| }) | ||
|
|
||
| t.Run("DROP DATABASE IF EXISTS correctly works.", func(t *testing.T) { |
There was a problem hiding this comment.
You need to also do this on a database that doesn't exist and assert no err
enginetest/enginetests.go
Outdated
| }) | ||
|
|
||
| t.Run("DROP DATABASE IF EXISTS correctly works.", func(t *testing.T) { | ||
| AssertWarning(t, e, harness, "DROP DATABASE IF EXISTS mydb", 1007) |
There was a problem hiding this comment.
Use consts for expected err codes
| "github.com/dolthub/vitess/go/vt/sqlparser" | ||
| ) | ||
|
|
||
| type CreateDB struct { |
There was a problem hiding this comment.
integrators are people who provide database implementation, e.g. dolt
sql/plan/dbddl.go
Outdated
| if d.IfExists { | ||
| ctx.Session.Warn(&sql.Warning{ | ||
| Level: "Note", | ||
| Code: 1007, |
Add functionality for
CREATE DATABASE