From f005cb646a79a445c7e610bd6eed8867c6a60465 Mon Sep 17 00:00:00 2001 From: yangxuan Date: Fri, 15 Nov 2024 14:42:17 +0800 Subject: [PATCH 1/3] fix: cannot pass consistency level for delete Also optinal variables default value should always be None. And empty str is always invalid variable See also: #2327 Signed-off-by: yangxuan --- pymilvus/client/grpc_handler.py | 12 ++++----- pymilvus/client/prepare.py | 34 +++++++++++++------------ pymilvus/milvus_client/milvus_client.py | 5 ++-- tests/test_prepare.py | 17 ++++++++++--- 4 files changed, 39 insertions(+), 29 deletions(-) diff --git a/pymilvus/client/grpc_handler.py b/pymilvus/client/grpc_handler.py index ad360e42b..a38c54b70 100644 --- a/pymilvus/client/grpc_handler.py +++ b/pymilvus/client/grpc_handler.py @@ -596,17 +596,15 @@ def delete( check_pass_param(collection_name=collection_name, timeout=timeout) try: req = Prepare.delete_request( - collection_name, - partition_name, - expression, - consistency_level=kwargs.get("consistency_level", 0), - param_name=kwargs.pop("param_name", None), + collection_name=collection_name, + expression=expression, + partition_name=partition_name, + consistency_level=kwargs.pop("consistency_level", 0), **kwargs, ) future = self._stub.Delete.future(req, timeout=timeout) - if kwargs.get("_async", False): - cb = kwargs.get("_callback") + cb = kwargs.pop("_callback") f = MutationFuture(future, cb, timeout=timeout, **kwargs) f.add_callback(ts_utils.update_ts_on_mutation(collection_name)) return f diff --git a/pymilvus/client/prepare.py b/pymilvus/client/prepare.py index 1427c8ec9..3c6d8ae3f 100644 --- a/pymilvus/client/prepare.py +++ b/pymilvus/client/prepare.py @@ -734,29 +734,31 @@ def batch_upsert_param( def delete_request( cls, collection_name: str, + expression: str, partition_name: str, - expr: str, - consistency_level: Optional[Union[int, str]], + consistency_level: Optional[Union[int, str]] = None, **kwargs, ): - def check_str(instr: str, prefix: str): - if instr is None: - raise ParamError(message=f"{prefix} cannot be None") - if not isinstance(instr, str): - raise ParamError(message=f"{prefix} value {instr} is illegal") - if len(instr) == 0: - raise ParamError(message=f"{prefix} cannot be empty") - - check_str(collection_name, "collection_name") - if partition_name is not None and partition_name != "": - check_str(partition_name, "partition_name") - param_name = kwargs.get("param_name", "expr") - check_str(expr, param_name) + def valid_str(var: Any) -> True: + return isinstance(var, str) and len(var) > 0 + + if not valid_str(collection_name): + raise ParamError( + message=f"collection_name {collection_name} is illegal, expect none empty str" + ) + + if not valid_str(expression): + raise ParamError(message=f"expression {expression} is illegal, expect none empty str") + + if partition_name is not None and not valid_str(partition_name): + raise ParamError( + message=f"partition_name {partition_name} is illegal, expect none empty str" + ) return milvus_types.DeleteRequest( collection_name=collection_name, partition_name=partition_name, - expr=expr, + expr=expression, consistency_level=get_consistency_level(consistency_level), expr_template_values=cls.prepare_expression_template(kwargs.get("expr_params", {})), ) diff --git a/pymilvus/milvus_client/milvus_client.py b/pymilvus/milvus_client/milvus_client.py index 4f593eef6..66fcd2ff1 100644 --- a/pymilvus/milvus_client/milvus_client.py +++ b/pymilvus/milvus_client/milvus_client.py @@ -543,8 +543,8 @@ def delete( collection_name: str, ids: Optional[Union[list, str, int]] = None, timeout: Optional[float] = None, - filter: Optional[str] = "", - partition_name: Optional[str] = "", + filter: Optional[str] = None, + partition_name: Optional[str] = None, **kwargs, ) -> Dict: """Delete entries in the collection by their pk or by filter. @@ -616,7 +616,6 @@ def delete( expr, partition_name, timeout=timeout, - param_name="filter or ids", expr_params=kwargs.pop("filter_params", {}), **kwargs, ) diff --git a/tests/test_prepare.py b/tests/test_prepare.py index 222347b12..c5c1f300d 100644 --- a/tests/test_prepare.py +++ b/tests/test_prepare.py @@ -8,6 +8,17 @@ class TestPrepare: + @pytest.mark.parametrize("coll_name", [None, "", -1, 1.1, []]) + def test_delete_request_wrong_coll_name(self, coll_name: str): + with pytest.raises(MilvusException): + Prepare.delete_request(coll_name, "id>1", None, 0) + + @pytest.mark.parametrize("part_name", ["", -1, 1.1, []]) + def test_delete_request_wrong_part_name(self, part_name): + with pytest.raises(MilvusException): + Prepare.delete_request("coll", "id>1", part_name, 0) + + def test_search_requests_with_expr_offset(self): fields = [ FieldSchema("pk", DataType.INT64, is_primary=True), @@ -42,7 +53,7 @@ def test_search_requests_with_expr_offset(self): params = json.loads(p.value) if PAGE_RETAIN_ORDER_FIELD in params: page_retain_order_exists = True - assert params[PAGE_RETAIN_ORDER_FIELD] == True + assert params[PAGE_RETAIN_ORDER_FIELD] is True assert offset_exists is True assert page_retain_order_exists is True @@ -112,7 +123,7 @@ def test_get_schema_from_collection_schema(self): c_schema = Prepare.get_schema_from_collection_schema("random", schema) - assert c_schema.enable_dynamic_field == False + assert c_schema.enable_dynamic_field is False assert c_schema.name == "random" assert len(c_schema.fields) == 2 assert c_schema.fields[0].name == "field_vector" @@ -190,7 +201,7 @@ def test_row_insert_param_with_auto_id(self): ] Prepare.row_insert_param("", rows, "", fields_info=schema.to_dict()["fields"], enable_dynamic=True) - + def test_row_insert_param_with_none(self): import numpy as np rng = np.random.default_rng(seed=19530) From 5aa7dc99c85c5fa687457c90de0c14f84111c010 Mon Sep 17 00:00:00 2001 From: yangxuan Date: Mon, 18 Nov 2024 17:58:56 +0800 Subject: [PATCH 2/3] fix check Signed-off-by: yangxuan --- pymilvus/client/check.py | 23 ++++++++++++++- pymilvus/client/grpc_handler.py | 2 +- pymilvus/client/prepare.py | 28 ++++++------------ pymilvus/milvus_client/milvus_client.py | 38 +++++++++++++------------ tests/test_prepare.py | 7 +++-- 5 files changed, 56 insertions(+), 42 deletions(-) diff --git a/pymilvus/client/check.py b/pymilvus/client/check.py index a7a698b30..2df3ab910 100644 --- a/pymilvus/client/check.py +++ b/pymilvus/client/check.py @@ -9,6 +9,27 @@ from .singleton_utils import Singleton +def validate_strs(**kwargs): + """ validate if all values are legal non-emtpy str """ + invalid_pair = {k: v for k, v in kwargs.items() if not validate_str(v)} + if invalid_pair: + msg = f"Illegal str variables: {invalid_pair}, expect non-empty str" + raise ParamError(message=msg) + + +def validate_nullable_strs(**kwargs): + """ validate if all values are either None or legal non-empty str """ + invalid_pair = {k: v for k, v in kwargs.items() if v is not None and not validate_str(v)} + if invalid_pair: + msg = f"Illegal nullable str variables: {invalid_pair}, expect None or non-empty str" + raise ParamError(message=msg) + + +def validate_str(var: Any) -> bool: + """ check if a variable is legal non-empty str """ + return var and isinstance(var, str) + + def is_legal_address(addr: Any) -> bool: if not isinstance(addr, str): return False @@ -65,7 +86,7 @@ def is_legal_index_size(index_size: Any) -> bool: def is_legal_table_name(table_name: Any) -> bool: - return table_name and isinstance(table_name, str) + return validate_str(table_name) def is_legal_db_name(db_name: Any) -> bool: diff --git a/pymilvus/client/grpc_handler.py b/pymilvus/client/grpc_handler.py index a38c54b70..eccba90db 100644 --- a/pymilvus/client/grpc_handler.py +++ b/pymilvus/client/grpc_handler.py @@ -597,7 +597,7 @@ def delete( try: req = Prepare.delete_request( collection_name=collection_name, - expression=expression, + filter=expression, partition_name=partition_name, consistency_level=kwargs.pop("consistency_level", 0), **kwargs, diff --git a/pymilvus/client/prepare.py b/pymilvus/client/prepare.py index 3c6d8ae3f..9a265dc05 100644 --- a/pymilvus/client/prepare.py +++ b/pymilvus/client/prepare.py @@ -12,7 +12,7 @@ from pymilvus.orm.schema import CollectionSchema from pymilvus.orm.types import infer_dtype_by_scalar_data -from . import __version__, blob, entity_helper, ts_utils, utils +from . import __version__, blob, check, entity_helper, ts_utils, utils from .check import check_pass_param, is_legal_collection_properties from .constants import ( DEFAULT_CONSISTENCY_LEVEL, @@ -734,31 +734,21 @@ def batch_upsert_param( def delete_request( cls, collection_name: str, - expression: str, - partition_name: str, + filter: str, + partition_name: Optional[str] = None, consistency_level: Optional[Union[int, str]] = None, **kwargs, ): - def valid_str(var: Any) -> True: - return isinstance(var, str) and len(var) > 0 - - if not valid_str(collection_name): - raise ParamError( - message=f"collection_name {collection_name} is illegal, expect none empty str" - ) - - if not valid_str(expression): - raise ParamError(message=f"expression {expression} is illegal, expect none empty str") - - if partition_name is not None and not valid_str(partition_name): - raise ParamError( - message=f"partition_name {partition_name} is illegal, expect none empty str" - ) + check.validate_strs( + collection_name=collection_name, + filter=filter, + ) + check.validate_nullable_strs(partition_name=partition_name) return milvus_types.DeleteRequest( collection_name=collection_name, partition_name=partition_name, - expr=expression, + expr=filter, consistency_level=get_consistency_level(consistency_level), expr_template_values=cls.prepare_expression_template(kwargs.get("expr_params", {})), ) diff --git a/pymilvus/milvus_client/milvus_client.py b/pymilvus/milvus_client/milvus_client.py index 66fcd2ff1..6094981fa 100644 --- a/pymilvus/milvus_client/milvus_client.py +++ b/pymilvus/milvus_client/milvus_client.py @@ -400,8 +400,8 @@ def search( limit=limit, output_fields=output_fields, partition_names=partition_names, - timeout=timeout, expr_params=kwargs.pop("filter_params", {}), + timeout=timeout, **kwargs, ) except Exception as ex: @@ -546,7 +546,7 @@ def delete( filter: Optional[str] = None, partition_name: Optional[str] = None, **kwargs, - ) -> Dict: + ) -> Dict[str, int]: """Delete entries in the collection by their pk or by filter. Starting from version 2.3.2, Milvus no longer includes the primary keys in the result @@ -558,14 +558,17 @@ def delete( Milvus(previous 2.3.2) is not empty, the list of primary keys is still returned. Args: - ids (list, str, int): The pk's to delete. Depending on pk_field type it can be int - or str or alist of either. Default to None. - filter(str, optional): A filter to use for the deletion. Defaults to empty. + ids (list, str, int, optional): The pk's to delete. + Depending on pk_field type it can be int or str or a list of either. + Default to None. + filter(str, optional): A filter to use for the deletion. Defaults to none. timeout (int, optional): Timeout to use, overides the client level assigned at init. Defaults to None. + Note: You need to passin either ids or filter, and they cannot be used at the same time. + Returns: - Dict: Number of rows that were deleted. + Dict: with key 'deleted_count' and value number of rows that were deleted. """ pks = kwargs.get("pks", []) if isinstance(pks, (int, str)): @@ -589,34 +592,32 @@ def delete( msg = f"wrong type of argument ids, expect list, int or str, got '{type(ids).__name__}'" raise TypeError(msg) + # validate ambiguous delete filter param before describe collection rpc + if filter and len(pks) > 0: + raise ParamError(message=ExceptionsMessage.AmbiguousDeleteFilterParam) + expr = "" conn = self._get_connection() - if pks: + if len(pks) > 0: try: schema_dict = conn.describe_collection(collection_name, timeout=timeout, **kwargs) except Exception as ex: logger.error("Failed to describe collection: %s", collection_name) raise ex from ex - expr = self._pack_pks_expr(schema_dict, pks) - - if filter: - if expr: - raise ParamError(message=ExceptionsMessage.AmbiguousDeleteFilterParam) - + else: if not isinstance(filter, str): raise DataTypeNotMatchException(message=ExceptionsMessage.ExprType % type(filter)) - expr = filter ret_pks = [] try: res = conn.delete( - collection_name, - expr, - partition_name, - timeout=timeout, + collection_name=collection_name, + expression=expr, + partition_name=partition_name, expr_params=kwargs.pop("filter_params", {}), + timeout=timeout, **kwargs, ) if res.primary_keys: @@ -625,6 +626,7 @@ def delete( logger.error("Failed to delete primary keys in collection: %s", collection_name) raise ex from ex + # compatible with deletions that returns primary keys if ret_pks: return ret_pks diff --git a/tests/test_prepare.py b/tests/test_prepare.py index c5c1f300d..171afaddd 100644 --- a/tests/test_prepare.py +++ b/tests/test_prepare.py @@ -9,11 +9,12 @@ class TestPrepare: @pytest.mark.parametrize("coll_name", [None, "", -1, 1.1, []]) - def test_delete_request_wrong_coll_name(self, coll_name: str): + @pytest.mark.parametrize("expr", [None, "", -1, 1.1, []]) + def test_delete_request_wrong_coll_name(self, coll_name: str, expr: str): with pytest.raises(MilvusException): - Prepare.delete_request(coll_name, "id>1", None, 0) + Prepare.delete_request(coll_name, expr, None, 0) - @pytest.mark.parametrize("part_name", ["", -1, 1.1, []]) + @pytest.mark.parametrize("part_name", []) def test_delete_request_wrong_part_name(self, part_name): with pytest.raises(MilvusException): Prepare.delete_request("coll", "id>1", part_name, 0) From 02bbf1d0a36a36415dc8b08c485ad25332aa29b8 Mon Sep 17 00:00:00 2001 From: yangxuan Date: Mon, 18 Nov 2024 18:15:02 +0800 Subject: [PATCH 3/3] fix coding style Signed-off-by: yangxuan --- pymilvus/client/check.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pymilvus/client/check.py b/pymilvus/client/check.py index 2df3ab910..9cae7b6f7 100644 --- a/pymilvus/client/check.py +++ b/pymilvus/client/check.py @@ -10,7 +10,7 @@ def validate_strs(**kwargs): - """ validate if all values are legal non-emtpy str """ + """validate if all values are legal non-emtpy str""" invalid_pair = {k: v for k, v in kwargs.items() if not validate_str(v)} if invalid_pair: msg = f"Illegal str variables: {invalid_pair}, expect non-empty str" @@ -18,7 +18,7 @@ def validate_strs(**kwargs): def validate_nullable_strs(**kwargs): - """ validate if all values are either None or legal non-empty str """ + """validate if all values are either None or legal non-empty str""" invalid_pair = {k: v for k, v in kwargs.items() if v is not None and not validate_str(v)} if invalid_pair: msg = f"Illegal nullable str variables: {invalid_pair}, expect None or non-empty str" @@ -26,7 +26,7 @@ def validate_nullable_strs(**kwargs): def validate_str(var: Any) -> bool: - """ check if a variable is legal non-empty str """ + """check if a variable is legal non-empty str""" return var and isinstance(var, str)