Skip to content

Python: Normalize constant-on-left and chained comparisons in VectorSearch._build_filter - #14780

Open
Hardik K (hardikkaurani) wants to merge 5 commits into
microsoft:mainfrom
hardikkaurani:fix/vector-filter-normalize-comparison-constants
Open

Hardik K (hardikkaurani) wants to merge 5 commits into
microsoft:mainfrom
hardikkaurani:fix/vector-filter-normalize-comparison-constants

Conversation

@hardikkaurani

Copy link
Copy Markdown
Contributor

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_parser implementations.

Connector _lambda_parser methods (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 as lambda x: 5 > x.price or lambda x: "active" == x.status) or chained range comparisons (such as lambda x: 0 <= x.price < 5), connectors produce malformed filters or fail:

  1. Chroma & MongoDB: The constant becomes the dictionary key (e.g., {5: {"$gt": "price"}} or {0: {"$lte": "price"}}), producing invalid query payloads.
  2. Qdrant: FieldCondition(key=5, range=Range(gt="price")) is created, raising a Pydantic ValidationError because Range.gt expects a float/int, not a string property name.
  3. Weaviate: _lambda_parser checks if not isinstance(left, str): raise NotImplementedError("Only simple property filters are supported."), raising NotImplementedError.

Meanwhile, InMemoryCollection evaluates 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 in VectorSearch._build_filter before connector parsers execute.

Description

  1. _OP_INVERSION mapping: Added mapping between relational operators and their symmetric inverses:
    • Lt (<) <-> Gt (>)
    • LtE (<=) <-> GtE (>=)
    • Eq (==) <-> Eq (==)
    • NotEq (!=) <-> NotEq (!=)
  2. _NormalizeComparisonConstants(NodeTransformer):
    • Single comparisons with a Constant on the left and an Attribute or Name on the right are flipped into standard property op constant form with the inverted operator.
    • Chained comparisons (e.g. 0 <= x.price < 5) are decomposed into an ast.BoolOp(op=ast.And(), values=[...]) of individual normalized comparisons, matching the AST format all connectors already support for logical AND.
  3. VectorSearch._build_filter: Hooked _NormalizeComparisonConstants into the filter compilation pipeline immediately after _FoldUnaryNumericConstants.
  4. Tests: Added unit tests covering AST transformations (single flips, bare names, folded negative bounds, preservation of constants-on-right and membership ops, chained decomposition) and connector integration tests across Chroma, Qdrant, and Weaviate.

Contribution Checklist

@hardikkaurani
Hardik K (hardikkaurani) requested a review from a team as a code owner November 11, 2026 06:02
Copilot AI balanced review requested due to automatic review settings November 11, 2026 06:02

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

@hardikkaurani

Copy link
Copy Markdown
Contributor Author

Hi Eduard van Valkenburg (@eavanvalkenburg) — opened this to normalize comparison operands in VectorSearch._build_filter.

Currently, filter lambdas with constants on the left (5 > x.price, "active" == x.status) or chained range expressions (0 <= x.price < 5) cause connector _lambda_parser implementations to emit malformed queries (e.g., Chroma/MongoDB treating the literal as the field key), fail schema validation (Qdrant), or raise NotImplementedError (Weaviate), even though InMemoryCollection accepts them.

This adds _NormalizeComparisonConstants right alongside _FoldUnaryNumericConstants (#14571) to flip single comparisons with constants on the left into canonical field op value form and decompose chained comparisons into an And BoolOp. Unit tests and connector integration coverage are included. Whenever you have a chance to take a look, any feedback would be appreciated!

@semantic-kernel-automation semantic-kernel-automation Bot added the python Pull requests for the Python Semantic Kernel label Nov 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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, taking left from comparators[i - 1]
  • flip </> and <=/>=, and keep ==/!=
  • leave in/not in alone, 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-runs generic_visit on children that were already visited, and its result is always an expr, so transformed if isinstance(transformed, expr) else sub_compare can just be self._flip(sub_compare). A helper like #14780's _normalize_single does that.
  • #14780: copy_location(inv_op_cls(), node.ops[0]) does nothing, because cmpop nodes 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 < num already 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.

@hardikkaurani

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed comparison between #14780 and #14781, Muhammad Abdullah Butt (@1AbdullahButt)!

Good catch on copy_location(inv_op_cls(), node.ops[0]) — since cmpop nodes don't carry source locations, instantiating inv_op_cls() directly is cleaner, and I'm happy to simplify that (or add anything else needed) if maintainers prefer going with #14780's narrower rule and Weaviate/Chroma/Qdrant coverage. Also appreciate calling out OracleCollection._build_filter handling SQL expressions directly.

This branch was successfully deployed

1 active deployment
github-app-auth — f236bfff Deployed Nov 11, 2026 by hardikkaurani via add_label #29666
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python Pull requests for the Python Semantic Kernel

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants