Skip to content

Commit 59b003a

Browse files
xiaocai2333xiaocai2333
authored
enhance: Skip modify field meta when rename collection or rename dbName (milvus-io#42875)
issue: milvus-io#42873 --------- Signed-off-by: Cai Zhang <cai.zhang@zilliz.com>
1 parent ee056f0 commit 59b003a

18 files changed

Lines changed: 219 additions & 84 deletions

‎cmd/tools/migration/mmap/mmap_230_240.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ func (m *MmapMigration) MigrateRootCoordCollection(ctx context.Context) {
5959
newColl.Properties = updateOrAddMmapKey(newColl.Properties, common.MmapEnabledKey, "true")
6060
fmt.Printf("migrate collection %v, %s\n", collection.CollectionID, collection.Name)
6161

62-
if err := m.rootcoordMeta.AlterCollection(ctx, collection, newColl, ts); err != nil {
62+
if err := m.rootcoordMeta.AlterCollection(ctx, collection, newColl, ts, false); err != nil {
6363
panic(err)
6464
}
6565
}

‎internal/core/src/segcore/SegmentGrowingImpl.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1255,7 +1255,8 @@ SegmentGrowingImpl::FinishLoad() {
12551255
}
12561256
// append_data is called according to schema before
12571257
// so we must check data empty here
1258-
if (!IsVectorDataType(field_meta.get_data_type()) &&insert_record_.get_data_base(field_id)->empty()) {
1258+
if (!IsVectorDataType(field_meta.get_data_type()) &&
1259+
insert_record_.get_data_base(field_id)->empty()) {
12591260
fill_empty_field(field_meta);
12601261
}
12611262
}

‎internal/metastore/catalog.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,8 @@ type RootCoordCatalog interface {
2626
ListCollections(ctx context.Context, dbID int64, ts typeutil.Timestamp) ([]*model.Collection, error)
2727
CollectionExists(ctx context.Context, dbID int64, collectionID typeutil.UniqueID, ts typeutil.Timestamp) bool
2828
DropCollection(ctx context.Context, collectionInfo *model.Collection, ts typeutil.Timestamp) error
29-
AlterCollection(ctx context.Context, oldColl *model.Collection, newColl *model.Collection, alterType AlterType, ts typeutil.Timestamp) error
29+
AlterCollection(ctx context.Context, oldColl *model.Collection, newColl *model.Collection, alterType AlterType, ts typeutil.Timestamp, fieldModify bool) error
30+
AlterCollectionDB(ctx context.Context, oldColl *model.Collection, newColl *model.Collection, ts typeutil.Timestamp) error
3031

3132
CreatePartition(ctx context.Context, dbID int64, partition *model.Partition, ts typeutil.Timestamp) error
3233
DropPartition(ctx context.Context, dbID int64, collectionID typeutil.UniqueID, partitionID typeutil.UniqueID, ts typeutil.Timestamp) error

‎internal/metastore/kv/rootcoord/kv_catalog.go‎

Lines changed: 38 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -629,10 +629,13 @@ func (kc *Catalog) DropCollection(ctx context.Context, collectionInfo *model.Col
629629
return kc.Snapshot.MultiSaveAndRemove(ctx, nil, collectionKeys, ts)
630630
}
631631

632-
func (kc *Catalog) alterModifyCollection(ctx context.Context, oldColl *model.Collection, newColl *model.Collection, ts typeutil.Timestamp) error {
632+
func (kc *Catalog) alterModifyCollection(ctx context.Context, oldColl *model.Collection, newColl *model.Collection, ts typeutil.Timestamp, fieldModify bool) error {
633633
if oldColl.TenantID != newColl.TenantID || oldColl.CollectionID != newColl.CollectionID {
634634
return errors.New("altering tenant id or collection id is forbidden")
635635
}
636+
if oldColl.DBID != newColl.DBID {
637+
return errors.New("altering dbID should use `AlterCollectionDB` interface")
638+
}
636639
oldCollClone := oldColl.Clone()
637640
oldCollClone.DBID = newColl.DBID
638641
oldCollClone.Name = newColl.Name
@@ -649,7 +652,6 @@ func (kc *Catalog) alterModifyCollection(ctx context.Context, oldColl *model.Col
649652
oldCollClone.Fields = newColl.Fields
650653
oldCollClone.UpdateTimestamp = newColl.UpdateTimestamp
651654

652-
oldKey := BuildCollectionKey(oldColl.DBID, oldColl.CollectionID)
653655
newKey := BuildCollectionKey(newColl.DBID, oldColl.CollectionID)
654656
value, err := proto.Marshal(model.MarshalCollectionModel(oldCollClone))
655657
if err != nil {
@@ -658,28 +660,45 @@ func (kc *Catalog) alterModifyCollection(ctx context.Context, oldColl *model.Col
658660
saves := map[string]string{newKey: string(value)}
659661
// no default aliases will be created.
660662
// save fields info to new path.
661-
for _, field := range newColl.Fields {
662-
k := BuildFieldKey(newColl.CollectionID, field.FieldID)
663-
fieldInfo := model.MarshalFieldModel(field)
664-
v, err := proto.Marshal(fieldInfo)
665-
if err != nil {
666-
return err
663+
if fieldModify {
664+
for _, field := range newColl.Fields {
665+
k := BuildFieldKey(newColl.CollectionID, field.FieldID)
666+
fieldInfo := model.MarshalFieldModel(field)
667+
v, err := proto.Marshal(fieldInfo)
668+
if err != nil {
669+
return err
670+
}
671+
saves[k] = string(v)
667672
}
668-
saves[k] = string(v)
669673
}
670-
if oldKey == newKey {
671-
return etcd.SaveByBatchWithLimit(saves, util.MaxEtcdTxnNum/2, func(partialKvs map[string]string) error {
672-
return kc.Snapshot.MultiSave(ctx, partialKvs, ts)
673-
})
674+
return etcd.SaveByBatchWithLimit(saves, util.MaxEtcdTxnNum/2, func(partialKvs map[string]string) error {
675+
return kc.Snapshot.MultiSave(ctx, partialKvs, ts)
676+
})
677+
}
678+
679+
func (kc *Catalog) AlterCollection(ctx context.Context, oldColl *model.Collection, newColl *model.Collection, alterType metastore.AlterType, ts typeutil.Timestamp, fieldModify bool) error {
680+
switch alterType {
681+
case metastore.MODIFY:
682+
return kc.alterModifyCollection(ctx, oldColl, newColl, ts, fieldModify)
683+
default:
684+
return fmt.Errorf("altering collection doesn't support %s", alterType.String())
674685
}
675-
return kc.Snapshot.MultiSaveAndRemove(ctx, saves, []string{oldKey}, ts)
676686
}
677687

678-
func (kc *Catalog) AlterCollection(ctx context.Context, oldColl *model.Collection, newColl *model.Collection, alterType metastore.AlterType, ts typeutil.Timestamp) error {
679-
if alterType == metastore.MODIFY {
680-
return kc.alterModifyCollection(ctx, oldColl, newColl, ts)
688+
func (kc *Catalog) AlterCollectionDB(ctx context.Context, oldColl *model.Collection, newColl *model.Collection, ts typeutil.Timestamp) error {
689+
if oldColl.TenantID != newColl.TenantID || oldColl.CollectionID != newColl.CollectionID {
690+
return errors.New("altering tenant id or collection id is forbidden")
681691
}
682-
return fmt.Errorf("altering collection doesn't support %s", alterType.String())
692+
oldKey := BuildCollectionKey(oldColl.DBID, oldColl.CollectionID)
693+
newKey := BuildCollectionKey(newColl.DBID, newColl.CollectionID)
694+
695+
value, err := proto.Marshal(model.MarshalCollectionModel(newColl))
696+
if err != nil {
697+
return err
698+
}
699+
saves := map[string]string{newKey: string(value)}
700+
701+
return kc.Snapshot.MultiSaveAndRemove(ctx, saves, []string{oldKey}, ts)
683702
}
684703

685704
func (kc *Catalog) alterModifyPartition(ctx context.Context, oldPart *model.Partition, newPart *model.Partition, ts typeutil.Timestamp) error {
@@ -860,7 +879,7 @@ func (kc *Catalog) fixDefaultDBIDConsistency(ctx context.Context, collMeta *pb.C
860879
coll := model.UnmarshalCollectionModel(collMeta)
861880
cloned := coll.Clone()
862881
cloned.DBID = util.DefaultDBID
863-
kc.alterModifyCollection(ctx, coll, cloned, ts)
882+
kc.AlterCollectionDB(ctx, coll, cloned, ts)
864883

865884
collMeta.DbId = util.DefaultDBID
866885
}

‎internal/metastore/kv/rootcoord/kv_catalog_test.go‎

Lines changed: 55 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ import (
1515
"github.com/stretchr/testify/require"
1616
"go.uber.org/atomic"
1717
"go.uber.org/zap"
18-
"golang.org/x/exp/maps"
1918
"google.golang.org/protobuf/proto"
2019

2120
"github.com/milvus-io/milvus-proto/go-api/v2/commonpb"
@@ -1019,14 +1018,14 @@ func TestCatalog_AlterCollection(t *testing.T) {
10191018
t.Run("add", func(t *testing.T) {
10201019
kc := NewCatalog(nil, nil)
10211020
ctx := context.Background()
1022-
err := kc.AlterCollection(ctx, nil, nil, metastore.ADD, 0)
1021+
err := kc.AlterCollection(ctx, nil, nil, metastore.ADD, 0, false)
10231022
assert.Error(t, err)
10241023
})
10251024

10261025
t.Run("delete", func(t *testing.T) {
10271026
kc := NewCatalog(nil, nil)
10281027
ctx := context.Background()
1029-
err := kc.AlterCollection(ctx, nil, nil, metastore.DELETE, 0)
1028+
err := kc.AlterCollection(ctx, nil, nil, metastore.DELETE, 0, false)
10301029
assert.Error(t, err)
10311030
})
10321031

@@ -1048,7 +1047,7 @@ func TestCatalog_AlterCollection(t *testing.T) {
10481047
var collectionID int64 = 1
10491048
oldC := &model.Collection{CollectionID: collectionID, State: pb.CollectionState_CollectionCreating}
10501049
newC := &model.Collection{CollectionID: collectionID, State: pb.CollectionState_CollectionCreated, UpdateTimestamp: rand.Uint64()}
1051-
err := kc.AlterCollection(ctx, oldC, newC, metastore.MODIFY, 0)
1050+
err := kc.AlterCollection(ctx, oldC, newC, metastore.MODIFY, 0, true)
10521051
assert.NoError(t, err)
10531052
key := BuildCollectionKey(0, collectionID)
10541053
value, ok := kvs[key]
@@ -1067,25 +1066,69 @@ func TestCatalog_AlterCollection(t *testing.T) {
10671066
var collectionID int64 = 1
10681067
oldC := &model.Collection{TenantID: "1", CollectionID: collectionID, State: pb.CollectionState_CollectionCreating}
10691068
newC := &model.Collection{TenantID: "2", CollectionID: collectionID, State: pb.CollectionState_CollectionCreated}
1070-
err := kc.AlterCollection(ctx, oldC, newC, metastore.MODIFY, 0)
1069+
err := kc.AlterCollection(ctx, oldC, newC, metastore.MODIFY, 0, true)
10711070
assert.Error(t, err)
10721071
})
10731072

10741073
t.Run("modify db name", func(t *testing.T) {
10751074
var collectionID int64 = 1
10761075
snapshot := kv.NewMockSnapshotKV()
1077-
snapshot.MultiSaveAndRemoveFunc = func(ctx context.Context, saves map[string]string, removals []string, ts typeutil.Timestamp) error {
1078-
assert.ElementsMatch(t, []string{BuildCollectionKey(0, collectionID)}, removals)
1079-
assert.Equal(t, len(saves), 1)
1080-
assert.Contains(t, maps.Keys(saves), BuildCollectionKey(1, collectionID))
1081-
return nil
1082-
}
10831076

10841077
kc := NewCatalog(nil, snapshot).(*Catalog)
10851078
ctx := context.Background()
10861079
oldC := &model.Collection{DBID: 0, CollectionID: collectionID, State: pb.CollectionState_CollectionCreated}
10871080
newC := &model.Collection{DBID: 1, CollectionID: collectionID, State: pb.CollectionState_CollectionCreated}
1088-
err := kc.AlterCollection(ctx, oldC, newC, metastore.MODIFY, 0)
1081+
err := kc.AlterCollection(ctx, oldC, newC, metastore.MODIFY, 0, true)
1082+
assert.Error(t, err)
1083+
})
1084+
1085+
t.Run("modify 64 fields", func(t *testing.T) {
1086+
var collectionID int64 = 1
1087+
snapshot := kv.NewMockSnapshotKV()
1088+
snapshot.MultiSaveFunc = func(ctx context.Context, saves map[string]string, ts typeutil.Timestamp) error {
1089+
assert.LessOrEqual(t, len(saves), 64)
1090+
return nil
1091+
}
1092+
1093+
kc := NewCatalog(nil, snapshot).(*Catalog)
1094+
ctx := context.Background()
1095+
// 2 system fields + 64 user fields
1096+
fields := make([]*model.Field, 66)
1097+
for i := range fields {
1098+
fields[i] = &model.Field{
1099+
FieldID: int64(i),
1100+
}
1101+
}
1102+
oldC := &model.Collection{DBID: 0, CollectionID: collectionID, State: pb.CollectionState_CollectionCreated, Fields: fields}
1103+
newC := &model.Collection{DBID: 0, CollectionID: collectionID, State: pb.CollectionState_CollectionCreated, Fields: fields}
1104+
err := kc.AlterCollection(ctx, oldC, newC, metastore.MODIFY, 0, true)
1105+
assert.NoError(t, err)
1106+
})
1107+
}
1108+
1109+
func TestCatalog_AlterCollectionDB(t *testing.T) {
1110+
snapshot := kv.NewMockSnapshotKV()
1111+
kvs := map[string]string{}
1112+
snapshot.MultiSaveFunc = func(ctx context.Context, saveKvs map[string]string, _ typeutil.Timestamp) error {
1113+
for k, v := range saveKvs {
1114+
kvs[k] = v
1115+
}
1116+
return nil
1117+
}
1118+
kc := NewCatalog(nil, snapshot).(*Catalog)
1119+
ctx := context.Background()
1120+
var collectionID int64 = 1
1121+
t.Run("rename tencentid", func(t *testing.T) {
1122+
oldC := &model.Collection{TenantID: "0", CollectionID: collectionID, State: pb.CollectionState_CollectionCreated, DBID: 0}
1123+
newC := &model.Collection{TenantID: "1", CollectionID: collectionID, State: pb.CollectionState_CollectionCreated, DBID: 1}
1124+
err := kc.AlterCollectionDB(ctx, oldC, newC, 0)
1125+
assert.Error(t, err)
1126+
})
1127+
1128+
t.Run("modify db", func(t *testing.T) {
1129+
oldC := &model.Collection{CollectionID: collectionID, State: pb.CollectionState_CollectionCreated, DBID: 0}
1130+
newC := &model.Collection{CollectionID: collectionID, State: pb.CollectionState_CollectionCreated, DBID: 1}
1131+
err := kc.AlterCollectionDB(ctx, oldC, newC, 0)
10891132
assert.NoError(t, err)
10901133
})
10911134
}

‎internal/metastore/mocks/mock_datacoord_catalog.go‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎internal/metastore/mocks/mock_querycoord_catalog.go‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎internal/metastore/mocks/mock_rootcoord_catalog.go‎

Lines changed: 63 additions & 13 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)