From 503a1ce6ac85af627c17444dff52acd7f3b9c2ea Mon Sep 17 00:00:00 2001 From: Michael Graczyk Date: Fri, 20 Sep 2024 11:05:55 -0700 Subject: [PATCH 1/5] feat: add IS_NOT_NULL operator to filters * unit tests --- google/cloud/firestore_v1/base_query.py | 34 +++++++++++++++++-------- tests/unit/v1/test_base_query.py | 22 +++++++++++++--- 2 files changed, 41 insertions(+), 15 deletions(-) diff --git a/google/cloud/firestore_v1/base_query.py b/google/cloud/firestore_v1/base_query.py index 3509bbf17c..70460b733c 100644 --- a/google/cloud/firestore_v1/base_query.py +++ b/google/cloud/firestore_v1/base_query.py @@ -67,10 +67,12 @@ _BAD_DIR_STRING: str -_BAD_OP_NAN_NULL: str +_BAD_OP_NAN: str +_BAD_OP_NULL: str _BAD_OP_STRING: str _COMPARISON_OPERATORS: Dict[str, Any] _EQ_OP: str +_NEQ_OP: str _INVALID_CURSOR_TRANSFORM: str _INVALID_WHERE_TRANSFORM: str _MISMATCH_CURSOR_W_ORDER_BY: str @@ -80,12 +82,13 @@ _EQ_OP = "==" +_NEQ_OP = "!=" _operator_enum = StructuredQuery.FieldFilter.Operator _COMPARISON_OPERATORS = { "<": _operator_enum.LESS_THAN, "<=": _operator_enum.LESS_THAN_OR_EQUAL, _EQ_OP: _operator_enum.EQUAL, - "!=": _operator_enum.NOT_EQUAL, + _NEQ_OP: _operator_enum.NOT_EQUAL, ">=": _operator_enum.GREATER_THAN_OR_EQUAL, ">": _operator_enum.GREATER_THAN, "array_contains": _operator_enum.ARRAY_CONTAINS, @@ -104,7 +107,8 @@ _operator_enum.NOT_IN, ) _BAD_OP_STRING = "Operator string {!r} is invalid. Valid choices are: {}." -_BAD_OP_NAN_NULL = 'Only an equality filter ("==") can be used with None or NaN values' +_BAD_OP_NAN = 'Only an equality filter ("==") can be used with NaN values' +_BAD_OP_NULL = 'Only equality ("==") or not-equal ("!=") filters can be used with None values' _INVALID_WHERE_TRANSFORM = "Transforms cannot be used as where values." _BAD_DIR_STRING = "Invalid direction {!r}. Must be one of {!r} or {!r}." _INVALID_CURSOR_TRANSFORM = "Transforms cannot be used as cursor values." @@ -144,13 +148,16 @@ def __init__(self, field_path, op_string, value=None): self.value = value if value is None: - if op_string != _EQ_OP: - raise ValueError(_BAD_OP_NAN_NULL) - self.op_string = StructuredQuery.UnaryFilter.Operator.IS_NULL + if op_string == _EQ_OP: + self.op_string = StructuredQuery.UnaryFilter.Operator.IS_NULL + elif op_string == _NEQ_OP: + self.op_string = StructuredQuery.UnaryFilter.Operator.IS_NOT_NULL + else: + raise ValueError(_BAD_OP_NULL) elif _isnan(value): if op_string != _EQ_OP: - raise ValueError(_BAD_OP_NAN_NULL) + raise ValueError(_BAD_OP_NAN) self.op_string = StructuredQuery.UnaryFilter.Operator.IS_NAN elif isinstance(value, (transforms.Sentinel, transforms._ValueList)): raise ValueError(_INVALID_WHERE_TRANSFORM) @@ -479,15 +486,20 @@ def where( stacklevel=2, ) if value is None: - if op_string != _EQ_OP: - raise ValueError(_BAD_OP_NAN_NULL) + if op_string == _EQ_OP: + op = StructuredQuery.UnaryFilter.Operator.IS_NULL + elif op_string == _NEQ_OP: + op = StructuredQuery.UnaryFilter.Operator.IS_NOT_NULL + else: + raise ValueError(_BAD_OP_NULL) + filter_pb = query.StructuredQuery.UnaryFilter( field=query.StructuredQuery.FieldReference(field_path=field_path), - op=StructuredQuery.UnaryFilter.Operator.IS_NULL, + op=op ) elif _isnan(value): if op_string != _EQ_OP: - raise ValueError(_BAD_OP_NAN_NULL) + raise ValueError(_BAD_OP_NAN) filter_pb = query.StructuredQuery.UnaryFilter( field=query.StructuredQuery.FieldReference(field_path=field_path), op=StructuredQuery.UnaryFilter.Operator.IS_NAN, diff --git a/tests/unit/v1/test_base_query.py b/tests/unit/v1/test_base_query.py index 24caa5e40c..62cd7457fa 100644 --- a/tests/unit/v1/test_base_query.py +++ b/tests/unit/v1/test_base_query.py @@ -301,6 +301,20 @@ def test_basequery_where_eq_null(unary_helper_function): unary_helper_function(None, op_enum) +@pytest.mark.parametrize( + "unary_helper_function", + [ + (_where_unary_helper), + (_where_unary_helper_field_filter), + ], +) +def test_basequery_where_neq_null(unary_helper_function): + from google.cloud.firestore_v1.types import StructuredQuery + + op_enum = StructuredQuery.UnaryFilter.Operator.IS_NOT_NULL + unary_helper_function(None, op_enum, op_string="!=") + + @pytest.mark.parametrize( "unary_helper_function", [ @@ -309,11 +323,11 @@ def test_basequery_where_eq_null(unary_helper_function): ], ) def test_basequery_where_gt_null(unary_helper_function): - from google.cloud.firestore_v1.base_query import _BAD_OP_NAN_NULL + from google.cloud.firestore_v1.base_query import _BAD_OP_NULL with pytest.raises(ValueError) as exc: unary_helper_function(None, 0, op_string=">") - assert str(exc.value) == _BAD_OP_NAN_NULL + assert str(exc.value) == _BAD_OP_NULL @pytest.mark.parametrize( @@ -338,11 +352,11 @@ def test_basequery_where_eq_nan(unary_helper_function): ], ) def test_basequery_where_le_nan(unary_helper_function): - from google.cloud.firestore_v1.base_query import _BAD_OP_NAN_NULL + from google.cloud.firestore_v1.base_query import _BAD_OP_NAN with pytest.raises(ValueError) as exc: unary_helper_function(float("nan"), 0, op_string="<=") - assert str(exc.value) == _BAD_OP_NAN_NULL + assert str(exc.value) == _BAD_OP_NAN @pytest.mark.parametrize( From 0e06a818074b3bdf07210c852278a0b89f10058d Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Thu, 19 Dec 2024 15:36:02 -0800 Subject: [PATCH 2/5] fixed lint --- google/cloud/firestore_v1/base_query.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/google/cloud/firestore_v1/base_query.py b/google/cloud/firestore_v1/base_query.py index 70460b733c..2212446bfe 100644 --- a/google/cloud/firestore_v1/base_query.py +++ b/google/cloud/firestore_v1/base_query.py @@ -108,7 +108,9 @@ ) _BAD_OP_STRING = "Operator string {!r} is invalid. Valid choices are: {}." _BAD_OP_NAN = 'Only an equality filter ("==") can be used with NaN values' -_BAD_OP_NULL = 'Only equality ("==") or not-equal ("!=") filters can be used with None values' +_BAD_OP_NULL = ( + 'Only equality ("==") or not-equal ("!=") filters can be used with None values' +) _INVALID_WHERE_TRANSFORM = "Transforms cannot be used as where values." _BAD_DIR_STRING = "Invalid direction {!r}. Must be one of {!r} or {!r}." _INVALID_CURSOR_TRANSFORM = "Transforms cannot be used as cursor values." @@ -495,7 +497,7 @@ def where( filter_pb = query.StructuredQuery.UnaryFilter( field=query.StructuredQuery.FieldReference(field_path=field_path), - op=op + op=op, ) elif _isnan(value): if op_string != _EQ_OP: From 28597aaa179e8738cf8d0fa625c06c7c61724845 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Thu, 19 Dec 2024 15:55:09 -0800 Subject: [PATCH 3/5] refactored --- google/cloud/firestore_v1/base_query.py | 70 +++++++++++++------------ 1 file changed, 36 insertions(+), 34 deletions(-) diff --git a/google/cloud/firestore_v1/base_query.py b/google/cloud/firestore_v1/base_query.py index 2212446bfe..41403c0bbf 100644 --- a/google/cloud/firestore_v1/base_query.py +++ b/google/cloud/firestore_v1/base_query.py @@ -142,29 +142,46 @@ def _to_pb(self): """Build the protobuf representation based on values in the filter""" +def _validate_opation(op_string, value): + """ + Given an input operator string (e.g, '!='), and a value (e.g. None), + ensure that the operator and value combination is valid, and return + an approproate new operator value. A new operator will be used if + the operaion is a comparison against Null or NaN + + Args: + op_string (Optional[str]): the requested operator + value (Any): the value the operator is acting on + Returns: + str | StructuredQuery.UnaryFilter.Operator: operator to use in requests + Raises: + ValueError: if the operator and value combination is invalid + """ + if value is None: + if op_string == _EQ_OP: + return StructuredQuery.UnaryFilter.Operator.IS_NULL + elif op_string == _NEQ_OP: + return StructuredQuery.UnaryFilter.Operator.IS_NOT_NULL + else: + raise ValueError(_BAD_OP_NULL) + + elif _isnan(value): + if op_string != _EQ_OP: + raise ValueError(_BAD_OP_NAN) + return StructuredQuery.UnaryFilter.Operator.IS_NAN + elif isinstance(value, (transforms.Sentinel, transforms._ValueList)): + raise ValueError(_INVALID_WHERE_TRANSFORM) + else: + return op_string + + class FieldFilter(BaseFilter): """Class representation of a Field Filter.""" def __init__(self, field_path, op_string, value=None): self.field_path = field_path self.value = value - - if value is None: - if op_string == _EQ_OP: - self.op_string = StructuredQuery.UnaryFilter.Operator.IS_NULL - elif op_string == _NEQ_OP: - self.op_string = StructuredQuery.UnaryFilter.Operator.IS_NOT_NULL - else: - raise ValueError(_BAD_OP_NULL) - - elif _isnan(value): - if op_string != _EQ_OP: - raise ValueError(_BAD_OP_NAN) - self.op_string = StructuredQuery.UnaryFilter.Operator.IS_NAN - elif isinstance(value, (transforms.Sentinel, transforms._ValueList)): - raise ValueError(_INVALID_WHERE_TRANSFORM) - else: - self.op_string = op_string + self.op_string = _validate_opation(op_string, value) def _to_pb(self): """Returns the protobuf representation, either a StructuredQuery.UnaryFilter or a StructuredQuery.FieldFilter""" @@ -487,27 +504,12 @@ def where( UserWarning, stacklevel=2, ) - if value is None: - if op_string == _EQ_OP: - op = StructuredQuery.UnaryFilter.Operator.IS_NULL - elif op_string == _NEQ_OP: - op = StructuredQuery.UnaryFilter.Operator.IS_NOT_NULL - else: - raise ValueError(_BAD_OP_NULL) - + op = _validate_opation(op_string, value) + if isinstance(op, StructuredQuery.UnaryFilter.Operator): filter_pb = query.StructuredQuery.UnaryFilter( field=query.StructuredQuery.FieldReference(field_path=field_path), op=op, ) - elif _isnan(value): - if op_string != _EQ_OP: - raise ValueError(_BAD_OP_NAN) - filter_pb = query.StructuredQuery.UnaryFilter( - field=query.StructuredQuery.FieldReference(field_path=field_path), - op=StructuredQuery.UnaryFilter.Operator.IS_NAN, - ) - elif isinstance(value, (transforms.Sentinel, transforms._ValueList)): - raise ValueError(_INVALID_WHERE_TRANSFORM) else: filter_pb = query.StructuredQuery.FieldFilter( field=query.StructuredQuery.FieldReference(field_path=field_path), From ce9ef67d36e7768e5c1b4ce5bf24ce3d8215f472 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Thu, 19 Dec 2024 16:06:07 -0800 Subject: [PATCH 4/5] added NOT_NAN --- google/cloud/firestore_v1/base_query.py | 16 ++++++++-------- tests/unit/v1/test_base_query.py | 22 ++++++++++++++++++---- 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/google/cloud/firestore_v1/base_query.py b/google/cloud/firestore_v1/base_query.py index 41403c0bbf..5a9efaf783 100644 --- a/google/cloud/firestore_v1/base_query.py +++ b/google/cloud/firestore_v1/base_query.py @@ -107,10 +107,7 @@ _operator_enum.NOT_IN, ) _BAD_OP_STRING = "Operator string {!r} is invalid. Valid choices are: {}." -_BAD_OP_NAN = 'Only an equality filter ("==") can be used with NaN values' -_BAD_OP_NULL = ( - 'Only equality ("==") or not-equal ("!=") filters can be used with None values' -) +_BAD_OP_NAN_NULL = 'Only equality ("==") or not-equal ("!=") filters can be used with None or NaN values' _INVALID_WHERE_TRANSFORM = "Transforms cannot be used as where values." _BAD_DIR_STRING = "Invalid direction {!r}. Must be one of {!r} or {!r}." _INVALID_CURSOR_TRANSFORM = "Transforms cannot be used as cursor values." @@ -163,12 +160,15 @@ def _validate_opation(op_string, value): elif op_string == _NEQ_OP: return StructuredQuery.UnaryFilter.Operator.IS_NOT_NULL else: - raise ValueError(_BAD_OP_NULL) + raise ValueError(_BAD_OP_NAN_NULL) elif _isnan(value): - if op_string != _EQ_OP: - raise ValueError(_BAD_OP_NAN) - return StructuredQuery.UnaryFilter.Operator.IS_NAN + if op_string == _EQ_OP: + return StructuredQuery.UnaryFilter.Operator.IS_NAN + elif op_string == _NEQ_OP: + return StructuredQuery.UnaryFilter.Operator.IS_NOT_NAN + else: + raise ValueError(_BAD_OP_NAN_NULL) elif isinstance(value, (transforms.Sentinel, transforms._ValueList)): raise ValueError(_INVALID_WHERE_TRANSFORM) else: diff --git a/tests/unit/v1/test_base_query.py b/tests/unit/v1/test_base_query.py index 62cd7457fa..7f6b0e5e2e 100644 --- a/tests/unit/v1/test_base_query.py +++ b/tests/unit/v1/test_base_query.py @@ -323,11 +323,11 @@ def test_basequery_where_neq_null(unary_helper_function): ], ) def test_basequery_where_gt_null(unary_helper_function): - from google.cloud.firestore_v1.base_query import _BAD_OP_NULL + from google.cloud.firestore_v1.base_query import _BAD_OP_NAN_NULL with pytest.raises(ValueError) as exc: unary_helper_function(None, 0, op_string=">") - assert str(exc.value) == _BAD_OP_NULL + assert str(exc.value) == _BAD_OP_NAN_NULL @pytest.mark.parametrize( @@ -344,6 +344,20 @@ def test_basequery_where_eq_nan(unary_helper_function): unary_helper_function(float("nan"), op_enum) +@pytest.mark.parametrize( + "unary_helper_function", + [ + (_where_unary_helper), + (_where_unary_helper_field_filter), + ], +) +def test_basequery_where_neq_nan(unary_helper_function): + from google.cloud.firestore_v1.types import StructuredQuery + + op_enum = StructuredQuery.UnaryFilter.Operator.IS_NOT_NAN + unary_helper_function(float("nan"), op_enum, op_string="!=") + + @pytest.mark.parametrize( "unary_helper_function", [ @@ -352,11 +366,11 @@ def test_basequery_where_eq_nan(unary_helper_function): ], ) def test_basequery_where_le_nan(unary_helper_function): - from google.cloud.firestore_v1.base_query import _BAD_OP_NAN + from google.cloud.firestore_v1.base_query import _BAD_OP_NAN_NULL with pytest.raises(ValueError) as exc: unary_helper_function(float("nan"), 0, op_string="<=") - assert str(exc.value) == _BAD_OP_NAN + assert str(exc.value) == _BAD_OP_NAN_NULL @pytest.mark.parametrize( From 0e205d809abc4af98e9cbe6bbbf04c17af5e290d Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Thu, 19 Dec 2024 16:38:51 -0800 Subject: [PATCH 5/5] add system tests for != null and != nan --- tests/system/test_system.py | 4 ++++ tests/system/test_system_async.py | 21 +++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/tests/system/test_system.py b/tests/system/test_system.py index ed525db576..0ab599c31f 100644 --- a/tests/system/test_system.py +++ b/tests/system/test_system.py @@ -1503,6 +1503,10 @@ def test_query_unary(client, cleanup, database): # Add to clean-up. cleanup(document1.delete) + _, document2 = collection.add({field_name: 123}) + # Add to clean-up. + cleanup(document2.delete) + # 0. Query for null. query0 = collection.where(filter=FieldFilter(field_name, "==", None)) values0 = list(query0.stream()) diff --git a/tests/system/test_system_async.py b/tests/system/test_system_async.py index 675b23a98a..200be7d8ab 100644 --- a/tests/system/test_system_async.py +++ b/tests/system/test_system_async.py @@ -1444,6 +1444,10 @@ async def test_query_unary(client, cleanup, database): # Add to clean-up. cleanup(document1.delete) + _, document2 = await collection.add({field_name: 123}) + # Add to clean-up. + cleanup(document2.delete) + # 0. Query for null. query0 = collection.where(filter=FieldFilter(field_name, "==", None)) values0 = [i async for i in query0.stream()] @@ -1462,6 +1466,23 @@ async def test_query_unary(client, cleanup, database): assert len(data1) == 1 assert math.isnan(data1[field_name]) + # 2. Query for not null + query2 = collection.where(filter=FieldFilter(field_name, "!=", None)) + values2 = [i async for i in query2.stream()] + assert len(values2) == 2 + # should fetch documents 1 (NaN) and 2 (int) + assert any(snapshot.reference._path == document1._path for snapshot in values2) + assert any(snapshot.reference._path == document2._path for snapshot in values2) + + # 3. Query for not NAN. + query3 = collection.where(filter=FieldFilter(field_name, "!=", nan_val)) + values3 = [i async for i in query3.stream()] + assert len(values3) == 1 + snapshot3 = values3[0] + assert snapshot3.reference._path == document2._path + # only document2 is not NaN + assert snapshot3.to_dict() == {field_name: 123} + @pytest.mark.parametrize("database", [None, FIRESTORE_OTHER_DB], indirect=True) async def test_collection_group_queries(client, cleanup, database):