mirror of
https://github.com/karatektus/pl_mail.git
synced 2026-10-25 01:54:10 +00:00
fix(search): let the words win before the vector suggests
Searching one company name returned 63 results where Gmail, over substantially the same mail, returned 16. Sixteen were matches on words. The other forty-seven had merely cleared the vector's similarity threshold — job alerts, a product newsletter, an invoice, outright spam — every one correctly badged "meaning", every one useless, and together enough to fill the first page exactly. The best match in the mailbox, a reply whose subject AND sender both carried the term, was on page two. **The threshold was not mistuned.** It was measured against the twelve-message corpus in EmbeddingPreset, where a false-positive rate of a couple of percent is invisible, and then applied to the two thousand most recent messages, where the same rate is forty-seven rows. No stricter fraction fixes that — the fix for "a small fraction of two thousand is still a lot" is a cap. So `sem_hits` takes the ten nearest and stops. The cap is INSIDE that CTE, which is what makes it safe: the note above it argued a top-k would make `COUNT(*) OVER ()` mean "lexical plus up to k, on page one only", and that is true of a top-k over the UNION, which sees OFFSET. A CTE is evaluated once per statement and the candidate window is deterministic, so the admitted set is identical on every page and the total stays honest. The earlier reasoning was right about the wrong position. Ordering carried the other half, and it is why the best match was displaced rather than merely outnumbered. A semantic hit can only be drawn from the most recent SEMANTIC_CANDIDATES messages, so under "most recent" every one of them is structurally newer than most of the mail it competes with — they do not tend to win, they cannot lose. `semantic_only ASC` now leads both sort orders. That fixes relevance too, where the disease was worse: `rank` is `GREATEST(ts_rank, cosine)`, and measured against this schema's weights a subject match scores 0.6079, a body match 0.1216, a token-part match 0.0608, while any admitted vector hit is ≥ 0.42 by definition. Sorting by relevance put every barely-admissible vector hit above every body match. Leading with provenance means rank is only ever compared within one scale. Nothing is removed. The meaning matches are all still there, badged, at the end. The body-substring rescue is counted over lexical rows now rather than over the page. It exists to catch a needle inside a token the tokenizer split the wrong way, and forty-seven vector hits padding a page to exactly fifty made `50 < 50` false — so the one arm that could still have found the mail never ran, and nothing said so. Both new tests were checked against the old behaviour and fail there: without the cap the fixture admits two rows instead of one, and without the ordering the newer meaning match takes first place. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
948213265c
commit
a1af660b45
3 files changed
+203
-10
No files matched your search
@@ -36,12 +36,36 @@ enum SearchSortOrder: string
|
||||
* differently the second time. Relevance ties are not an edge case either:
|
||||
* `ts_rank` is degenerate for a query that stems to nothing, so every row
|
||||
* scores the same and the entire ordering rests on the tiebreaker.
|
||||
*
|
||||
* ## Words before vectors, whichever order is chosen
|
||||
*
|
||||
* `semantic_only ASC` leads both, and it is the fix for a search that put
|
||||
* the best match in the mailbox on page two. A semantic hit can only be
|
||||
* drawn from the most recent SEMANTIC_CANDIDATES messages, so under
|
||||
* `Recent` every one of them is structurally newer than most of the mail it
|
||||
* is competing with — they do not merely tend to win, they are guaranteed
|
||||
* to. Forty-seven of them once sat above a reply whose subject AND sender
|
||||
* both contained the search term, purely because it was six weeks older.
|
||||
*
|
||||
* It fixes `Relevance` as well, and there the disease was worse. `rank` is
|
||||
* `GREATEST(ts_rank, cosine_similarity)`, which mixes two scales that have
|
||||
* no common unit: measured against this schema's weights, a subject match
|
||||
* scores 0.6079, a body match 0.1216, a token-part match 0.0608 — while any
|
||||
* admitted semantic hit is ≥ 0.42 by definition. So sorting by relevance
|
||||
* put every barely-admissible vector hit above every body match, and any
|
||||
* vector hit over 0.608 above an exact subject match. Leading with
|
||||
* provenance means `rank` is now only ever compared within one scale:
|
||||
* ts_rank against ts_rank among the rows words found, similarity against
|
||||
* similarity among the rows only the vector did. The two never meet.
|
||||
*
|
||||
* This is a presentation rule and not a filter. Nothing is removed — the
|
||||
* meaning matches are all still there, badged, in a block at the end.
|
||||
*/
|
||||
public function orderBy(): string
|
||||
{
|
||||
return match ($this) {
|
||||
self::Recent => 'last_message_at DESC, thread_id DESC',
|
||||
self::Relevance => 'rank DESC, last_message_at DESC, thread_id DESC',
|
||||
self::Recent => 'semantic_only ASC, last_message_at DESC, thread_id DESC',
|
||||
self::Relevance => 'semantic_only ASC, rank DESC, last_message_at DESC, thread_id DESC',
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@@ -57,6 +57,13 @@ class MessageThreadRepository extends ServiceEntityRepository
|
||||
* three-row fixture. See buildSearchSql().
|
||||
*/
|
||||
private readonly int $semanticCandidates = self::SEMANTIC_CANDIDATES,
|
||||
/**
|
||||
* And the fourth, for the reason the other three are injectable: a
|
||||
* bound nothing exercises is a bound nobody knows the shape of. A test
|
||||
* that had to seed eleven near-neighbours to see this one work would
|
||||
* not be written; at 1 it is a two-row fixture.
|
||||
*/
|
||||
private readonly int $semanticHits = self::SEMANTIC_HITS,
|
||||
/**
|
||||
* The first logger this repository has ever had, and it is here for
|
||||
* one call site: the vector arm's give-up path, which discarded the
|
||||
@@ -1290,7 +1297,16 @@ class MessageThreadRepository extends ServiceEntityRepository
|
||||
// Thin, so the expensive pass earns its keep — see the rescue note in
|
||||
// buildSearchSql(). Only ever reached on a page that did not fill,
|
||||
// which is the same condition the pass exists to serve.
|
||||
if (count($rows) < self::SEARCH_RESCUE_BELOW) {
|
||||
//
|
||||
// COUNTED OVER THE ROWS THE WORDS FOUND, not over the page. The body
|
||||
// pass exists to catch a needle inside a token that the tokenizer split
|
||||
// the wrong way — a LEXICAL rescue — and it was being called off by
|
||||
// rows that are not lexical at all. Forty-seven vector hits padding a
|
||||
// page to exactly fifty made `50 < 50` false, so the one arm that could
|
||||
// still have found the mail never ran, and nothing said so. The cap on
|
||||
// sem_hits makes that arithmetic much harder to hit; counting the right
|
||||
// thing makes it impossible.
|
||||
if ($this->lexicalCount($rows) < self::SEARCH_RESCUE_BELOW) {
|
||||
$rows = $this->rescueRows(
|
||||
fn (): array => $this->searchRows($user, $query, $sort, $perPage, $offset, true, $semantic),
|
||||
$rows,
|
||||
@@ -1422,6 +1438,28 @@ class MessageThreadRepository extends ServiceEntityRepository
|
||||
return $this->getEntityManager()->getConnection()->fetchAllAssociative($sql, $params, $types);
|
||||
}
|
||||
|
||||
/**
|
||||
* How many of these rows some arm made of words actually found.
|
||||
*
|
||||
* The distinction the body rescue needs and never had. `semantic_only` is
|
||||
* on every row already — zero where no vector ran at all — so this is a
|
||||
* count over data in hand rather than a second question to the database.
|
||||
*
|
||||
* @param list<array<string,mixed>> $rows
|
||||
*/
|
||||
private function lexicalCount(array $rows): int
|
||||
{
|
||||
$found = 0;
|
||||
|
||||
foreach ($rows as $row) {
|
||||
if (1 !== (int) ($row['semantic_only'] ?? 0)) {
|
||||
++$found;
|
||||
}
|
||||
}
|
||||
|
||||
return $found;
|
||||
}
|
||||
|
||||
/**
|
||||
* Below this many cheap hits, the body substring pass is worth its 3
|
||||
* seconds; at or above it, it is not.
|
||||
@@ -1466,6 +1504,44 @@ class MessageThreadRepository extends ServiceEntityRepository
|
||||
*/
|
||||
private const int SEMANTIC_CANDIDATES = 2000;
|
||||
|
||||
/**
|
||||
* How many of those candidates may actually reach the results.
|
||||
*
|
||||
* **A threshold alone does not bound a result set, and this is what that
|
||||
* costs.** Searching one company name returned 63 rows where Gmail over the
|
||||
* same mail returned 16: sixteen matches on words, and forty-seven rows the
|
||||
* vector liked enough to clear 0.42 — LinkedIn job alerts, a newsletter, an
|
||||
* invoice, a piece of outright spam. Every one correctly badged "meaning",
|
||||
* every one useless, and together they filled the page so completely that
|
||||
* the best match in the mailbox — the company's own reply, matching on
|
||||
* subject AND sender — was on page two.
|
||||
*
|
||||
* The threshold was not mistuned. It was measured on the twelve-message
|
||||
* corpus in {@see \App\Domain\Ai\EmbeddingPreset}, where a false-positive
|
||||
* rate of a couple of percent is invisible. Against SEMANTIC_CANDIDATES it
|
||||
* is forty-seven rows. A precision figure taken at n=12 does not survive
|
||||
* being applied at n=2000, and no value of it would: the fix for "a small
|
||||
* fraction of two thousand is still a lot" is a cap, not a stricter
|
||||
* fraction.
|
||||
*
|
||||
* **Why the cap is safe HERE when the note above says it is not.** That
|
||||
* objection is real and applies to a top-k taken over the UNION, which
|
||||
* would interact with OFFSET and make `COUNT(*) OVER ()` mean "lexical plus
|
||||
* up to k, on page one only". This LIMIT is inside `sem_hits`, which is a
|
||||
* CTE evaluated once per statement and does not see the page: the candidate
|
||||
* window is deterministic (`received_at DESC, id DESC`), so the admitted set
|
||||
* is byte-identical on every page and the total stays honest. The earlier
|
||||
* reasoning was right about the wrong position.
|
||||
*
|
||||
* Ten, because the semantic arm is a SUGGESTION and not an answer. A search
|
||||
* whose words found something shows what they found first — see
|
||||
* {@see \App\Domain\Enum\Mail\SearchSortOrder::orderBy()} — and these ride
|
||||
* behind it as a short tail somebody can read or ignore. A search whose
|
||||
* words found nothing is the case the feature exists for, and ten is a
|
||||
* reasonable answer there too.
|
||||
*/
|
||||
private const int SEMANTIC_HITS = 10;
|
||||
|
||||
/**
|
||||
* The threshold is NOT HERE ANY MORE, and its absence is the fix.
|
||||
*
|
||||
@@ -1481,10 +1557,9 @@ class MessageThreadRepository extends ServiceEntityRepository
|
||||
* it has to agree with, and it is a setting rather than a release. See
|
||||
* {@see \App\Domain\Ai\EmbeddingPreset} for the measurements.
|
||||
*
|
||||
* It stays a THRESHOLD rather than becoming "the nearest k", and that part
|
||||
* is unchanged: the total rides on the same statement as
|
||||
* `COUNT(*) OVER ()`, and a top-k arm would make that total "lexical
|
||||
* matches plus up to k", which is only true on the first page.
|
||||
* It is still a threshold. It is no longer the ONLY bound — see
|
||||
* {@see SEMANTIC_HITS}, which is the cap this docblock used to argue
|
||||
* against and which turns out to be safe in the place it is now written.
|
||||
*/
|
||||
|
||||
/**
|
||||
@@ -1776,8 +1851,15 @@ class MessageThreadRepository extends ServiceEntityRepository
|
||||
// MAX(0) rather than 0: this is one of the aggregate columns of a
|
||||
// GROUP BY, and a search with no free text at all still has to produce
|
||||
// the `rank` alias the sort orders by.
|
||||
$rankSelect = 'MAX(0)';
|
||||
$semanticColumn = '';
|
||||
$rankSelect = 'MAX(0)';
|
||||
|
||||
// Zero rather than absent, for the same reason `rank` is MAX(0) above:
|
||||
// the ORDER BY names this column on every search so that words sort
|
||||
// ahead of vectors, and a statement that omitted it when no vector ran
|
||||
// would be a search that fails only on the installations where the
|
||||
// feature is switched off. Overwritten below where there is a vector
|
||||
// to have an opinion.
|
||||
$semanticColumn = ",\n 0 AS semantic_only";
|
||||
$semanticRank = null;
|
||||
$semanticCte = '';
|
||||
|
||||
@@ -2001,7 +2083,11 @@ class MessageThreadRepository extends ServiceEntityRepository
|
||||
) recent
|
||||
)
|
||||
), sem_hits AS (
|
||||
SELECT message_id, similarity FROM sem_scores WHERE similarity >= :semanticSimilarity
|
||||
SELECT message_id, similarity
|
||||
FROM sem_scores
|
||||
WHERE similarity >= :semanticSimilarity
|
||||
ORDER BY similarity DESC, message_id DESC
|
||||
LIMIT :semanticHits
|
||||
)
|
||||
|
||||
SQL;
|
||||
@@ -2042,8 +2128,10 @@ class MessageThreadRepository extends ServiceEntityRepository
|
||||
$params['semanticModel'] = $semantic->model;
|
||||
$params['semanticDimensions'] = $semantic->dimensions;
|
||||
$params['semanticCandidates'] = $this->semanticCandidates;
|
||||
$params['semanticHits'] = $this->semanticHits;
|
||||
$types['semanticDimensions'] = ParameterType::INTEGER;
|
||||
$types['semanticCandidates'] = ParameterType::INTEGER;
|
||||
$types['semanticHits'] = ParameterType::INTEGER;
|
||||
}
|
||||
|
||||
if (null !== $free->substring) {
|
||||
|
||||
@@ -658,6 +658,87 @@ final class MessageSearchTest extends KernelTestCase
|
||||
self::assertCount(0, $page->threads, 'a vector from another model is not a hit');
|
||||
}
|
||||
|
||||
/**
|
||||
* The vector may suggest; it may not take over the page.
|
||||
*
|
||||
* A threshold alone does not bound a result set. Searching one company name
|
||||
* returned 63 rows against Gmail's 16 over the same mail: sixteen matches
|
||||
* on words and forty-seven rows that merely cleared 0.42 — job alerts, a
|
||||
* newsletter, an invoice, spam. The threshold was not mistuned; it was
|
||||
* measured on a twelve-message corpus, where a couple of percent of false
|
||||
* positives is invisible, and then applied to two thousand candidates,
|
||||
* where it is forty-seven rows. No value of a fraction fixes that.
|
||||
*
|
||||
* A cap of one here, with two rows that both clear the threshold, so the
|
||||
* assertion is about the bound rather than about the arithmetic.
|
||||
*/
|
||||
public function testTheSemanticArmIsCappedAndNotOnlyThresholded(): void
|
||||
{
|
||||
foreach (['Notes from the standup', 'Notes from the retro'] as $subject) {
|
||||
$this->seedMessage($subject, body: 'Who is doing what this week.');
|
||||
$this->store()->store($this->messageId($subject), [1.0, 0.0, 0.0], 'test-model');
|
||||
}
|
||||
|
||||
$query = $this->parser->parse('meeting minutes');
|
||||
$vector = $this->semantic([1.0, 0.0, 0.0], minSimilarity: 0.5);
|
||||
|
||||
self::assertCount(
|
||||
2,
|
||||
$this->repository->searchPage($this->user, $query, semantic: $vector)->threads,
|
||||
'both rows clear the threshold, so the fixture is about the cap and nothing else',
|
||||
);
|
||||
|
||||
$capped = new MessageThreadRepository(
|
||||
self::getContainer()->get(ManagerRegistry::class),
|
||||
new FreeTextCompiler(),
|
||||
semanticHits: 1,
|
||||
);
|
||||
|
||||
self::assertCount(
|
||||
1,
|
||||
$capped->searchPage($this->user, $query, semantic: $vector)->threads,
|
||||
'the cap admits the nearest and stops, however many others clear the bar',
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Words beat vectors, even when the vector's row is newer.
|
||||
*
|
||||
* This is the one that put the best match in the mailbox on page two. A
|
||||
* semantic hit can only come from the most recent SEMANTIC_CANDIDATES
|
||||
* messages, so under the default "most recent" sort every semantic hit is
|
||||
* structurally newer than most of the mail it competes with — they do not
|
||||
* merely tend to win, they cannot lose. The reply whose subject AND sender
|
||||
* both held the search term was six weeks older than the noise above it.
|
||||
*
|
||||
* Deliberately stacked against the fix: the lexical row here is a year
|
||||
* older than the semantic one, so recency alone would reverse this order.
|
||||
*/
|
||||
public function testAWordMatchOutranksAMeaningMatchEvenWhenItIsOlder(): void
|
||||
{
|
||||
$this->seedMessage('Quarterly figures', body: 'The pelican audit is attached.', receivedAt: '2025-01-01');
|
||||
$this->seedMessage('Notes from the standup', body: 'Who is doing what this week.', receivedAt: '2026-06-01');
|
||||
$this->store()->store($this->messageId('Notes from the standup'), [1.0, 0.0, 0.0], 'test-model');
|
||||
|
||||
$page = $this->repository->searchPage(
|
||||
$this->user,
|
||||
$this->parser->parse('pelican'),
|
||||
semantic: $this->semantic([1.0, 0.0, 0.0], minSimilarity: 0.5),
|
||||
);
|
||||
|
||||
self::assertCount(2, $page->threads, 'both arms still contribute — this is an ordering, not a filter');
|
||||
self::assertSame(
|
||||
'Quarterly figures',
|
||||
$page->threads[0]->subject,
|
||||
'the row the words found comes first even though it is eighteen months older',
|
||||
);
|
||||
self::assertSame(
|
||||
[$page->threads[1]->id],
|
||||
$page->semanticOnly,
|
||||
'and the row only the vector found is the one still badged, at the end',
|
||||
);
|
||||
}
|
||||
|
||||
/** The same, for a model that kept its name and changed its width. */
|
||||
public function testVectorsOfAnotherWidthAreNotSearched(): void
|
||||
{
|
||||
|
||||
Reference in new issue
Block a user