schema: preserve table metadata when updating column tags#11152
schema: preserve table metadata when updating column tags#11152elianddb wants to merge 6 commits into
Conversation
|
@elianddb DOLT
|
elianddb
changed the title
elianddb
force-pushed
the
elian/11007-1-schema
branch
from
16a9945 to
b11391a
Compare
fulghum
left a comment
fulghum
left a comment
There was a problem hiding this comment.
Looks good! Just a couple small things to check out.
| } | ||
|
|
||
| @test "sql-update-column-tag: preserves indexes, check constraints, comment, and multi-column primary key" { | ||
| # See https://github.com/dolthub/dolt/issues/11007 |
There was a problem hiding this comment.
This issue doesn't seem related, right? It's about dolt reset not losing unstaged tables, but we don't seem to use reset at all here. Seems like we can just delete this comment if that's the case.
| } | ||
|
|
||
| @test "column_tags: update-tag preserves indexes, check constraints, comment, and multi-column primary key" { | ||
| # See https://github.com/dolthub/dolt/issues/11007 |
There was a problem hiding this comment.
Noticed this was in all three BATS tests, but doesn't seem to be the issue we're actually testing for, so seems like we either fix this link to the related issue or take it out. Let me know if I'm just missing something.
| pkOrdinals := slices.Clone(sch.GetPkOrdinals()) | ||
| newSch, err := NewSchema(NewColCollection(cols...), pkOrdinals, sch.GetCollation(), nil, sch.Checks().Copy()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| newSch.SetTargetRowSize(sch.GetTargetRowSize()) | ||
| newSch.SetComment(sch.GetComment()) |
There was a problem hiding this comment.
You may be able to use Schema.Copy() to get a copy and save some code here, as well as not having to update this code in the future if new properties are added to schema that would then need to be set on the new instance. I see you're updating the indexes, too, so would need to account for that, but it may still end up being simpler.
|
|
||
| err = sch.Indexes().Iter(func(idx Index) (stop bool, err error) { | ||
| tags := RemapTags(idx.IndexedColumnTags(), tagRemap) | ||
| _, err = newSch.Indexes().AddIndexByColTags(idx.Name(), tags, idx.PrefixLengths(), idx.Properties()) |
There was a problem hiding this comment.
I believe foreign keys also need some help, since they encode a column tag, if I'm remembering correctly. Would be good to test that case, but I suspect it needs to be updated, too.
| } | ||
|
|
||
| func TestWithRemappedColumnTags(t *testing.T) { | ||
| // See https://github.com/dolthub/dolt/issues/11007 |
There was a problem hiding this comment.
I left a couple comments about this below, but I think I see now that this issue kinda turned into multiple PRs. The issue is a bit confusing though, because the repro doesn't use update-tag or show that the tags are conflicting. Something to think about, just wasn't immediately clear how that issue is related to these tests when the repro in the issue doesn't modify column tags.
| require.Equal(t, uint64(200), pkCols[1].Tag, "PK column must carry the remapped id tag") | ||
| }) | ||
|
|
||
| t.Run("PreservesIndexes", func(t *testing.T) { |
There was a problem hiding this comment.
Good coverage of different schema features. I think foreign keys are also worth looking at, since I believe they work off of column tags, too.
3 participants
updateColumnTagwas duplicated as a private helper inschcmds/update-tag.goanddprocedures/dolt_update_column_tag.go, and rebuilt the schema without preserving a schema's secondary indexes, CHECK constraints, table comment, and target row size.schema.WithRemappedColumnTagsandschema.WithUpdatedColumnTagto rebuild schema with all metadata: indexes, CHECK constraints, comment, target row size, PK ordinals, and collation.Index.Properties()so indexes can be re-added on the rebuilt schema with their full property set.Refs:
reset --hardsilently drops untracked tables with column tag collisions #11007