Commit Graph
7 Commits
Author SHA1 Message Date
timothy 05542946ad fix(668): reach accented facet values via a registered Unicode fold on SQLite
SQLite's LOWER() folds ASCII only -- lower('Édith') is 'Édith' unchanged --
so the EF-sourced facet fields UNDER-matched any stored value whose prefix
carries an uppercase non-ASCII character. An under-match is unrecoverable:
no later stage can reintroduce a row SQL never returned.

Adds a SECOND, ADDITIVE query taken only when the provider is SQLite and q
contains a non-ASCII character: raw Dapper SQL folding through etv_upper(),
a SqliteConnection.CreateFunction scalar implementing ToUpperInvariant.
Every other case -- all-ASCII q, and MySQL for all q -- runs the existing
EF query byte-identically.

MySQL needed no change and gets none: verified on MySQL 8.4 that its LOWER()
is Unicode-aware and its ci collation makes the predicate OVER-match, which
the existing ordinal filter already discards.

The fold is ToUpperInvariant because OrdinalIgnoreCase equality is a strict
SUBSET of invariant-uppercase equality, so the SQL stage yields a superset of
the final filter's matches and can never under-match. Note OrdinalIgnoreCase
is NOT "invariant-upper then ordinal": ToUpperInvariant('ſ') is 'S', yet
"ſweet".StartsWith("S", OrdinalIgnoreCase) is false. Tests pin that.

No migration, no model change; both provider snapshots are untouched.

Refs #668
Decisions-Edit: yes
2026-07-27 20:36:28 +02:00
timothy fc3ede09bc docs(578): the retracted claim survived in 9 places, including the record title and rule
PR Gates / Script tests (pytest) (pull_request) Successful in 51s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 1m38s
Review verdict / Set review-verdict status (pull_request) Successful in 1m18s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 4m51s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 18m52s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 21m10s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 25m44s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
review-verdict/h10 Review-verdict: MERGEABLE @ fc3ede0 (base: main)
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 7s
PR Gates / Docs update reminder (pull_request) Successful in 8s
PR Gates / decisions lifecycle (pull_request) Successful in 21s
Comment- and docs-only; verified no non-comment line changed in any .cs.

I reported last round that I had "classified every surviving hit". That was false, and the false
confidence is the expensive part: a confidently-stated "I checked everything" stops anyone else
checking. The retracted wording survived in nine places, two of them the record's title and rule: —
and the catalog copies rule: verbatim, so the generated entry point and the record disagreed
semantically while docs/decisions.md said the correct thing.

Root cause of the miss, because it will recur otherwise: I built the sweep term list from the
DELETED MECHANISM's vocabulary (LIKE, superset, keyspace, anchor, over-match) and never added the
RETRACTED CLAIM's own words. "no predicate", "bound on work", "index entries", "no gap" and
"holds in memory" were never grepped. After a retraction the subject list has to include the words
of the thing being retracted, not just the thing already deleted.

Second, worse: my first attempt at this round's sweep printed nothing for every term and I nearly
read that as "all clear". zsh does not word-split an unquoted $FILES, so grep received one giant
non-existent path — and the `|| echo "(none)"` never fired because the pipeline's exit status was
sed's. Same failure shape as the bug arc itself: a check reporting success while examining nothing.
Re-run with a proper array plus a control term ("SongMetadata" -> 42 hits) so an empty result is
distinguishable from a broken grep.

Fixed all nine, replacing "no predicate" with the seekable-cursor-vs-residual distinction already
written correctly elsewhere:
- handler: the "real bound on work" claim, the short-page rationale
- SearchFieldValuesQueryShapeTests: "ANY predicate" + "reads exactly n index entries", and added what
  the test can and cannot pin (a SQL string, not a plan / visibility work / payload I/O)
- GetSearchFieldValuesHandlerTests: "no gap between what the engine looks at and what it hands back",
  and the current-behaviour comment
- record title, rule:, attempt-5 table row; api-conventions
- regenerated docs/decisions/README.md so catalog and record agree again

Tenth item, the same overclaim one level down and it survived the first retraction: the row bound was
said to cap what the process holds in memory. It does not — payload width is unrestricted and one
JSON array can contain arbitrarily many strings, each of which may enter the in-memory distinct set.
It caps logical rows returned/materialized and round-trip count, nothing about bytes. Added as a
third struck-through bullet next to the other two retractions.
2026-07-27 03:10:36 +02:00
timothy 5f73cd4482 docs(578): scope the bound to LOGICAL ROWS — physical work is not bounded, and I claimed it was
Comment- and docs-only. Verified: the diff for both .cs files contains no non-comment line.

HIGH (claim). "LIMIT n reads exactly n index entries and n rows" is false, and the bounded-physical-
work reading it implied is false with it. Two mechanisms, both retracted explicitly in the record
rather than quietly reworded:

- MySQL purge lag. Deleted clustered-index records survive until purge runs and a range scan still
  traverses them, so returning 2,000 VISIBLE rows can touch far more index records. Deletion history
  therefore STILL affects physical work — precisely what attempt 4's failure was supposed to have
  made irrelevant. Attempt 5 removes the LOGICAL dependence on Id distribution; it does not make
  physical work independent of deletion history.
- Unbounded row width. Artists/AlbumArtists are unrestricted TEXT/longtext and both SQLite and InnoDB
  spill to overflow pages, so a row count implies neither a byte count nor a page-read count.

The guarantee is now stated as exactly what it is: at most 20,000 LOGICAL rows returned/materialized,
and at most 10 round trips (11 for artist). The 392 KiB measurement is labelled one data point on one
library whose credits average ~20 B/row, with an instruction to re-measure rather than extrapolate
for long credit lists or MySQL over a network. Also noted: the query-shape test pins the SQL STRING —
it cannot pin a plan, MVCC visibility work or payload I/O, and on MySQL using the index to satisfy
ORDER BY is an optimizer choice, not a semantic guarantee.

Precision fix, and the reviewer is right that the sloppy version misleads: "any predicate defeats
LIMIT" is wrong, since the query's own cursor is a predicate. The distinction is a SEEKABLE PREDICATE
ON THE ORDERING KEY (positions the scan, never discards a row) versus a RESIDUAL predicate (discards
rows the engine already produced, so LIMIT bounds survivors and says nothing about how many were
produced). Restated in the handler, the record and api-conventions.

MEDIUM. docs/decisions.md still advertised "a superset LIKE pre-filter that may over-match but never
under-match" for list-valued columns. That is the documented entry point for convention lookups, so a
maintainer starting there was told to preserve exactly what round 4 deleted, in direct contradiction
of the linked active record. Fixed.

LOW. Two test comments read as current: "however many non-matching rows" (false past the ceiling —
now bounded and cross-referenced to the test that pins that boundary) and the ampersand case
"widens to the bare anchor" (no prefix predicate exists; kept because it is the input shape that
broke the old scheme, now labelled as such).

Fifth consecutive round of stale text, so this sweep was done by grepping the subject across handler,
tests, record, decisions.md, api-conventions.md and the endpoint description: LIKE, ESCAPE,
pre-filter, superset, over-match, under-match, anchor, keyspace, window, candidate, row cap, index
entries, 392, and every deleted constant name. Every surviving hit is now either current-and-correct
or explicitly framed as history.
2026-07-27 03:10:36 +02:00
timothy 373956fcee fix(578): delete the SQL predicate — LIMIT only bounds work when there is nothing to discard
BLOCKER. Attempt 4 bounded the Id KEYSPACE, and keyspace is not rows. Delete 20,000 historical rows,
put one song at Id 20001, query artist?q=que: the walk burned all ten windows on empty ranges and
returned [] for a table containing exactly one row. Capacity fell linearly with deletion ratio and no
ratio was safe — one placed gap hides the next match. My record called that "heavily fragmented" and
the endpoint description said loss happens "on a very large library"; the one-row example disproves
both.

Option A. The query now carries NO predicate at all — no LIKE, no LOWER, not even IS NOT NULL:

  SELECT Id, Artists AS Payload FROM SongMetadata WHERE Id > @AfterId ORDER BY Id LIMIT @Batch

That is the whole fix, and it is the point. LIMIT truncates what survives a predicate, so with any
predicate present it bounds the OUTPUT and says nothing about the WORK; the engine may evaluate and
discard arbitrarily many rows first. Stripped to a bare primary-key range, LIMIT n reads exactly n
index entries and n rows — independent of sparsity, deletion history or where the gaps fall. All
selectivity moves into memory. A short page can now only mean exhaustion, which is precisely what it
could not mean while a predicate was present.

Four attempts, four wrong quantities: the result (a fixed budget the over-matching pre-filter
starved), candidates returned (a no-match query must evaluate every eligible row before returning an
empty page), keyspace width (above), and finally actual rows. The record carries the table; it is
worth more than the code.

Deleting the predicate deletes a whole bug family with it: the JSON-escape reasoning, the
narrow-only-on-verbatim-ASCII rule, the exhaustive Unicode sweep that proved it sound, the ESCAPE '/'
portability workaround, and the may-over-match-never-under-match invariant that turned out to be
conditional on something untrue. SearchFieldValuesPrefilterSupersetTests is deleted entirely; the one
assertion worth keeping — that the SQL has no predicate — moved to the query-shape suite, which pins
the SQL string exactly so "just a cheap filter" fails a test instead of silently unbounding the walk.

Measured cost of no server-side narrowing, on a seeded 20,000-song library (in-memory SQLite):
worst case (no match, full walk) 20,000 rows / 10 round trips / 391.9 KiB / 119ms SQL, ~40ms warm
end-to-end. Empty q, dense and non-ASCII prefixes all stop on page 1 at ~39 KiB and ~40ms. Judged
acceptable for a debounced typeahead against a local file. If it ever is not, the answer is #669, not
reintroducing selectivity — the record says so explicitly.

Also fixed:
- Round-trip count was advertised as 10; it is at most 10 for album_artist and 11 for artist, which
  also runs its EF query. The MAX(Id) probe is gone with the keyspace scheme, so there is no extra
  scalar call.
- The duplicated-formula ceiling test is deleted rather than rewritten. It re-implemented the loop's
  arithmetic and would have passed through an off-by-one or a stall in the real loop; the dense
  integration tests carry that coverage. Its MaxVisited >= Window assertion was a style constraint in
  correctness clothing.
- Stale text swept by grepping the mechanism nouns rather than re-reading: candidate/keyspace/
  pre-filter/superset/row cap/LIKE/ESCAPE and the removed constant names, across handler, tests,
  record, api-conventions and the endpoint description. The two surviving "pre-filter" mentions are
  deliberate history. Test comments that rendered escaped non-ASCII as literal characters (which
  contradicted the raw-storage assertion in the same file) now show the escape text.

New test List_Valued_Walk_Reads_Live_Rows_Regardless_Of_Id_Density reproduces the one-row killer and
fails against attempt 4.
2026-07-27 03:10:36 +02:00
timothy fbc7b2a1dd fix(578): bound the Id KEYSPACE — LIMIT cannot bound a query that matches nothing
BLOCKER. Round 3's ceiling counted LIKE-positive candidates, which is not the quantity that needed
bounding. To return an empty page the engine must first evaluate every eligible row, so a no-match
query came back with rows.Count == 0, ended the walk having counted zero against the ceiling, and had
already inspected the entire table. Round trips and materialized rows were bounded; database work was
not. Worse, a dense widened prefix could materialize 20x the candidates and make 10x the round trips
of round 1 — a regression dressed as a bound.

Third time bounding the wrong quantity: revisions 1-2 bounded the RESULT (a fixed LIMIT budget the
widened pattern starved), revision 3 bounded the CANDIDATES, and neither bounds what the database
LOOKS AT. Now the Id range is closed on both sides — `Id > @AfterId AND Id <= @AfterId + @Window` —
so each round trip is a primary-key range scan of known width. The LIKE still decides what comes
back; it no longer decides how much gets looked at. The walk advances by the WINDOW, never by what
returned (an empty page means "nothing matched in this stretch", not "exhausted"), and there is
deliberately no LIMIT in the SQL — the window caps the row count, and a LIMIT would only restore the
illusion that it is doing the bounding. One indexed SELECT MAX(Id) up front stops the walk burning
windows on empty keyspace. Result: at most 10 round trips and 20,000 rows inspected for any q,
matching or not.

New test `List_Valued_Walk_Cannot_Inspect_Past_The_Bound_When_Nothing_Matches` covers the case every
earlier bound test missed — they all gave the pre-filter something to return. With a narrow pattern
and a needle at row 30,000, revision 3 returns the needle (right answer, unbounded work) and this
revision returns []. Paired with a positive control at row 20,000 inside the bound.

The ceiling's remaining false negative (20,000 "zzz" rows then "éclair" at 20,001 -> []) is kept and
still pinned: it is the documented bounded-best-effort contract at a rarer threshold, not a defect.

Stale/false comments, several of them repeat offenders:
- The handler still used "Zulu"/"Éclair" as the truncation example, where both orderings pick "Zulu".
  I had corrected the test and left the comment. Now "Zulu"/"apple" in both.
- "leans entirely on the row cap" described a mechanism that no longer exists.
- The escape example rendered as literal Édith/É/é rather than the escape TEXT (Édith,
  É, é) in both the handler and the record, contradicting the explanation it was giving.
- "every accented artist was unsuggestable" overstated it: exactly three of the nine pinned cases
  fail 1b78dc9e (those where query and stored casing differ, so the escape texts diverge); the other
  six pass. The record now says what the test comment already said.
- The record claimed the ordinal switch left response SETS unchanged. False — ordering runs before
  Take(limit), so with "Zulu"/"apple", empty q and limit=1 the comparer changes which value survives.
- The_Scan_Ceiling_Is_A_Whole_Number_Of_Batches was a style preference wearing a correctness costume
  (a 20,500 ceiling is perfectly safe — the final window clamps). Replaced with the invariant that
  actually matters: the walk's own arithmetic terminates and lands exactly on the ceiling.
2026-07-27 03:10:35 +02:00
timothy a37847e509 fix(578): a fixed row budget turns an over-matching prefilter into a false negative
BLOCKER 1. The invariant a883e5f0 established — "the pre-filter may over-match, it must never
under-match" — is only sound while the candidate set is not truncated. It was truncated. A non-ASCII
or JSON-escaped prefix collapses the pattern to the bare `%"%` anchor, so every row becomes a
candidate, and `ORDER BY Id LIMIT 1000` then spent the whole budget on rows that could not match.
Seed 1000 songs by "zzz", put the only "éclair" in row 1001, ask for album_artist?q=é: a883e5f0
returns [], while 1b78dc9e returned "éclair" because its (separately broken) tighter pattern kept
the candidate set small. Neither revision was correct — the old one under-matched at the pattern,
the new one under-matched at the cap. Widening a predicate under a fixed budget starves it.

So the budget is gone. Candidate rows are now walked keyset-paged on Id (`Id > @AfterId … ORDER BY
Id LIMIT @Batch`), continuing past non-matching candidates and stopping on the first of: enough
distinct exact matches for `limit`, a short page (source exhausted), or a 20,000-candidate-row
ceiling in 2,000-row batches. The bound is on effort; it no longer silently decides the result, and
the lossy case needs 20,000 rows that already passed the pre-filter before it bites.

BLOCKER 2. The endpoint description and the record's rule claimed ordinal matching/dedup/ordering
endpoint-wide. False for EF-backed fields: the database runs LOWER/DISTINCT/ORDER BY/LIMIT before
any ordinal code, so `genre?q=é` still misses a stored "Éclair" on SQLite. Both are now scoped to
the final in-memory stages, and the underlying gap is referenced as #668 rather than described as
fixed. #669 (normalized SongArtist table) is referenced as the follow-up for the scan cost.

Accuracy corrections to my own claims, all verified by re-running the mutations:
- Only THREE of the nine Unicode cases fail 1b78dc9e (é/édith/BJÖRK — where query and stored casing
  differ, so the escape texts diverge); the other six pass it. The comment said all nine. They stay
  as continuity coverage, now labelled as such rather than as regression guards.
