Repository navigation
fix(router): make Route.distance_threshold inclusive - #763
Merged
Merged
Conversation
The per-route aggregation filter used `@distance < threshold`, while the VECTOR_RANGE query feeding it (and SemanticCache) treat the radius as inclusive. A reference exactly at a route's threshold was retrieved by the range query and then dropped. Use `<=` so the threshold means the same thing across extensions, document it on the field, and add a boundary test at the exact float32 distance Redis compares.
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.
Motivation
SemanticRoutertreatedRoute.distance_thresholdas a strict bound, while the range query that feeds it andSemanticCacheboth treat the same number as inclusive. The router retrieves candidates with aVECTOR_RANGEquery, which keeps every reference whose distance is at most the radius, and then applies a per-route aggregation filter of@distance < threshold. A reference exactly at its route's threshold was therefore retrieved and then discarded.SemanticCacherelies onVECTOR_RANGEalone, so a cache entry at exactly its threshold is a hit. The field's docstring said only "Distance threshold for matching the route", so neither behaviour was documented.Exact ties are rare because distances are continuous, so the effect is an inconsistency rather than a visible failure. Making the router inclusive gives the threshold one meaning across both extensions, and it matches the existing integration test, which already asserts
match.distance <= route_thresholds["greeting"].Changes
Inclusive per-route filter
_distance_threshold_filternow emits(@route_name == '...' && @distance <= {threshold}). TheRoute.distance_thresholddocstring now reads "Maximum distance for matching the route, inclusive." Thegt=0, le=2bounds are unchanged.Boundary test at the value Redis compares
test_distance_threshold_is_inclusiveroutes "hey" against the greeting route with theminaggregation. It reads the reported distance, sets the threshold to exactly that distance, and asserts the route matches. It then sets the threshold to the next double below and asserts the route does not match.The test cannot use the reported distance as is. Redis prints the distance to 12 significant digits but compares the float32 it computed, held as a double. Measured on Redis 8.2.7 with a three-dimensional FLOAT32 cosine index, the reported distance was
0.042173743248and the compared value0.04217374324798584. The rounded figure sits above the true one, so a threshold set to it matches under both<and<=, and in the opposite rounding case it matches under neither. Rounding the reported value back to float32 recovers the compared value exactly, because 12 digits are more than the 9 needed to identify a float32. The test usesminbecause anavgover two float32 distances is a double that float32 rounding would not recover.On the strict filter the test fails at the exact-threshold assertion. With
<=it passes, alongside the other 91 router tests.Notes
No other comparison in the router depends on strictness. The range query's radius is the largest threshold across routes, which
VECTOR_RANGEapplies inclusively. The reducers run before the filter, so they are unaffected.The router also has a separate ordering defect, which this PR does not change. The aggregation applies
SORTBY @distance MAX max_kbeforeFILTER, soroute()keeps only the closest route and then tests it against that route's own threshold. When the closest route fails a tight threshold,route()returns no match, even when a farther route passes its looser threshold.route_many()loses matches in the same way. That fix belongs in its own PR.Release Notes
SemanticRouternow treatsRoute.distance_thresholdas inclusive, consistent withVectorRangeQueryandSemanticCache: a reference whose distance exactly equals a route's threshold now matches that route instead of being dropped.Note
Low Risk
Small behavioral change at exact threshold boundaries; routing logic otherwise unchanged and covered by a new integration test.
Overview
SemanticRouternow treatsRoute.distance_thresholdas an inclusive upper bound, aligning withVECTOR_RANGE/SemanticCacheinstead of dropping matches whose aggregated distance equals the threshold.The per-route Redis aggregate filter in
_distance_threshold_filterswitches from@distance < thresholdto@distance <= threshold, and theRoutefield docstring states the limit is inclusive.A new integration test locks the boundary: it sets the greeting threshold to the measured distance (rounded to float32 so it matches what Redis compares) and expects a match, then expects no match with the next lower float.
Reviewed by Cursor Bugbot for commit 11c0f70. Bugbot is set up for automated code reviews on this repo. Configure here.