Repository navigation
Python: normalize comparison filters with constant on left and chained comparisons (#14779) - #14781
Conversation
GitHub Actions
Active
semantic-kernel-automation
Bot
added
the
python
Muhammad Abdullah Butt (1AbdullahButt)
left a comment
Muhammad Abdullah Butt (1AbdullahButt)
left a comment
There was a problem hiding this comment.
I'm posting the same review on #14780 and #14781, since both fix #14779 the same way and they conflict with each other in data/vector.py (git merge-tree of the two heads stops on the import block and the new class). Each one merges cleanly against main on its own, so the maintainers will want to pick one.
What both get right. Both add a NodeTransformer that runs in VectorSearch._build_filter right after _FoldUnaryNumericConstants. So -5 < x.num is folded to a Constant before the flip, and both include a test for that. Both:
- split a chain into
BoolOp(And)pair by pair, takingleftfromcomparators[i - 1] - flip
</>and<=/>=, and keep==/!= - leave
in/not inalone, as the issue asked
I traced the three filters from the issue through both, and they give the same tree: x.num > 2, x.num == 3, and x.num >= 1 and x.num < 4. A middle constant also works in both. x.a < 5 < x.b becomes x.a < 5 and x.b > 5. Every connector parser I checked (Chroma, Qdrant, Weaviate, Pinecone, MongoDB, Cosmos DB) already handles a nested BoolOp(And). So a chain combined with and still parses.
Where they differ.
#14780 _NormalizeComparisonConstants |
#14781 _NormalizeComparisonFilters |
|
|---|---|---|
| When it flips | left is a Constant and right is Attribute/Name |
left is not Attribute/Name and right is |
['a'] == x.tags, (1, 2) == x.pair |
left as is (List/Tuple aren't Constant) |
flipped |
None is x.f |
left as is (Is/IsNot aren't in the map) |
flipped to x.f is None |
| Connector tests | Chroma, Qdrant, Weaviate _build_filter |
Chroma and Qdrant _build_filter, plus an InMemoryCollection.search run |
#14781's wider rule is safe for the cases above. It only swaps operands of symmetric or mirrored operators, and the tests pin two fields (x.num < x.status) and two constants (1 < 2) as unchanged. #14780's narrower rule is easier to reason about. List/tuple literals and is on the left of a field are uncommon in filters. sql_server.py:600 already swaps a None on the left, so the extra cases in #14781 add little. Either is fine to merge on correctness. I'd decide on test coverage: #14780 covers Weaviate, which the issue names as raising NotImplementedError, and #14781 covers an end-to-end in-memory search.
Small things:
- #14781
vector.py(the chain loop):self.visit_Compare(sub_compare)re-runsgeneric_visiton children that were already visited, and its result is always anexpr, sotransformed if isinstance(transformed, expr) else sub_comparecan just beself._flip(sub_compare). A helper like #14780's_normalize_singledoes that. - #14780:
copy_location(inv_op_cls(), node.ops[0])does nothing, becausecmpopnodes have no location attributes.inv_op_cls()is enough. - In both,
OracleCollection._build_filter(oracle.py:1016) overrides the base method and parses filters without the folder or the new normalizer. Oracle builds SQL, so:b1 < numalready works there. I'm only noting it so it doesn't read as covered.
I read both diffs and the connector _lambda_parsers at the PR heads. I didn't run the tests.
Reviewed with help from Claude Code.
Motivation and Context
Fixes #14779.
In Python vector search, filters passed as lambda expressions fail across vector store connectors when a comparison has the constant/literal on the left side (e.g.
lambda x: 2 < x.numorlambda x: 3 == x.num), or uses Python's chained comparison syntax (e.g.lambda x: 1 <= x.num < 4).Connector
_lambda_parserimplementations (Qdrant, Chroma, Weaviate, Pinecone, MongoDB, Azure Cosmos DB, etc.) assume the field identifier is on the left (AttributeorName) and the value is on the right (Constant). When reversed:ValidationError(attempting to parse a string field name into a numericRangebound, or using an int askey).{2: {"$lt": "num"}}, which Chroma rejects withExpected where key to be a str, got 2.len(ops) > 1) fail because the first sub-comparison in a range chain like1 <= x.num < 4always has the lower constant bound on the left.In-memory collection and FAISS evaluate the lambda directly in Python, so they succeed while database connectors fail.
Description
_NormalizeComparisonFilters(NodeTransformer)inpython/semantic_kernel/data/vector.pyexecuted withinVectorSearch._build_filterdirectly following_FoldUnaryNumericConstants(Python: Vector store filters raise NotImplementedError for negative number literals such as x.price > -5 #14571):Compare(left, ops, comparators)wherelen(ops) > 1into pairwise comparisons combined withBoolOp(op=And(), values=[...]).Constant op FieldintoField op' Constant, flipping inequality operators (<↔>,<=↔>=) and preserving equality/identity operators (==,!=,is,is not).in,not in) and comparisons between two fields or two constants are preserved as-is.ChromaCollection,QdrantCollection, andInMemoryCollection.python/tests/unit/data/test_vector_filter_comparison_normalization.py.Contribution Checklist