fix(vocab): stop reading the whole vocabulary for similar terms (#277) - #279
Merged
Conversation
Opening the term editor selected every term of the language into PHP and profiled each one — letter pairs plus phonetic normalisation — before the 0.33 threshold threw almost all of them away. That is affordable for the few thousand terms a reader adds by hand, but a vocabulary seeded from a dictionary import runs to hundreds of thousands, and clicking a word spent a minute before dying on the memory limit in PreparedStatement::fetchAll(). Move the admission test into SQL so the rows that come back are the ones that stood a chance. Two ways in, matching how rankByCoverage() admits a candidate: the same word family, or enough shared letter pairs. Dice is 2|A∩B| / (|A|+|B|), so clearing a threshold t needs |A∩B| >= t(|A|+|B|)/2, and dropping the candidate's own pair count leaves the weaker t|B|/2 that a sum of LIKE tests can check. A cap backs that up, with the scan ordered by shared pairs so a vocabulary large enough to reach it loses its weakest matches; the rows go back into WoID order before ranking, since ties are broken on whichever candidate was seen first. Below the cap the results are unchanged. The prefilter needs placeholders, and hand-writing the SQL would have lost QueryBuilder's automatic WoUsID scoping, so whereRaw() and selectRaw() take an optional bindings array. Both stay backward compatible. On a seeded 200k-term vocabulary: 174 MB peak becomes 8 MB, 0.8-1.2s becomes 0.2-0.5s, and the suggestions are identical. At the default memory_limit of 128M the old path reproduces the reported fatal; the new one peaks at 4 MB. The one narrowing is documented: the bound covers the character half of the score, so a term whose spelling has almost nothing in common is no longer admitted on pronunciation alone. With the defaults the phonetic side alone tops out at 0.30 and cannot clear the 0.33 threshold anyway.
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.
Fixes #277.
Root cause
FindSimilarTerms::execute()selected every term of the language into PHP and built aLetterPairProfilefor each — letter pairs plus phonetic normalisation — before the 0.33 threshold threw almost all of them away. Fine for a hand-built vocabulary; fatal for one seeded from a dictionary import. The reported fatal atPreparedStatement.php:209is the row-accumulation loop infetchAll(), exactly as @sarahmccuan guessed in the issue.Both branches of
GET /api/v1/terms/for-editreach it throughgetSimilarTermsForEdit(), so every word click paid for it.Fix
The admission test moves into SQL, so the rows that come back are the ones that stood a chance. Two ways in, matching the two ways
rankByCoverage()admits a candidate:WoLemmaLC = ? OR WoTextLC = ?, mirroringsharesWordFamily(), so irregular forms still get in whatever they score on spelling.2|A∩B| / (|A|+|B|), so clearing a thresholdtneeds|A∩B| >= t(|A|+|B|)/2; dropping the candidate's own pair count, unknown until it is read, leaves the weakert|B|/2that a sum ofLIKEtests can check. A term pair never spans a space, so it belongs to a candidate's pair set exactly when it is a substring of the candidate.A
LIMITbacks that up, with the scan ordered by shared-pair count so a vocabulary large enough to reach the cap loses its weakest matches rather than whichever have the lowestWoID. The rows then go back intoWoIDorder before ranking, becauserankByCoverage()breaks ties on whichever candidate it saw first — below the cap the results are unchanged.QueryBuilder::whereRaw()andselectRaw()gained an optional$bindingsarray. The prefilter needs placeholders, and hand-writing the SQL would have gone around the builder's automaticWoUsIDmulti-user scoping. Both signatures stay backward compatible — the one existingwhereRaw()caller passes SQL only.Measured, on a seeded 200k-term vocabulary
At PHP's default
memory_limit=128Mthe old path reproduces the reportedAllowed memory size ... exhausted; the new one returns its 10 suggestions at a 4 MB peak.Known narrowing
The bound covers the character half of the score. A term whose spelling has almost nothing in common with the searched one but whose pronunciation does is no longer admitted on phonetics alone. With the defaults (
minRanking = 0.33,phoneticWeight = 0.3) the phonetic side alone tops out at 0.30 and cannot clear the threshold anyway. It is documented onbuildCandidateFilter().Tests
Eight new tests: five for the prefilter — including a property test that it never drops a term the ranking would keep, plus word-family admission, the threshold scaling with term length, the short-term short circuit, and
%/_escaping — and two forwhereRaw()binding order.Psalm 0 errors, PHPCS 0 errors and 0 warnings, PHPUnit 9098 pass. Live checks also confirm
100%_offreturns nothing rather than erroring, and a term too short for a letter pair short-circuits without a query.