-
-
Notifications
You must be signed in to change notification settings - Fork 192
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 adding new primary key to table with > 1 row iff it has auto_increment #1343
Conversation
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 tests should go in this package, not in dolt, and 2) where are tests for the error case (e.g. preventing you from adding a primary key column if it's not auto increment)?
sql/plan/alter_table.go
Outdated
@@ -491,6 +491,12 @@ func (i *addColumnIter) rewriteTable(ctx *sql.Context, rwt sql.RewritableTable) | |||
|
|||
rowIter := sql.NewTableRowIter(ctx, rwt, partitions) | |||
|
|||
var autoTbl sql.AutoIncrementTable | |||
if newSch.HasAutoIncrement() { | |||
autoTbl = rwt.(sql.AutoIncrementTable) |
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 will panic if the interface doesn't match, always use the two-param version
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, just a couple comments
sql/plan/alter_table.go
Outdated
} | ||
autoTbl = t | ||
} | ||
idx := newSch.IndexOf(i.a.column.Name, i.a.column.Source) |
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.
Call this autoIncColIdx
sql/plan/alter_table.go
Outdated
@@ -504,6 +514,14 @@ func (i *addColumnIter) rewriteTable(ctx *sql.Context, rwt sql.RewritableTable) | |||
return false, err | |||
} | |||
|
|||
if autoTbl != nil { | |||
val, err := autoTbl.GetNextAutoIncrementValue(ctx, newRow[idx]) |
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 is a really inefficient way to do this. Get this value once, before the loop, and then increment it inside the loop
) | ||
}) | ||
|
||
t.Run("Add primary key column with auto increment first", func(t *testing.T) { |
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.
Have a test with a float too
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.
according to this warning, mysql plans on deprecating auto_increment float, do you still want me to add a test?
MySQL > alter table t add column pk float primary key auto_increment;
Query OK, 0 rows affected, 1 warning (0.0405 sec)
Records: 0 Duplicates: 0 Warnings: 1
Warning (code 3856): AUTO_INCREMENT support for FLOAT/DOUBLE columns is deprecated and will be removed in a future release. Consider removing AUTO_INCREMENT from column 'pk'.
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 support it in dolt (and in GMS) so we should have a test for it
enginetest/enginetests.go
Outdated
@@ -4996,6 +4996,80 @@ func TestAddDropPks(t *testing.T, harness Harness) { | |||
{1, 1}, | |||
}, nil, nil) | |||
}) | |||
|
|||
if _, ok := harness.(*MemoryHarness); ok { |
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.
Don't skip like this. Engine tests should always be harness agnostic.
Make a new test method called TestAddAutoIncrementColumn
and skip it from memory_engine_test.go
fix for: dolthub/dolt#4581
tests in dolt because
memory.Table
doesn't implementRewriteableTable
dolthub/dolt#4593