Vinai/insert ignore into #381
Conversation
VinaiRachakonda
changed the title
memory/table.go
Outdated
| @@ -1004,7 +1002,7 @@ func (t *Table) GetIndexes(ctx *sql.Context) ([]sql.Index, error) { | |||
| return append(indexes, nonPrimaryIndexes...), nil | |||
| } | |||
|
|
|||
| // GetForeignKeys implements sql.ForeignKeyTable | |||
| // GetForeignKeys implements sql. gnKeyTable | |||
sql/errors.go
Outdated
| @@ -80,7 +80,8 @@ var ( | |||
| ErrDuplicateAliasOrTable = errors.NewKind("Not unique table/alias: %s") | |||
|
|
|||
| // ErrPrimaryKeyViolation is returned when a primary key constraint is violated | |||
| ErrPrimaryKeyViolation = errors.NewKind("duplicate primary key given") | |||
| // TODO: This should be a ErDupKey instead | |||
There was a problem hiding this comment.
ErrDuplicateEntry? Not sure what ErDupKey is, should write the whole name.
| ignore := false | ||
| if strings.Contains(strings.ToLower(i.Ignore), "ignore") { | ||
| ignore = true | ||
| } |
There was a problem hiding this comment.
This should be a bool in vitess rather than matching on the string. Either ignore is there or it isn't.
Change that in vitess (should be super quick), or add a TODO and get this merged in first.
sql/plan/insert.go
Outdated
| for { | ||
| if err := i.replacer.Insert(i.ctx, row); err != nil { | ||
| if !sql.ErrPrimaryKeyViolation.Is(err) && !sql.ErrUniqueKeyViolation.Is(err) { | ||
| return i.ignoreOrClose(err) |
There was a problem hiding this comment.
This is not a thing. REPLACE IGNORE does not exist AFAIK. This should be reverted to the original logic.
sql/plan/row_update_accumulator.go
Outdated
| @@ -209,6 +209,8 @@ func (a *accumulatorIter) Next() (sql.Row, error) { | |||
| row, err := a.iter.Next() | |||
| if err == io.EOF { | |||
| return sql.NewRow(a.updateRowHandler.okResult()), nil | |||
| } else if ErrInsertIgnore.Is(err) { // return err if its not an ignorable error | |||
| continue | |||
| } | |||
There was a problem hiding this comment.
Move the err != nil up to make it an else if on this chain. Or you could move both of these inside of the if err != nil, returning the error if these two cases aren't matched.
sql/plan/insert.go
Outdated
| // In the case of an IGNORE we set the nil value to a default and add a warning | ||
| if i.ignore { | ||
| row[count] = col.Type.Zero() | ||
| _ = i.handleErrorAndQuitOrIgnore(sql.ErrInsertIntoNonNullableProvidedNull.New(col.Name)) // will always return nil |
There was a problem hiding this comment.
It looks like you're using this to add a warning? I'd write a comment stating as such. Also maybe break handleErrorAndQuitOrIgnore up and have a function that only warns on ignorable errors. It isn't immediately clear what calling a function that has quit or ignore and ignoring the returned variable is for.
Maybe name it warnOnIgnorableError or something.
| {sql.OkResult{RowsAffected: 0}}, | ||
| }, | ||
| ExpectedWarning: mysql.ERSubqueryNo1Row, | ||
| }, |
There was a problem hiding this comment.
Throw in a case with a duplicate primary key but different c1 value, so like (4, 8) or such.
This PR adds support for INSERT IGNORE INTO. It is currently missing functionality for ignoring typer errors. After scoping the type errors component of this it looked to a lot of work to capture all of them with the correct MySQL notation
I also noticed a lack of partition support in MySQL terms as well so I ignored that.