Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions python/semantic_kernel/connectors/in_memory.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
VectorStore,
VectorStoreCollection,
VectorStoreCollectionDefinition,
_FoldUnaryNumericConstants,
)
from semantic_kernel.exceptions import VectorSearchExecutionException, VectorStoreModelValidationError
from semantic_kernel.exceptions.vector_store_exceptions import VectorStoreModelException, VectorStoreOperationException
Expand Down Expand Up @@ -790,6 +791,10 @@ def _parse_and_validate_filter(self, filter_str: str) -> Callable:
except SyntaxError as e:
raise VectorStoreOperationException(f"Filter string is not valid Python: {e}") from e

# Fold negative literals such as `-5` into constants, as other connectors do (#14571),
# so they pass the allowlist without allowing unary minus in general.
tree = _FoldUnaryNumericConstants().visit(tree)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_FoldUnaryNumericConstants (in semantic_kernel.data.vector) only folds UnaryOp(USub, Constant(n)). Python also parses +1 as UnaryOp(UAdd, Constant(1)), and the helper's own docstring talks about parsers that reject unary +/- wholesale. So lambda x: x.vector[0] > +1 still fails the allowlist after this visit, while -1 now works. Other connectors that already call this helper have the same gap. I'd fold UAdd of an int/float constant to Constant(+n) as well, and add a +1 / +2.5 case next to the negative-literal tests (plus a check that -x.vector[0] still raises).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking. You're right that +1 still fails here, but that matches every other connector today. The shared helper deliberately leaves unary plus alone: test_fold_unary_numeric_constants_preserves_other_unary_expressions in tests/unit/data/test_vector_negative_filter_literals.py (added with #14572) asserts that +5 and +5.5 are not folded.

This PR only brings InMemoryCollection in line with the other connectors, so I'd rather not change that shared behaviour here. If the maintainers want +n folded too, it's a small follow-up to the helper and that test, and I'm happy to open it.


# Only allow lambda expressions at the top level
if not (isinstance(tree, ast.Expression) and isinstance(tree.body, ast.Lambda)):
raise VectorStoreOperationException(
Expand Down
23 changes: 23 additions & 0 deletions python/tests/unit/connectors/memory/test_in_memory.py
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,29 @@ async def test_multiple_callable_filters_are_combined_with_and(collection):
assert list(results) == ["1"]


@mark.parametrize(
"filter_str, expected",
[
("lambda x: x.vector[0] > -1", ["1"]),
("lambda x: x.vector[0] == -2.5", ["2"]),
("lambda x: -3 < x.vector[0] < -2", ["2"]),
],
)
async def test_string_filter_with_negative_literal(collection, filter_str, expected):
# Python parses -1 as UnaryOp(USub, Constant(1)), which the allowlist rejects unless it is folded (#14571).
await collection.upsert([
{"id": "1", "vector": [1, 2, 3, 4, 5]},
{"id": "2", "vector": [-2.5, 4, 3, 2, 1]},
])
results = collection._get_filtered_records(type("opt", (), {"filter": filter_str})())
assert list(results) == expected


async def test_string_filter_still_rejects_unary_minus_on_a_field(collection):
with raises(VectorStoreOperationException, match="'USub' is not allowed"):
collection._get_filtered_records(type("opt", (), {"filter": "lambda x: -x.vector[0] > 1"})())


async def test_invalid_filter_not_lambda(collection):
with raises(VectorStoreOperationException, match="must be a lambda expression"):
collection._get_filtered_records(type("opt", (), {"filter": "x.id == '1'"})())
Expand Down
Loading