diff --git a/python/semantic_kernel/data/vector.py b/python/semantic_kernel/data/vector.py index 964e9d92bec7..151269b27cac 100644 --- a/python/semantic_kernel/data/vector.py +++ b/python/semantic_kernel/data/vector.py @@ -5,7 +5,7 @@ import operator import sys from abc import abstractmethod -from ast import AST, Lambda, NodeVisitor, expr, parse +from ast import AST, Constant, Lambda, NodeTransformer, NodeVisitor, UnaryOp, USub, copy_location, expr, parse from collections.abc import AsyncIterable, Callable, Mapping, Sequence from copy import deepcopy from dataclasses import dataclass @@ -714,6 +714,28 @@ class GetFilteredRecordOptions: order_by: Mapping[str, bool] | None = None +class _FoldUnaryNumericConstants(NodeTransformer): + """Fold negative numeric literals into constants before filter parsers run. + + Python parses negative literals as ``UnaryOp(USub, Constant(n))``. Most vector-store + ``_lambda_parser`` implementations reject unary ``+/-`` wholesale, so filters such as + ``lambda x: x.price > -5`` fail even though a negative bound is otherwise valid. + Folding the unary into a constant here keeps connector parsers unchanged (#14571). + """ + + def visit_UnaryOp(self, node: UnaryOp) -> AST: + """Fold unary minus of an int/float constant; leave other unary ops alone.""" + self.generic_visit(node) + if ( + isinstance(node.op, USub) + and isinstance(node.operand, Constant) + and isinstance(node.operand.value, (int, float)) + and not isinstance(node.operand.value, bool) + ): + return copy_location(Constant(-node.operand.value), node) + return node + + class LambdaVisitor(NodeVisitor, Generic[TFilters]): """Visitor class to visit the AST nodes.""" @@ -1987,9 +2009,12 @@ def _build_filter(self, search_filter: OptionalOneOrMany[Callable | str] | None) created_filters: list[Any] = [] visitor = LambdaVisitor(self._lambda_parser) + folder = _FoldUnaryNumericConstants() for filter_ in filters: # parse lambda expression with AST tree = parse(filter_ if isinstance(filter_, str) else getsource(filter_).strip()) + # Fold negative literals so connector parsers see a numeric Constant (#14571). + tree = folder.visit(tree) visitor.visit(tree) created_filters = visitor.output_filters if len(created_filters) == 0: diff --git a/python/tests/unit/connectors/memory/test_postgres_store.py b/python/tests/unit/connectors/memory/test_postgres_store.py index 0ddf51ef2b72..fb292b97a31f 100644 --- a/python/tests/unit/connectors/memory/test_postgres_store.py +++ b/python/tests/unit/connectors/memory/test_postgres_store.py @@ -506,6 +506,9 @@ class FilterRecord: ("x.id == 1", '"id" = %s', [1]), ("x.id != 1", '"id" <> %s', [1]), ("x.id > 1", '"id" > %s', [1]), + ("x.id > -1", '"id" > %s', [-1]), + ("x.id == -1", '"id" = %s', [-1]), + ("x.id == -1.5", '"id" = %s', [-1.5]), ("x.id >= 1", '"id" >= %s', [1]), ("x.id < 1", '"id" < %s', [1]), ("x.id <= 1", '"id" <= %s', [1]), @@ -636,7 +639,6 @@ def test_filter_identifier_quoting(filter_collection): ("1j != None", VectorStoreOperationException), ("x.id is None", NotImplementedError), ("x.id is not None", NotImplementedError), - ("x.id == -1", NotImplementedError), ("x.id == +1", NotImplementedError), ("x.id == ~1", NotImplementedError), ("x.tenant.startswith('a')", NotImplementedError), diff --git a/python/tests/unit/data/test_vector_negative_filter_literals.py b/python/tests/unit/data/test_vector_negative_filter_literals.py new file mode 100644 index 000000000000..7dcba10e25e0 --- /dev/null +++ b/python/tests/unit/data/test_vector_negative_filter_literals.py @@ -0,0 +1,86 @@ +# Copyright (c) Microsoft. All rights reserved. + +"""Regression: negative numeric literals in vector-store filter lambdas (#14571).""" + +from ast import Constant, UnaryOp, dump, parse +from unittest.mock import MagicMock + +import pytest +from chromadb.api import ClientAPI +from qdrant_client.async_qdrant_client import AsyncQdrantClient +from qdrant_client.models import FieldCondition, Range + +from semantic_kernel.connectors.chroma import ChromaCollection +from semantic_kernel.connectors.qdrant import QdrantCollection +from semantic_kernel.data.vector import ( + VectorStoreCollectionDefinition, + VectorStoreField, + _FoldUnaryNumericConstants, +) + + +@pytest.fixture +def numeric_definition() -> VectorStoreCollectionDefinition: + return VectorStoreCollectionDefinition( + fields=[ + VectorStoreField("key", name="id", type="str"), + VectorStoreField("data", name="price", type="float"), + VectorStoreField("vector", name="vector", dimensions=2, type="float"), + ] + ) + + +@pytest.mark.parametrize("literal,expected", [("-5", -5), ("-5.5", -5.5)]) +def test_fold_unary_numeric_constants_negates_literal(literal, expected): + tree = parse(f"lambda x: x.price > {literal}") + folded = _FoldUnaryNumericConstants().visit(tree) + # Body of the lambda should now compare against Constant(-5), not UnaryOp(USub, ...). + compare = folded.body[0].value.body + right = compare.comparators[0] + assert isinstance(right, Constant) + assert right.value == expected + assert not isinstance(right, UnaryOp) + + +@pytest.mark.parametrize("expression", ["-True", "-False", "+True", "+False", "+5", "+5.5", "~5", "-x.price"]) +def test_fold_unary_numeric_constants_preserves_other_unary_expressions(expression): + tree = parse(f"lambda x: x.price > {expression}") + original = dump(tree) + folded = _FoldUnaryNumericConstants().visit(tree) + assert dump(folded) == original + + +@pytest.mark.parametrize("literal,expected", [("-5", -5), ("-5.5", -5.5)]) +def test_chroma_build_filter_accepts_negative_literal(numeric_definition, literal, expected): + collection = ChromaCollection( + collection_name="temps", + record_type=dict, + definition=numeric_definition, + client=MagicMock(spec=ClientAPI), + ) + parsed = collection._build_filter(f"lambda x: x.price > {literal}") + assert parsed == {"price": {"$gt": expected}} + + +@pytest.mark.parametrize("expression", ["-True", "-False", "+True", "+False", "+5", "+5.5", "~5", "-x.price"]) +def test_chroma_build_filter_rejects_other_unary_expressions(numeric_definition, expression): + collection = ChromaCollection( + collection_name="temps", + record_type=dict, + definition=numeric_definition, + client=MagicMock(spec=ClientAPI), + ) + with pytest.raises(NotImplementedError, match="Unary"): + collection._build_filter(f"lambda x: x.price > {expression}") + + +@pytest.mark.parametrize("literal,expected", [("-5", -5), ("-5.5", -5.5)]) +def test_qdrant_build_filter_accepts_negative_literal(numeric_definition, literal, expected): + collection = QdrantCollection( + record_type=dict, + collection_name="temps", + definition=numeric_definition, + client=MagicMock(spec=AsyncQdrantClient), + ) + parsed = collection._build_filter(f"lambda x: x.price > {literal}") + assert parsed == FieldCondition(key="price", range=Range(gt=expected))