From 24c88833693d3e22b6c73ed9e25825a2b3d28ebe Mon Sep 17 00:00:00 2001 From: Xianhui Lin <35839735+JsDove@users.noreply.github.com> Date: Thu, 12 Dec 2024 21:46:44 +0800 Subject: [PATCH] enhance: [2.4]altercollection validation logic (#38421) altercollection validation logic issue: https://github.com/milvus-io/milvus/issues/37436 pr: https://github.com/milvus-io/milvus/pull/38419 --------- Signed-off-by: Xianhui.Lin --- internal/proxy/task.go | 68 +++++++++++++++++---- internal/proxy/task_index.go | 4 +- internal/rootcoord/alter_collection_task.go | 28 --------- tests/python_client/requirements.txt | 4 +- 4 files changed, 60 insertions(+), 44 deletions(-) diff --git a/internal/proxy/task.go b/internal/proxy/task.go index 514d75668a299..c7e12f0071994 100644 --- a/internal/proxy/task.go +++ b/internal/proxy/task.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "math" + "strconv" "time" "github.com/cockroachdb/errors" @@ -982,7 +983,7 @@ func (t *alterCollectionTask) PreExecute(ctx context.Context) error { return err } if loaded { - return merr.WrapErrCollectionLoaded(" %s %s can not delete mmap properties if collection loaded", t.CollectionName, key) + return merr.WrapErrCollectionLoaded(t.CollectionName, "can not delete mmap properties if collection loaded") } } } @@ -1108,24 +1109,67 @@ func (t *alterCollectionFieldTask) OnEnqueue() error { return nil } +var allowedProps = []string{ + common.MaxLengthKey, + common.MmapEnabledKey, +} + +func IsKeyAllowed(key string) bool { + for _, allowedKey := range allowedProps { + if key == allowedKey { + return true + } + } + return false +} + func (t *alterCollectionFieldTask) PreExecute(ctx context.Context) error { collSchema, err := globalMetaCache.GetCollectionSchema(ctx, t.GetDbName(), t.CollectionName) if err != nil { return err } - IsStringType := false - fieldName := "" - var dataType int32 - for _, field := range collSchema.Fields { - if field.GetName() == t.FieldName && (typeutil.IsStringType(field.DataType) || typeutil.IsArrayContainStringElementType(field.DataType, field.ElementType)) { - IsStringType = true - fieldName = field.GetName() - dataType = int32(field.DataType) + for _, prop := range t.Properties { + if !IsKeyAllowed(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) + 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") + } + case common.MaxLengthKey: + IsStringType := false + fieldName := "" + var dataType int32 + for _, field := range collSchema.Fields { + if field.GetName() == t.FieldName && (typeutil.IsStringType(field.DataType) || typeutil.IsArrayContainStringElementType(field.DataType, field.ElementType)) { + IsStringType = true + fieldName = field.GetName() + dataType = int32(field.DataType) + } + } + if !IsStringType { + return merr.WrapErrParameterInvalid(fieldName, "%s can not modify the maxlength for non-string types", schemapb.DataType_name[dataType]) + } + value, err := strconv.Atoi(prop.Value) + if err != nil { + return merr.WrapErrParameterInvalid("%s should be an integer, but got %T", prop.Key, prop.Value) + } + + if value > defaultMaxVarCharLength { + return merr.WrapErrParameterInvalid("%s exceeds the maximum allowed value 65535", prop.Value) + } } - } - if !IsStringType { - return merr.WrapErrParameterInvalid(fieldName, "%s can not modify the maxlength for non-string types", schemapb.DataType_name[dataType]) } return nil diff --git a/internal/proxy/task_index.go b/internal/proxy/task_index.go index 7fd3824d51c59..088e4edd00179 100644 --- a/internal/proxy/task_index.go +++ b/internal/proxy/task_index.go @@ -550,13 +550,13 @@ func (t *alterIndexTask) PreExecute(ctx context.Context) error { if len(t.req.GetExtraParams()) > 0 { for _, param := range t.req.GetExtraParams() { if !indexparams.IsConfigableIndexParam(param.GetKey()) { - return merr.WrapErrParameterInvalidMsg("%s is not configable index param in extraParams", param.GetKey()) + return merr.WrapErrParameterInvalidMsg("%s is not a configable index proptery", param.GetKey()) } } } else if len(t.req.GetDeleteKeys()) > 0 { for _, param := range t.req.GetDeleteKeys() { if !indexparams.IsConfigableIndexParam(param) { - return merr.WrapErrParameterInvalidMsg("%s is not configable index param in deleteKeys", param) + return merr.WrapErrParameterInvalidMsg("%s is not a configable index proptery", param) } } } diff --git a/internal/rootcoord/alter_collection_task.go b/internal/rootcoord/alter_collection_task.go index 001d0fe303d22..da0be55c05280 100644 --- a/internal/rootcoord/alter_collection_task.go +++ b/internal/rootcoord/alter_collection_task.go @@ -163,11 +163,6 @@ func (a *alterCollectionFieldTask) Prepare(ctx context.Context) error { return fmt.Errorf("alter collection field failed, filed name does not exists") } - err := IsValidateUpdatedFieldProps(a.Req.GetProperties()) - if err != nil { - return err - } - return nil } @@ -221,29 +216,6 @@ func (a *alterCollectionFieldTask) Execute(ctx context.Context) error { return redoTask.Execute(ctx) } -var allowedProps = []string{ - common.MaxLengthKey, - common.MmapEnabledKey, -} - -func IsKeyAllowed(key string) bool { - for _, allowedKey := range allowedProps { - if key == allowedKey { - return true - } - } - return false -} - -func IsValidateUpdatedFieldProps(updatedProps []*commonpb.KeyValuePair) error { - for _, prop := range updatedProps { - if !IsKeyAllowed(prop.Key) { - return merr.WrapErrParameterInvalidMsg("%s does not allow update in collection field param", prop.Key) - } - } - return nil -} - func UpdateFieldProperties(coll *model.Collection, fieldName string, updatedProps []*commonpb.KeyValuePair) error { for i, field := range coll.Fields { if field.Name == fieldName { diff --git a/tests/python_client/requirements.txt b/tests/python_client/requirements.txt index be9d9c1be1126..52cd51cb6a577 100644 --- a/tests/python_client/requirements.txt +++ b/tests/python_client/requirements.txt @@ -12,8 +12,8 @@ allure-pytest==2.7.0 pytest-print==0.2.1 pytest-level==0.1.1 pytest-xdist==2.5.0 -pymilvus==2.4.11rc5 -pymilvus[bulk_writer]==2.4.11rc5 +pymilvus==2.4.11rc8 +pymilvus[bulk_writer]==2.4.11rc8 pytest-rerunfailures==9.1.1 git+https://github.com/Projectplace/pytest-tags ndg-httpsclient