Repository navigation
Python: Normalize constant-on-left and chained comparisons in VectorSearch._build_filter - #14780
Conversation
Signed-off-by: Hardik Kaurani <hardikkaurani1@gmail.com>
…lter lambdas Signed-off-by: Hardik Kaurani <hardikkaurani1@gmail.com>
…orSearch._build_filter Signed-off-by: Hardik Kaurani <hardikkaurani1@gmail.com>
…sformation Signed-off-by: Hardik Kaurani <hardikkaurani1@gmail.com>
…s across Chroma, Qdrant, and Weaviate Signed-off-by: Hardik Kaurani <hardikkaurani1@gmail.com>
|
Hi Eduard van Valkenburg (@eavanvalkenburg) — opened this to normalize comparison operands in Currently, filter lambdas with constants on the left ( This adds |
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.
|
Thanks for the detailed comparison between #14780 and #14781, Muhammad Abdullah Butt (@1AbdullahButt)! Good catch on |
Motivation and Context
Vector store search filters in Python are defined as lambda functions (e.g.
lambda x: x.price < 5) and translated into backend filter queries via connector-specific_lambda_parserimplementations.Connector
_lambda_parsermethods (such as those in Chroma, Qdrant, Weaviate, MongoDB, etc.) assume the record property is on the left-hand side of comparison AST nodes (node.left) and the literal/constant is on the right-hand side (node.comparators[0]). When users write filters with constant-on-left comparisons (such aslambda x: 5 > x.priceorlambda x: "active" == x.status) or chained range comparisons (such aslambda x: 0 <= x.price < 5), connectors produce malformed filters or fail:{5: {"$gt": "price"}}or{0: {"$lte": "price"}}), producing invalid query payloads.FieldCondition(key=5, range=Range(gt="price"))is created, raising a PydanticValidationErrorbecauseRange.gtexpects a float/int, not a string property name._lambda_parserchecksif not isinstance(left, str): raise NotImplementedError("Only simple property filters are supported."), raisingNotImplementedError.Meanwhile,
InMemoryCollectionevaluates filter lambdas directly in Python, so these expressions succeed in in-memory tests but fail when deployed to production vector databases.Following the design pattern established in #14571 (
_FoldUnaryNumericConstants), this PR introduces a canonical AST normalization step inVectorSearch._build_filterbefore connector parsers execute.Description
_OP_INVERSIONmapping: Added mapping between relational operators and their symmetric inverses:Lt(<) <->Gt(>)LtE(<=) <->GtE(>=)Eq(==) <->Eq(==)NotEq(!=) <->NotEq(!=)_NormalizeComparisonConstants(NodeTransformer):Constanton the left and anAttributeorNameon the right are flipped into standardproperty op constantform with the inverted operator.0 <= x.price < 5) are decomposed into anast.BoolOp(op=ast.And(), values=[...])of individual normalized comparisons, matching the AST format all connectors already support for logical AND.VectorSearch._build_filter: Hooked_NormalizeComparisonConstantsinto the filter compilation pipeline immediately after_FoldUnaryNumericConstants.Contribution Checklist