Repository navigation
fix(router): apply per-route thresholds before max_k truncation - #766
Open
vishal-bala wants to merge 2 commits into
Open
vishal-bala wants to merge 2 commits into
vishal-bala wants to merge 2 commits into
Conversation
The FT.AGGREGATE plan ran SORTBY @distance MAX max_k before FILTER, so a closer route that missed its own distance_threshold could crowd out a farther route that met its threshold. route() then returned an empty RouteMatch and route_many() returned fewer matches than it should. Call .filter() before .sort_by() so the plan is GROUPBY -> REDUCE -> FILTER -> SORTBY MAX.
Address review feedback: the test's premise relies on avg aggregation (under min, greeting would sit at ~0 and pass its threshold), so pin it rather than depend on the default. Exercise max_k via the routing config in route_many(), and say in the builder comment that FT.AGGREGATE itself runs steps in order.
vishal-bala
marked this pull request as ready for review
October 15, 2026 08:39
This branch has not been deployed
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.
Summary
SemanticRouter._build_aggregate_requestbuilt its FT.AGGREGATE as GROUPBY → REDUCE → SORTBY @distance MAX max_k → FILTER. FT.AGGREGATE runs its steps in the order given, and redis-py emits them in call order, so the router cut the result down to the max_k closest routes before it checked each route's owndistance_threshold. When a close route missed its threshold, it still took a slot, and a farther route that met its threshold never reached the FILTER.route()(and calling the router directly) always aggregates with max_k=1, whateverRoutingConfig.max_ksays. So the most common call was always exposed: whenever the closest route missed its own threshold,route()returned an emptyRouteMatch, even if another route qualified.route_many()could likewise return fewer matches than it should.For example, with the test suite's
all-mpnet-base-v2vectorizer, avg aggregation and max_k=1, the query "hello" sits about 0.07 from greeting (threshold 0.01) and about 0.60 from farewell (threshold 0.8). The old plan kept only greeting, the FILTER then dropped it, and nothing came back.The fix calls
.filter()before.sort_by().build_args()now showsGROUPBY … REDUCE … FILTER … SORTBY 2 @distance ASC MAX 1. RedisVL has no async router, so nothing else needed changing.Tests
test_threshold_filter_applies_before_max_k_truncationcovers bothroute()androute_many(), withmax_ktaken from the routing config. It fails onmain(assert None == 'farewell') and passes with the fix.tests/integration/test_semantic_router.pyandtests/unit/test_semantic_router_queries.pypass locally against Redis 8.2 (43 passed, 1 skipped).Release Notes
Fixed
SemanticRouter.route()androute_many()sometimes missing a route that met itsdistance_threshold. Each route's threshold is now applied before results are truncated tomax_k, so a closer route that fails its own threshold no longer displaces a farther one that passes. Previouslyroute()could return an emptyRouteMatchandroute_many()could return fewer matches than expected.Note
Medium Risk
Changes Redis aggregate step order and can alter which route
route()/route_many()return when the nearest route fails its threshold.Overview
SemanticRouter was building
FT.AGGREGATEwithSORTBY … MAX max_kbefore the per-routedistance_thresholdFILTER, so the closest routes were truncated first and a nearer route that failed its threshold could block a farther route that passed—often yielding an emptyroute()result or too fewroute_many()matches._build_aggregate_requestnow chains.filter()before.sort_by(), so thresholds are applied beforemax_klimits results. An integration test covers the case where greeting is closer but below threshold and farewell is selected for"hello".Reviewed by Cursor Bugbot for commit a433882. Bugbot is set up for automated code reviews on this repo. Configure here.