- Ordering_Is_Best_Effort used "Zulu"/"Éclair", where DB and ordinal orderings BOTH pick "Zulu" — it
  could not demonstrate the divergence it claimed. Now "Zulu"/"apple", which actually diverges:
  ordinal ranks "Zulu" first, the DB ranks "apple" first, and limit=1 returns ["apple"]. The record
  sentence was false and is corrected.
- The record printed literal "Édith"/"é" where it needed to show the escape TEXT (Édith,
  é), contradicting the very explanation it was giving.
- Corrected the cost claim: the leading wildcard forces scan ACCESS, but each page stops once it has
  filled @Batch, so a dense query finishes early — it is not necessarily a full table scan.
- The Unicode sweep is labelled a PROOF OBLIGATION: it is revision-independent and passes every
  revision, which is correct for what it is but must not read as regression coverage.

One process note: the new record's frontmatter had a lone apostrophe inside a single-quoted YAML
scalar ("SQLite's"). decisions_validate.py's hand parser accepted it; scripts/tests caught it.
2026-07-27 03:10:35 +02:00
timothy 1641ca8305 fix(578): the LIKE prefilter under-matched every accented artist; make the superset provable
Review of 1b78dc9e found the pre-filter's correctness claim was false, and the claim was in the
decision record as well as the code.

F1 (high). The pattern JSON-encoded the whole query prefix on the reasoning that the stored text
escapes non-ASCII, so encoding the prefix the same way would line up. It does not: SQL LOWER()
lowercases the *escape text* (`É` -> `é`); it cannot case-fold the codepoint that escape
denotes. So `q=é` built `%"é%`, the stored `Édith Piaf` never matched, and the row was
discarded before the in-memory filter could accept it. Every accented artist — Beyoncé, Björk,
Sigur Rós, Édith Piaf — was silently unsuggestable, which in a music library is the common case.

The invariant that was missing, now stated in the code: the SQL pre-filter is an OPTIMIZATION. It
may over-match; it must never under-match. Correctness lives in the in-memory filter. So the pattern
now narrows only on the leading run of characters the JSON writer stores verbatim and stops at the
first character it cannot prove — `q=Beyoncé` still narrows on `beyonc`, `q=é` narrows on nothing
and leans on the row cap. Soundness rests on two facts now asserted by exhaustive computation rather
than argued: no non-ASCII codepoint in U+0080..U+10FFFF OrdinalIgnoreCase-equals a printable ASCII
character (false for InvariantCultureIgnoreCase, which folds ~190 — the choice of Ordinal is
load-bearing), and the exact set of ASCII the encoder escapes.

F1b. `UseRequestLocalization` honours Accept-Language, so the culture was caller-controlled and
`ToLower()` plus the default linguistic `StartsWith(string)` let a header change the answer.
Comparison is now OrdinalIgnoreCase and ordering StringComparer.Ordinal throughout — including the
shared FilterSortTake that state/video_dynamic_range/content_rating also use. Sets unchanged,
order now ordinal rather than culture-dependent.

F2. The merge comment asserted an exactness the code does not have: sources truncate by their own
ordering (DB collation / primary key), not the merge's, so a dropped value can outrank a survivor.
Comment and record now say best-effort, exact only below the truncation points.

F3/F4. The cap now rides `ORDER BY Id` rather than the JSON column: MySQL sorts TEXT by only
max_sort_length bytes, so the old ordering was not deterministic there, and sorting the whole
matching set was avoidable work. What the cap still does NOT bound is the scan — a leading-wildcard
LIKE cannot seek an index — so that cost is now documented as accepted, with a normalized
`SongArtist` table named as the follow-up candidate rather than left implicit.

Every clause above is covered by a test verified to FAIL when that clause is mutated (old pattern
builder: 5 red; culture chain: 3 red; cap=3 / cap=limit / ORDER BY json / no cap: red each).

F5. Converted to a proper supersession. The old record did not merely hold a stale fact — it
recorded song/music-video credits as an "intentionally-uncovered gap" and album_artist as
unsupported, and this reverses that call, which `docs.decision-lifecycle` says is never a
line-edit. `api.search-field-values` is archived with its original prose restored, and
`api.search-field-values-sources` replaces it carrying the whole endpoint contract.
2026-07-27 03:10:35 +02:00