From f005cb646a79a445c7e610bd6eed8867c6a60465 Mon Sep 17 00:00:00 2001 From: yangxuan Date: Fri, 15 Nov 2024 14:42:17 +0800 Subject: [PATCH] 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)