fix(sqlserver): preserve MMR ranking order in results - #813
Open
DevNinja (0xDevNinja) wants to merge 3 commits into
Open
fix(sqlserver): preserve MMR ranking order in results#813DevNinja (0xDevNinja) wants to merge 3 commits into
DevNinja (0xDevNinja) wants to merge 3 commits into
Conversation
max_marginal_relevance_search_by_vector returned documents in the store's original fetch order instead of the order chosen by the MMR algorithm. It filtered results_as_docs by `idx in mmr_selects`, which discards the ranking that maximal_marginal_relevance returns. Index into results_as_docs by the selected indices so the diversity-optimized order is preserved. Add a unit test that asserts the MMR ordering without needing a live DB.
Copilot started reviewing on behalf of
Folashade Daniel (beccadaniel)
July 21, 2026 17:31
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes SQLServerVectorStore.max_marginal_relevance_search_by_vector to preserve the document ranking produced by the MMR algorithm, instead of returning results in the store fetch order.
Changes:
- Build the MMR results by indexing
results_as_docsusingmmr_selectsto preserve ranking. - Add a unit test that stubs
_search_storeto validate MMR ordering without a live database.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| libs/sqlserver/langchain_sqlserver/vectorstores.py | Adjusts MMR result construction to return documents in MMR-selected order. |
| libs/sqlserver/tests/unit_tests/test_mmr.py | Adds a unit test asserting the returned document order matches MMR ranking. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # into the original fetch order. | ||
| return [ | ||
| value for idx, value in enumerate(results_as_docs) if idx in mmr_selects | ||
| results_as_docs[idx] for idx in mmr_selects if idx < len(results_as_docs) |
Comment on lines
+42
to
+44
| docs = store.max_marginal_relevance_search_by_vector( | ||
| [1.0, 0.0], k=2, fetch_k=3, lambda_mult=0.5 | ||
| ) |
Address review feedback: - The bounds check only rejected indices past the end of the result list, so a negative index would have silently wrapped around and indexed from the end. `maximal_marginal_relevance` can return -1 when it cannot select a next candidate, so require `0 <= idx` as well. - The ordering test used `lambda_mult=0.5`, where the relevance and redundancy terms cancel for both remaining candidates and the second pick depends on tie-breaking rather than the ranking. Use 0.7 so the expected order is determined by the MMR score itself.
Contributor
Author
|
Both good catches, thanks — fixed in a5af8a9.
Also merged current |
Facundo Santiago (santiagxf)
requested a review
from Folashade Daniel (beccadaniel)
July 24, 2026 14:28
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
max_marginal_relevance_search_by_vectorreturns documents in the store's original fetch order rather than the order chosen by the MMR algorithm.maximal_marginal_relevancereturns a list of indices in ranked (relevance/diversity) order, but the result was built as:Iterating
results_as_docsand filtering by membership re-sorts the selected docs back into ascending fetch order, discarding the MMR ranking.Example: query
[1, 0]over docs A[0.9, 0.1], B[0.0, 1.0], C[1.0, 0.0]withk=2. MMR selects[2, 0]→ should return[C, A], but the old code returned[A, C].Fix
Index into
results_as_docsby the selected indices so the ranking is preserved:Tests
Added
tests/unit_tests/test_mmr.py, which stubs_search_storeand asserts the MMR ordering ([C, A]). Runs without a live database. Ruff + mypy clean; unit suite passes.