From 72f26042145f01e404018ae4005cb3cee45c3cd1 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Wed, 14 Oct 2026 11:25:05 +0200 Subject: [PATCH 1/2] fix(router): apply per-route thresholds before max_k truncation 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. --- redisvl/extensions/router/semantic.py | 12 ++++----- tests/integration/test_semantic_router.py | 30 +++++++++++++++++++++++ 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/redisvl/extensions/router/semantic.py b/redisvl/extensions/router/semantic.py index f2befc31..55fa6887 100644 --- a/redisvl/extensions/router/semantic.py +++ b/redisvl/extensions/router/semantic.py @@ -437,22 +437,20 @@ def _build_aggregate_request( aggregation_func = reducers.avg aggregate_query = str(vector_range_query).split(" RETURN")[0] - aggregate_request = ( + # redis-py emits pipeline steps in call order, so FILTER must come before + # SORTBY ... MAX: otherwise the max_k closest routes are kept first and + # any that miss their own threshold leave the result short. + return ( AggregateRequest(aggregate_query) .group_by( "@route_name", # type: ignore aggregation_func("vector_distance").alias("distance"), ) + .filter(self._distance_threshold_filter()) .sort_by("@distance", max=max_k) # type: ignore .dialect(2) ) - filter = self._distance_threshold_filter() - - aggregate_request.filter(filter) - - return aggregate_request - def _get_route_matches( self, vector: list[float], diff --git a/tests/integration/test_semantic_router.py b/tests/integration/test_semantic_router.py index d6d9ba74..e3727056 100644 --- a/tests/integration/test_semantic_router.py +++ b/tests/integration/test_semantic_router.py @@ -616,6 +616,36 @@ def test_routes_different_distance_thresholds_get_one( router.delete() +def test_threshold_filter_applies_before_max_k_truncation( + client, routes, hf_vectorizer, redis_test_name +): + """A closer route that misses its own threshold must not crowd out a + farther one that meets its threshold.""" + skip_if_no_redis_search(client) + # "hello" sits ~0.07 from greeting and ~0.60 from farewell (avg distance). + routes[0].distance_threshold = 0.01 + routes[1].distance_threshold = 0.8 + + router = SemanticRouter( + name=redis_test_name("test_threshold_filter_before_max_k"), + routes=routes, + routing_config=RoutingConfig(max_k=1), + redis_client=client, + overwrite=True, + vectorizer=hf_vectorizer, + ) + try: + assert router("hello").name == "farewell" + + matches = router.route_many("hello", max_k=1) + assert [match.name for match in matches] == ["farewell"] + finally: + with suppress(Exception): + router.clear() + with suppress(Exception): + router.delete() + + def test_add_delete_route_references(semantic_router): skip_if_redis_version_below(semantic_router._index.client, "7.0.0") From a433882de95efb25e7bb95d2146c09d3869b02fe Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Wed, 14 Oct 2026 11:31:18 +0200 Subject: [PATCH 2/2] test(router): pin avg aggregation in the threshold-before-max_k test 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. --- redisvl/extensions/router/semantic.py | 6 +++--- tests/integration/test_semantic_router.py | 11 +++++++---- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/redisvl/extensions/router/semantic.py b/redisvl/extensions/router/semantic.py index 55fa6887..7ccba363 100644 --- a/redisvl/extensions/router/semantic.py +++ b/redisvl/extensions/router/semantic.py @@ -437,9 +437,9 @@ def _build_aggregate_request( aggregation_func = reducers.avg aggregate_query = str(vector_range_query).split(" RETURN")[0] - # redis-py emits pipeline steps in call order, so FILTER must come before - # SORTBY ... MAX: otherwise the max_k closest routes are kept first and - # any that miss their own threshold leave the result short. + # FT.AGGREGATE runs its steps in the order given, and redis-py emits them + # in call order. FILTER must precede SORTBY ... MAX, or the max_k closest + # routes are kept before their thresholds are checked. return ( AggregateRequest(aggregate_query) .group_by( diff --git a/tests/integration/test_semantic_router.py b/tests/integration/test_semantic_router.py index e3727056..b1e20edd 100644 --- a/tests/integration/test_semantic_router.py +++ b/tests/integration/test_semantic_router.py @@ -622,14 +622,17 @@ def test_threshold_filter_applies_before_max_k_truncation( """A closer route that misses its own threshold must not crowd out a farther one that meets its threshold.""" skip_if_no_redis_search(client) - # "hello" sits ~0.07 from greeting and ~0.60 from farewell (avg distance). + # With all-mpnet-base-v2 and avg aggregation, "hello" sits ~0.07 from + # greeting and ~0.60 from farewell. (Under min, greeting would be ~0.) routes[0].distance_threshold = 0.01 routes[1].distance_threshold = 0.8 router = SemanticRouter( name=redis_test_name("test_threshold_filter_before_max_k"), routes=routes, - routing_config=RoutingConfig(max_k=1), + routing_config=RoutingConfig( + max_k=1, aggregation_method=DistanceAggregationMethod.avg + ), redis_client=client, overwrite=True, vectorizer=hf_vectorizer, @@ -637,8 +640,8 @@ def test_threshold_filter_applies_before_max_k_truncation( try: assert router("hello").name == "farewell" - matches = router.route_many("hello", max_k=1) - assert [match.name for match in matches] == ["farewell"] + # max_k comes from the routing config; route() always uses max_k=1. + assert [match.name for match in router.route_many("hello")] == ["farewell"] finally: with suppress(Exception): router.clear()