Repository navigation
Python: Fold negative literals in InMemoryCollection string filters - #14776
Harshil Kaneria (Harshil26202) wants to merge 1 commit into
Conversation
InMemoryCollection parses string filters itself and checks them against an AST allowlist that doesn't include USub, so a filter such as 'lambda x: x.price > -5' was rejected. The fix for microsoft#14571 folds negative literals in _build_filter, which InMemoryCollection doesn't use. Apply the same _FoldUnaryNumericConstants transform before validation. Unary minus on anything other than a numeric literal is still rejected.
GitHub Actions
Active
semantic-kernel-automation
Bot
added
the
python
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are reported for the change adding negative numeric literal support to InMemoryCollection filters.
No open issues 🎉
What changed in this PR
This PR extends the negative-literal filter fix from #14572 to InMemoryCollection, which validates string filters through a separate parsing path.
Changes:
- Reuses
_FoldUnaryNumericConstantsbefore AST allowlist validation to support negative numeric literals. - Adds tests for integer bounds, float equality, and chained comparisons, plus a regression test confirming unary minus on fields remains rejected.
| File | Description |
|---|---|
python/tests/unit/connectors/memory/test_in_memory.py |
Tests negative-literal filters and continued rejection of field negation. |
python/semantic_kernel/connectors/in_memory.py |
Folds negative numeric literals before validating string filters. |
Review effort: Balanced
Mangesh Raut (mangeshraut712)
left a comment
Mangesh Raut (mangeshraut712)
left a comment
There was a problem hiding this comment.
The InMemory path needed this fold, and the tests correctly keep unary minus on a field rejected. One gap in the shared helper: it only folds USub, not UAdd.
|
|
||
| # 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) |
There was a problem hiding this comment.
_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).
There was a problem hiding this comment.
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.
|
@microsoft-github-policy-service agree |
Motivation and Context
#14571 reported that filters with a negative number literal, such as
lambda x: x.price > -5, raisedNotImplementedErrorin most vector store connectors. #14572 fixed it by foldingUnaryOp(USub, Constant(n))intoConstant(-n)inVectorSearch._build_filter.InMemoryCollectiondoesn't go through_build_filter. It parses string filters in_parse_and_validate_filterand checks every node againstallowed_filter_ast_nodes, which doesn't includeUSub. So the same filter still fails there when it is passed as a string, which is also the form the generated search functions pass:Description
_parse_and_validate_filternow runs the existing_FoldUnaryNumericConstantstransform on the parsed tree before the allowlist check. Only a unary minus applied directly to an int or float literal is folded, so the sandbox is unchanged:USubstays off the allowlist, and something likelambda x: -x.price > 1is still rejected.Tests in
tests/unit/connectors/memory/test_in_memory.py:test_string_filter_with_negative_literalcovers an int bound, a float equality and a chained comparison. All three fail onmainand pass with this change.test_string_filter_still_rejects_unary_minus_on_a_fieldchecks that unary minus on a field is still blocked.Locally,
tests/unit/dataplus the in-memory, Qdrant, Qdrant local, Chroma and FAISS connector tests pass (253 passed).ruff check,ruff format --checkandmypyare clean on the changed files.AI disclosure: I found this with Claude Code while comparing filter results across the Python vector store connectors. It also drafted the fix and the tests.
Contribution Checklist