Skip to content

feat: support to drop properties of field #41954

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

Merged
merged 1 commit into from
May 27, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion client/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ require (
github.com/blang/semver/v4 v4.0.0
github.com/cockroachdb/errors v1.9.1
github.com/grpc-ecosystem/go-grpc-middleware v1.3.0
github.com/milvus-io/milvus-proto/go-api/v2 v2.5.12
github.com/milvus-io/milvus-proto/go-api/v2 v2.5.13-0.20250520065018-13f9a20ffaad
github.com/milvus-io/milvus/pkg/v2 v2.5.7
github.com/quasilyte/go-ruleguard/dsl v0.3.22
github.com/samber/lo v1.27.0
Expand Down
4 changes: 2 additions & 2 deletions client/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -318,8 +318,8 @@ github.com/matttproud/golang_protobuf_extensions v1.0.4/go.mod h1:BSXmuO+STAnVfr
github.com/mediocregopher/radix/v3 v3.4.2/go.mod h1:8FL3F6UQRXHXIBSPUs5h0RybMF8i4n7wVopoX3x7Bv8=
github.com/microcosm-cc/bluemonday v1.0.2/go.mod h1:iVP4YcDBq+n/5fb23BhYFvIMq/leAFZyRl6bYmGDlGc=
github.com/miekg/dns v1.0.14/go.mod h1:W1PPwlIAgtquWBMBEV9nkV9Cazfe8ScdGz/Lj7v3Nrg=
github.com/milvus-io/milvus-proto/go-api/v2 v2.5.12 h1:zRMCf6W4kzBNqZb1CMmmfetUAG0y3hlT5Ow0H9MaASM=
github.com/milvus-io/milvus-proto/go-api/v2 v2.5.12/go.mod h1:/6UT4zZl6awVeXLeE7UGDWZvXj3IWkRsh3mqsn0DiAs=
github.com/milvus-io/milvus-proto/go-api/v2 v2.5.13-0.20250520065018-13f9a20ffaad h1:tkSvshW0g1nbf7gPIsku838FFvrlucnPtAXYBiGuTAs=
github.com/milvus-io/milvus-proto/go-api/v2 v2.5.13-0.20250520065018-13f9a20ffaad/go.mod h1:/6UT4zZl6awVeXLeE7UGDWZvXj3IWkRsh3mqsn0DiAs=
github.com/milvus-io/milvus/pkg/v2 v2.5.7 h1:b45jq1s1v03AekFucs2/dkkXohB57gEx7gspJuAkfbY=
github.com/milvus-io/milvus/pkg/v2 v2.5.7/go.mod h1:pImw1IGNS7k/5yvlZV2tZi5vZu1VQRlQij+r39d+XnI=
github.com/mitchellh/cli v1.0.0/go.mod h1:hNIlj7HEI86fIcpObd7a0FcrxTWetlwJDGcceTlRvqc=
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ require (
github.com/grpc-ecosystem/go-grpc-middleware v1.3.0
github.com/klauspost/compress v1.18.0
github.com/mgutz/ansi v0.0.0-20200706080929-d51e80ef957d
github.com/milvus-io/milvus-proto/go-api/v2 v2.5.12
github.com/milvus-io/milvus-proto/go-api/v2 v2.5.13-0.20250520065018-13f9a20ffaad
github.com/minio/minio-go/v7 v7.0.73
github.com/panjf2000/ants/v2 v2.11.3 // indirect
github.com/pingcap/log v1.1.1-0.20221015072633-39906604fb81
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -634,8 +634,8 @@ github.com/milvus-io/cgosymbolizer v0.0.0-20240722103217-b7dee0e50119 h1:9VXijWu
github.com/milvus-io/cgosymbolizer v0.0.0-20240722103217-b7dee0e50119/go.mod h1:DvXTE/K/RtHehxU8/GtDs4vFtfw64jJ3PaCnFri8CRg=
github.com/milvus-io/gorocksdb v0.0.0-20220624081344-8c5f4212846b h1:TfeY0NxYxZzUfIfYe5qYDBzt4ZYRqzUjTR6CvUzjat8=
github.com/milvus-io/gorocksdb v0.0.0-20220624081344-8c5f4212846b/go.mod h1:iwW+9cWfIzzDseEBCCeDSN5SD16Tidvy8cwQ7ZY8Qj4=
github.com/milvus-io/milvus-proto/go-api/v2 v2.5.12 h1:zRMCf6W4kzBNqZb1CMmmfetUAG0y3hlT5Ow0H9MaASM=
github.com/milvus-io/milvus-proto/go-api/v2 v2.5.12/go.mod h1:/6UT4zZl6awVeXLeE7UGDWZvXj3IWkRsh3mqsn0DiAs=
github.com/milvus-io/milvus-proto/go-api/v2 v2.5.13-0.20250520065018-13f9a20ffaad h1:tkSvshW0g1nbf7gPIsku838FFvrlucnPtAXYBiGuTAs=
github.com/milvus-io/milvus-proto/go-api/v2 v2.5.13-0.20250520065018-13f9a20ffaad/go.mod h1:/6UT4zZl6awVeXLeE7UGDWZvXj3IWkRsh3mqsn0DiAs=
github.com/milvus-io/pulsar-client-go v0.12.1 h1:O2JZp1tsYiO7C0MQ4hrUY/aJXnn2Gry6hpm7UodghmE=
github.com/milvus-io/pulsar-client-go v0.12.1/go.mod h1:dkutuH4oS2pXiGm+Ti7fQZ4MRjrMPZ8IJeEGAWMeckk=
github.com/minio/asm2plan9s v0.0.0-20200509001527-cdd76441f9d8 h1:AMFGa4R4MiIpspGNG7Z948v4n35fFGB3RR3G/ry4FWs=
Expand Down
78 changes: 63 additions & 15 deletions internal/proxy/task.go
Original file line number Diff line number Diff line change
Expand Up @@ -1157,30 +1157,48 @@
MmapEnabledKey = "mmap_enabled"
)

var allowedProps = []string{
var allowedAlterProps = []string{
common.MaxLengthKey,
common.MmapEnabledKey,
common.MaxCapacityKey,
}

func IsKeyAllowed(key string) bool {
for _, allowedKey := range allowedProps {
var allowedDropProps = []string{
common.MmapEnabledKey,
}

func IsKeyAllowAlter(key string) bool {
for _, allowedKey := range allowedAlterProps {
if key == allowedKey {
return true
}
}
return false
}

func IsKeyAllowDrop(key string) bool {
for _, allowedKey := range allowedDropProps {
if key == allowedKey {
return true
}
}
return false
}

func updateKey(key string) string {
var updatedKey string
if key == MmapEnabledKey {
updatedKey = common.MmapEnabledKey
} else {
updatedKey = key
}
return updatedKey
}

func updatePropertiesKeys(oldProps []*commonpb.KeyValuePair) []*commonpb.KeyValuePair {
props := make(map[string]string)
for _, prop := range oldProps {
var updatedKey string
if prop.Key == MmapEnabledKey {
updatedKey = common.MmapEnabledKey
} else {
updatedKey = prop.Key
}
updatedKey := updateKey(prop.Key)
props[updatedKey] = prop.Value
}

Expand All @@ -1200,22 +1218,31 @@
if err != nil {
return err
}

isCollectionLoadedFn := func() (bool, error) {
collectionID, err := globalMetaCache.GetCollectionID(ctx, t.GetDbName(), t.CollectionName)
if err != nil {
return false, err
}

Check warning on line 1226 in internal/proxy/task.go

View check run for this annotation

Codecov / codecov/patch

internal/proxy/task.go#L1225-L1226

Added lines #L1225 - L1226 were not covered by tests
loaded, err1 := isCollectionLoaded(ctx, t.queryCoord, collectionID)
if err1 != nil {
return false, err1
}

Check warning on line 1230 in internal/proxy/task.go

View check run for this annotation

Codecov / codecov/patch

internal/proxy/task.go#L1229-L1230

Added lines #L1229 - L1230 were not covered by tests
return loaded, nil
}

t.Properties = updatePropertiesKeys(t.Properties)
for _, prop := range t.Properties {
if !IsKeyAllowed(prop.Key) {
if !IsKeyAllowAlter(prop.Key) {
return merr.WrapErrParameterInvalidMsg("%s does not allow update in collection field param", prop.Key)
}
// Check the value type based on the key
switch prop.Key {
case common.MmapEnabledKey:
collectionID, err := globalMetaCache.GetCollectionID(ctx, t.GetDbName(), t.CollectionName)
loaded, err := isCollectionLoadedFn()
if err != nil {
return err
}
loaded, err1 := isCollectionLoaded(ctx, t.queryCoord, collectionID)
if err1 != nil {
return err1
}
if loaded {
return merr.WrapErrCollectionLoaded(t.CollectionName, "can not alter collection field properties if collection loaded")
}
Expand Down Expand Up @@ -1266,6 +1293,27 @@
}
}

deleteKeys := make([]string, 0)
for _, key := range t.DeleteKeys {
updatedKey := updateKey(key)
if !IsKeyAllowDrop(updatedKey) {
return merr.WrapErrParameterInvalidMsg("%s is not allowed to drop in collection field param", key)
}

if updatedKey == common.MmapEnabledKey {
loaded, err := isCollectionLoadedFn()
if err != nil {
return err
}

Check warning on line 1307 in internal/proxy/task.go

View check run for this annotation

Codecov / codecov/patch

internal/proxy/task.go#L1306-L1307

Added lines #L1306 - L1307 were not covered by tests
if loaded {
return merr.WrapErrCollectionLoaded(t.CollectionName, "can not drop collection field properties if collection loaded")
}
}

deleteKeys = append(deleteKeys, updatedKey)
}
t.DeleteKeys = deleteKeys

return nil
}

Expand Down
65 changes: 61 additions & 4 deletions internal/proxy/task_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4451,6 +4451,8 @@ func TestAlterCollectionFieldCheckLoaded(t *testing.T) {
CollectionIDs: []int64{resp.CollectionID},
InMemoryPercentages: []int64{100},
}, nil)

// update property "mmap.enabled" but the collection is loaded
task := &alterCollectionFieldTask{
AlterCollectionFieldRequest: &milvuspb.AlterCollectionFieldRequest{
Base: &commonpb.MsgBase{},
Expand All @@ -4461,6 +4463,18 @@ func TestAlterCollectionFieldCheckLoaded(t *testing.T) {
}
err = task.PreExecute(context.Background())
assert.Equal(t, merr.Code(merr.ErrCollectionLoaded), merr.Code(err))

// delete property "mmap.enabled" but the collection is loaded
task = &alterCollectionFieldTask{
AlterCollectionFieldRequest: &milvuspb.AlterCollectionFieldRequest{
Base: &commonpb.MsgBase{},
CollectionName: collectionName,
DeleteKeys: []string{common.MmapEnabledKey},
},
queryCoord: qc,
}
err = task.PreExecute(context.Background())
assert.Equal(t, merr.Code(merr.ErrCollectionLoaded), merr.Code(err))
}

func TestAlterCollectionField1(t *testing.T) {
Expand Down Expand Up @@ -4504,18 +4518,25 @@ func TestAlterCollectionField1(t *testing.T) {
ShardsNum: 1,
}
rc.CreateCollection(context.Background(), createColReq)
resp, err := rc.DescribeCollection(context.Background(), &milvuspb.DescribeCollectionRequest{CollectionName: collectionName})
assert.NoError(t, err)

// The collection is not loaded
qc.EXPECT().ShowCollections(mock.Anything, mock.Anything).Return(&querypb.ShowCollectionsResponse{
Status: &commonpb.Status{ErrorCode: commonpb.ErrorCode_Success},
CollectionIDs: []int64{resp.CollectionID},
InMemoryPercentages: []int64{100},
CollectionIDs: []int64{},
InMemoryPercentages: []int64{},
}, nil)

// Test cases
// 1. the collection is not loaded, updating properties is allowed, deleting mmap.enabled/mmap_enabled is allowed
// 2. max_length can only be updated on varchar field or arrar field with varchar element
// 3. max_capacity can only be updated on array field
// 4. invalid number for max_length/max_capacity is not allowed
// 5. not allow to delete max_length/max_capacity
tests := []struct {
name string
fieldName string
properties []*commonpb.KeyValuePair
deleteKeys []string
expectError bool
errCode int32
}{
Expand Down Expand Up @@ -4598,6 +4619,41 @@ func TestAlterCollectionField1(t *testing.T) {
expectError: true,
errCode: merr.Code(merr.ErrParameterInvalid),
},
{
name: "not allow to update max_length on non-varchar field",
fieldName: "array_field",
properties: []*commonpb.KeyValuePair{
{Key: common.MaxLengthKey, Value: "10"},
},
expectError: true,
errCode: merr.Code(merr.ErrParameterInvalid),
},
{
name: "delete mmap.enabled is allowed",
fieldName: "string_field",
deleteKeys: []string{common.MmapEnabledKey},
expectError: false,
},
{
name: "delete mmap_enabled is allowed",
fieldName: "string_field",
deleteKeys: []string{MmapEnabledKey},
expectError: false,
},
{
name: "delete max_length is not allowed",
fieldName: "string_field",
deleteKeys: []string{common.MaxLengthKey},
expectError: true,
errCode: merr.Code(merr.ErrParameterInvalid),
},
{
name: "delete max_capacity is not allowed",
fieldName: "array_field",
deleteKeys: []string{common.MaxCapacityKey},
expectError: true,
errCode: merr.Code(merr.ErrParameterInvalid),
},
}

for _, test := range tests {
Expand All @@ -4608,6 +4664,7 @@ func TestAlterCollectionField1(t *testing.T) {
CollectionName: collectionName,
FieldName: test.fieldName,
Properties: test.properties,
DeleteKeys: test.deleteKeys,
},
queryCoord: qc,
}
Expand Down
12 changes: 9 additions & 3 deletions internal/rootcoord/alter_collection_task.go
Original file line number Diff line number Diff line change
Expand Up @@ -274,8 +274,8 @@ func (a *alterCollectionFieldTask) Prepare(ctx context.Context) error {
}

func (a *alterCollectionFieldTask) Execute(ctx context.Context) error {
if a.Req.GetProperties() == nil {
return errors.New("only support alter collection properties, but collection field properties is empty")
if len(a.Req.GetProperties()) == 0 && len(a.Req.GetDeleteKeys()) == 0 {
return errors.New("The field properties to alter and keys to delete must not be empty at the same time")
}

oldColl, err := a.core.meta.GetCollectionByName(ctx, a.Req.GetDbName(), a.Req.GetCollectionName(), a.ts)
Expand Down Expand Up @@ -314,7 +314,13 @@ func executeAlterCollectionFieldTaskSteps(ctx context.Context,
) error {
var err error
filedName := request.GetFieldName()
newFieldProperties := UpdateFieldPropertyParams(oldFieldProperties, request.GetProperties())

var newFieldProperties []*commonpb.KeyValuePair
if len(request.Properties) > 0 {
newFieldProperties = UpdateFieldPropertyParams(oldFieldProperties, request.GetProperties())
} else if len(request.DeleteKeys) > 0 {
newFieldProperties = DeleteProperties(oldFieldProperties, request.GetDeleteKeys())
}
oldColl := col.Clone()
err = ResetFieldProperties(oldColl, filedName, oldFieldProperties)
if err != nil {
Expand Down
Loading
Loading