feat(state): give recall a relevance score, kind filter, time window and floor - #439
Merged
Conversation
…and floor
RecallQuery was {Query, Scope, Limit}, which cannot express the one thing that
decides what a per-turn recall costs in context: the most relevant K items above
a relevance floor. Limit caps how many rows come back but says nothing about how
good they are, so the strongest match is truncated away by a row that merely
arrived later.
- MemoryItem.Score grades the match, in [0,1]. It is a read-side annotation:
Recall sets it, Write clears it (in the shared Stamper, so every backend that
stamps through it is covered), and no backend persists it. A store that cannot
rank reports 1 rather than 0 - "it matched, no opinion how well" - so a floor
never silently empties a recall against a backend with no ranking to offer.
- RecallQuery gains Kinds, the half-open [Since, Until) window, MinScore, and
Order. All are opt-in: the zero query behaves exactly as before.
- OrderRelevance ranks by score, recency breaking ties, and degrades to plain
recency against a backend where every match scores 1 - so the ordering rule
stays identical across backends even though their scores differ.
SQLite surfaces bm25 instead of discarding it. FTS5 computed the rank for the
MATCH already and the contract threw it away, ordering by created_at. bm25 is
<= 0 with a more negative value meaning a better match, so -b/(1-b) maps it onto
[0,1) increasing in match quality.
The kind filter pushes into SQL, where it cuts rows before the sort. The time
window deliberately does not: created_at is stored as RFC3339Nano, which strips
trailing zeros from the fractional second, so the column is not fixed-width and
does not compare lexicographically - "...T00:00:00.000000001Z" sorts before
"...T00:00:00Z". Comparing parsed times is the only correct answer, and a query
that filters after the rows come back must also cap there, or a SQL LIMIT
truncates rows the Go stage was going to drop anyway and returns short.
memorytest gains Selectors and Relevance cases. They assert the contract's rules
rather than any backend's numbers - scores are non-increasing under
OrderRelevance, the window partitions at its boundary, Score never persists -
so they hold whether or not the backend has real ranking. The window case
separates its writes past the coarsest platform clock granularity and asserts
the stamps are strictly increasing, so it cannot quietly degenerate into
"everything landed on one side" on a fast runner.
Signed-off-by: Ion Alpha <contact@ionalpha.io>
SortRecall is where the ordering contract lives and every backend sorts through it, but no test could separate two items by score: against both bundled backends every match scores 1, and SQLite orders in SQL rather than here. So the key precedence - relevance, then scope, then recency, then ID - went unexercised. state/recall_order_test.go drives SortRecall directly with items that differ on each axis at once, so which key wins is asserted rather than assumed. The conformance suite's new cases move onto the helpers introduced with the scope fix, and the two older cases follow, so the whole suite now reads as one assertion per line and the failure branches are not duplicated at every check. Two helpers join them: wantSet for questions about which items came back (so a set answer cannot accidentally pin an ordering the contract leaves open) and wantIncreasingStamps, which fails outright if two writes land on one clock tick instead of letting the window assertions quietly weaken. One real gap closed while measuring: a limit alongside a time window was never asserted. That is the case a backend gets wrong by pushing the limit into its query language while applying the window afterwards - it caps first and returns short. The suite now pins it. Signed-off-by: Ion Alpha <contact@ionalpha.io>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
What
RecallQuerygainsKinds, a half-open[Since, Until)window,MinScore, andOrder;MemoryItemgainsScore. SQLite surfaces bm25 instead of discarding it. All of it is opt-in, so the zero query behaves exactly as before.Stacked on the scope-resolution branch; review that one first.
Why
The recall contract was
{Query, Scope, Limit}, which cannot express the one thing that decides what a per-turn recall costs in context: the most relevant K items above a relevance floor.Limitcaps how many rows come back and says nothing about how good they are, so the strongest match gets truncated away by a row that merely arrived later.Three consequences, all closed here:
storage/sqlitejoinsmemory_ftson aMATCH, so FTS5 computes bm25 for that join whether we want it or not, and then it ordered bycreated_at. With no score there is no floor, so the only lever on context cost was row count.Scoreis a read-side annotation, not part of the record:Recallsets it,Writeclears it, no backend persists it. Clearing happens in the sharedstate.Stamper, so every backend that stamps through it is covered rather than each remembering. A backend that cannot rank reports 1, not 0 ("it matched, no opinion how well"), so a floor excludes nothing on a store with no ranking to offer instead of silently emptying every recall against it.OrderRelevancetherefore degrades to plain recency on those backends: the ordering rule stays identical across backends even though their scores do not.How to verify
memorytestgainsSelectorsandRelevance. They assert the contract's rules rather than any backend's numbers (scores are non-increasing underOrderRelevance, the window partitions cleanly at its boundary,Scorenever survives a write), so they hold whether or not the backend ranks.state/recall_order_test.godrivesSortRecalldirectly with items differing on every axis at once, pinning the key precedence: relevance, then scope, then recency, then ID. No conformance run can reach this, because against both bundled backends every match scores 1 and SQLite orders in SQL.Notes for reviewers
created_atis stored as RFC3339Nano, which strips trailing zeros from the fractional second, so the column is not fixed-width and does not compare lexicographically:...T00:00:00.000000001Zsorts before...T00:00:00Z, though it is later. Comparing parsed times is the only correct answer available. The kind filter does push down, being an exact match on an indexed column that cuts rows before the sort.LIMITtruncates rows the Go stage was going to drop anyway and the recall comes back short. The suite pins this case.created_atmeans SQLite'sORDER BY m.created_at DESCis wrong whenever two rows differ in stored precision, which is roughly one timestamp in ten (any reading that lands on a zero nanosecond). Fixing it means a fixed-width encoding plus a migration rewriting every timestamp column, which is its own change with its own risk; tracked separately.MemoryItem.Scoresays so, because a caller tuningMinScoreis tuning it for one backend and one corpus, not setting a portable constant.