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
No-op bad alter table auto increment #1930
Conversation
// In the case that someone runs an alter table auto increment on a table with no existing auto increment key, nop | ||
if te.autoIncCol.TypeInfo == nil { | ||
return nil | ||
} |
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.
we should no-op in the SQL engine, before we get here
// In the case that the alter table is adding a new auto increment value, then it will return a | ||
// ErrorNoAutoIncrement value. We can ignore that. | ||
if err != nil && !errors.Is(err, doltdb.ErrNoAutoIncrementValue) { |
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.
inspect the old/new schemas and catch the case where auto_increment
has been added to a column
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.
LGTM
@@ -147,7 +147,7 @@ func updateTableWithModifiedColumn(ctx context.Context, tbl *doltdb.Table, oldSc | |||
return nil, err | |||
} | |||
var autoVal types.Value | |||
if schema.HasAutoIncrement(newSch) { | |||
if schema.HasAutoIncrement(newSch) && schema.HasAutoIncrement(oldSch) { |
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.
this looks much better, could you add a comment explaining the alter table case? Also, what happens if the AUTO_INCREMENT
is removed from the schema?
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.
The new schema would not have auto increment an a nil value would be passed for autoVal
Change the Title from "Nop" to "No-op" please |
Pr does 2 things