Skip to content
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

Allow auto_increment on non-primary key columns #936

Merged
merged 21 commits into from
Apr 8, 2022
Merged

Conversation

jycor
Copy link
Contributor

@jycor jycor commented Apr 5, 2022

Fix for: dolthub/dolt#2981

Since Unique Keys are not a part of sql.Schema and sql.Column, we need to check if there are any valid indexes defined over the columns we want to mark as auto_increment.
Added skipped tests for queries that don't work.

TODO:

  • Adding auto_increment adds NOT NULL constraint too
  • Support adding columns; need to be able to alter table t add column j int unique first
  • Autoincrement on keys defined over multiple columns; MySQL only allows leftmost column to be auto_incremented

@jycor jycor requested a review from zachmu April 7, 2022 22:19
Copy link
Contributor

@andy-wm-arthur andy-wm-arthur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good, Need to revert the Index interface change

sql/index.go Outdated
@@ -49,6 +49,8 @@ type Index interface {
// ColumnExpressionTypes returns each expression and its associated Type. Each expression string should exactly
// match the string returned from Index.Expressions().
ColumnExpressionTypes(ctx *Context) []ColumnExpressionType
// ColumnNames returns the names of the Columns that this Index is defined over
ColumnNames() []string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is effectively a duplicate of Expressions() []string. Instead of adding ColumnNames(), you should use the Expressions() in combination with GetColumnFromIndExpr

Copy link
Contributor

@andy-wm-arthur andy-wm-arthur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jycor jycor merged commit 3ad39b3 into main Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants