Max/check constraint #317
Max/check constraint #317
Conversation
enginetest/enginetests.go
Outdated
| @@ -114,6 +114,15 @@ func createForeignKeys(t *testing.T, harness Harness, engine *sqle.Engine) { | |||
| } | |||
| } | |||
|
|
|||
| func createCheckConstraint(t *testing.T, harness Harness, engine *sqle.Engine) { | |||
There was a problem hiding this comment.
Since you have the tests for check constraints defined as separate methods, no need to declare harness support. That's only needed for things included in queries.go and friends. Integrators can just not call the check constraint test methods if they don't support it.
memory/table.go
Outdated
| return nil | ||
| } | ||
|
|
||
| // CreateCheck implements sql.CheckAlterableTable | ||
| func (t *Table) CreateCheckConstraint(_ *sql.Context, chName string, expr sql.Expression, enforced bool) error { |
There was a problem hiding this comment.
Think you need to check all rows at this time, right?
| } else if chConstraint, ok := cd.Details.(*sqlparser.CheckConstraintDefinition); ok { | ||
| var c sql.Expression | ||
| var err error | ||
| if chConstraint.Expr != nil { |
There was a problem hiding this comment.
This is illegal otherwise right? Should either throw an error or not check this
There was a problem hiding this comment.
DROP CHECK doesn't have an expression
sql/plan/alter_check.go
Outdated
| } | ||
| } | ||
|
|
||
| // Execute inserts the rows in the database. |
sql/plan/alter_check.go
Outdated
| cols[col.Name] = true | ||
| } | ||
|
|
||
| sql.Inspect(p.ChDef.Expr, func(expr sql.Expression) bool { |
There was a problem hiding this comment.
You should do this at analyzer time, not at execution time. You basically get resolution of things like columns and functions "for free" if you make this node implement sql.Expressioner
There was a problem hiding this comment.
Then after things are resolved, add a validation rule that enforces that only allowable expression types are present in the check expression
sql/plan/ddl.go
Outdated
| @@ -91,7 +92,7 @@ var _ sql.Node = (*CreateTable)(nil) | |||
| var _ sql.Expressioner = (*CreateTable)(nil) | |||
|
|
|||
| // NewCreateTable creates a new CreateTable node | |||
| func NewCreateTable(db sql.Database, name string, schema sql.Schema, ifNotExists bool, idxDefs []*IndexDefinition, fkDefs []*sql.ForeignKeyConstraint) *CreateTable { | |||
| func NewCreateTable(db sql.Database, name string, schema sql.Schema, ifNotExists bool, idxDefs []*IndexDefinition, fkDefs []*sql.ForeignKeyConstraint, chDefs []*sql.CheckConstraint) *CreateTable { | |||
There was a problem hiding this comment.
This is getting a little cumbersome. I think it's time to break out a tableSpecs object with named with methods or similar.
sql/analyzer/check_constraints.go
Outdated
|
|
||
| // Make sure that all columns are valid, in the table, and there are no duplicates | ||
| switch expr := e.(type) { | ||
| case *expression.UnresolvedColumn: |
There was a problem hiding this comment.
Add a TODO here -- the columns should be resolved (or fail to resolve) before running this validation pass
There was a problem hiding this comment.
non-existent fields are necessarily unresolved here, added a case for GetField in case a different table's column resolves (not sure if that's valid)
There was a problem hiding this comment.
my bad deferred*, not unresolved
| } | ||
| case sqlparser.DropStr: | ||
| switch c := parsedConstraint.(type) { | ||
| case *sql.ForeignKeyConstraint: | ||
| return plan.NewAlterDropForeignKey(table, c), nil | ||
| case *sql.CheckConstraint: | ||
| return plan.NewAlterDropCheck(table, c), nil | ||
| case namedConstraint: |
There was a problem hiding this comment.
Add a TODO here: this is broken if they try to drop a check constraint without saying it's a check constraint
There was a problem hiding this comment.
added TODO, should plan.NewAlterDropConstraint just be generalized or do we need two different plans?
check constraints in memory engine
depends on dolthub/vitess#53
TODOs maybe:
TODOs in future: