From d7696237548d759d363df5bbe49608899e5931a3 Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Tue, 27 Oct 2026 12:00:31 -0700 Subject: [PATCH] fix(tagging): Images tag search goes through the tag index (#379, PR 3) The Images Tags filter and text search no longer stream every image tag row per request. image_tag_ids reads the names that tag at least one image (exact under Tags, case-folded substring for q) and image_tag_locators reads their files by tag_id through idx_media_image_tags_tag. Results are unchanged: a differential test compares the lookup with the old stream, and a trace test checks that no statement reads media_image_tags except by tag id. The search contract in TESTING.md and a browser test (CUJ 8) follow. Closes #379 --- TESTING.md | 13 +- database/annotations.py | 56 ++++++--- py/api/images.py | 26 ++-- tests/browser/workspace-tagging.spec.mjs | 35 ++++++ tests/test_annotations.py | 18 +-- tests/test_api_workspace_images.py | 148 ++++++++++++++++++++++- 6 files changed, 250 insertions(+), 46 deletions(-) diff --git a/TESTING.md b/TESTING.md index 1b56e9b3..d79b9cd0 100644 --- a/TESTING.md +++ b/TESTING.md @@ -4238,8 +4238,17 @@ impact and command receipts report `image_count` / `images` / `(size, mtime_ns)` equals its verified fingerprint; an in-place rewrite that preserves both keeps the old set matching until the file is next observed. The Images Tags filter (a file matches when every tag is on its linked positive or -its current image tag set) and `q` search stream the tagged records once and -stat only the listed files whose records carry a filter tag or a `q` hit; the +its current image tag set) and `q` search find their candidates through the tag +index (#379), never by streaming every image tag row: `image_tag_ids` reads the +tag names that tag at least one image (an `EXISTS` on `idx_media_image_tags_tag`) +and keeps a name when it equals a Tags filter tag exactly or contains `q` +case-folded; `image_tag_locators` reads the files carrying those ids (`tag_id IN`, +500 ids a chunk). Only the listed ones among them are stat'ed, and only their +current records count; a linked positive's prompt tags still combine with the +image tags under a Tags filter. `test_indexed_candidates_equal_the_full_stream` +compares the lookup with the old full stream (kept in the test as the reference) +and `test_tag_search_reads_image_tags_only_by_tag_id` traces every statement; the + inspector's `image_tags` facet (`current` / `none` / `unavailable`) uses the inspection's own stable stat (`tests/test_api_workspace_images.py` `TestWorkspaceImageTags`, `tests/test_api_prompt_inspector.py`). The diff --git a/database/annotations.py b/database/annotations.py index 219a5039..d7b4b681 100644 --- a/database/annotations.py +++ b/database/annotations.py @@ -96,12 +96,16 @@ _TAGS_OF = ( "JOIN tags t ON t.id=mit.tag_id WHERE mit.byte_id IN ({marks}) " "ORDER BY mit.byte_id,mit.position" ) -_RECORDS = ( - "SELECT f.locator,b.id,b.size,b.mtime_ns,t.name " # nosec B608 - f"FROM {FILES_TABLE} f JOIN {BYTES_TABLE} b ON b.file_id=f.id " - f"LEFT JOIN {IMAGE_TAGS_TABLE} mit ON mit.byte_id=b.id " - "LEFT JOIN tags t ON t.id=mit.tag_id " - "ORDER BY f.locator,b.first_seen_at,b.id,mit.position" +# Tag search (#379): the names that tag at least one image (the tag index +# answers the EXISTS), then the locators carrying chosen names (by tag_id). +_IMAGE_TAG_NAMES = ( + "SELECT t.id,t.name FROM tags t WHERE EXISTS " # nosec B608 + f"(SELECT 1 FROM {IMAGE_TAGS_TABLE} mit WHERE mit.tag_id=t.id)" +) +_TAG_LOCATORS = ( + f"SELECT DISTINCT f.locator FROM {IMAGE_TAGS_TABLE} mit " # nosec B608 + f"JOIN {BYTES_TABLE} b ON b.id=mit.byte_id " + f"JOIN {FILES_TABLE} f ON f.id=b.file_id WHERE mit.tag_id IN ({{marks}})" ) @@ -442,20 +446,32 @@ class AnnotationOperations: result[owners[byte_id]].append(name) return result - def iter_image_tag_records(self) -> Iterator[tuple[str, int, int, list[str]]]: - """Stream every byte record as ``(locator, size, mtime_ns, tags)``. + def image_tag_ids(self, wanted, query: str) -> list[int]: + """Ids of the tag names on at least one image that a search wants. - One SELECT ordered by locator, then by the bytes' first sighting, with - each set's names in position order. + A name counts when it is in ``wanted`` exactly (the Tags filter) or + contains ``query`` case-folded (the text search; ``query`` is + already folded). One read of the names that tag an image. """ + wanted = set(wanted) conn = self.model.get_connection() - current: tuple | None = None - for locator, byte_id, size, mtime_ns, name in conn.execute(_RECORDS): - if current is None or current[0] != byte_id: - if current is not None: - yield current[1] - current = (byte_id, (locator, size, mtime_ns, [])) - if name is not None: - current[1][3].append(name) - if current is not None: - yield current[1] + return [ + tag_id + for tag_id, name in conn.execute(_IMAGE_TAG_NAMES) + if name in wanted or (query and query in name.casefold()) + ] + + def image_tag_locators(self, tag_ids) -> set[str]: + """Locators with any byte record (current or not) carrying one of + ``tag_ids``, through the tag index, in chunks of 500 ids.""" + ids = list(dict.fromkeys(tag_ids)) + found: set[str] = set() + with self._annotation_read() as conn: + for start in range(0, len(ids), _LOOKUP_CHUNK): + chunk = ids[start : start + _LOOKUP_CHUNK] + marks = ",".join("?" * len(chunk)) + found.update( + row[0] + for row in conn.execute(_TAG_LOCATORS.format(marks=marks), chunk) + ) + return found diff --git a/py/api/images.py b/py/api/images.py index bfafcca0..95d5e2ec 100644 --- a/py/api/images.py +++ b/py/api/images.py @@ -575,12 +575,6 @@ def _current_fingerprint(path: Path) -> tuple[int, int] | None: return observed.st_size, observed.st_mtime_ns -def _image_tags_relevant(names: list, wanted: set, query: str) -> bool: - if wanted and not wanted.isdisjoint(names): - return True - return bool(query) and any(query in name.casefold() for name in names) - - def _workspace_key(entry: tuple, order: str) -> tuple: """The keyset sort key of *entry* (a _Listed or a 4-tuple entry).""" if not isinstance(entry, _Listed): @@ -2091,8 +2085,9 @@ class ImageRoutesMixin: def _image_tag_matches(self, listings: list[tuple], filters: dict) -> dict: """Normalized path -> _ImageTagMatch for files whose image tags count. - Streams the tagged byte records once and keeps only listed locators - carrying a filter tag or a ``q`` hit; only those candidates are + Looks the relevant tag names up (exact under Tags, a case-folded + substring for ``q``) and their files through the tag index, keeping + only listed locators; only those candidates are stat'ed (never every listed file), and a candidate's tags count only while a byte record is verified at its present ``(size, mtime_ns)`` (spec amendment S1). Under a Tags filter the candidates' linked @@ -2101,16 +2096,15 @@ class ImageRoutesMixin: tags, query, category = filters["tags"], filters["query"], filters["category"] wanted = set(tags) located = self._workspace_locators(listings) + # The tag index answers which files carry a relevant name (#379); no + # read streams every image tag row. + tag_ids = self.db.image_tag_ids(wanted, query) candidates = {} - records = self.db.iter_image_tag_records() - try: - for locator, _size, _mtime_ns, names in records: + if tag_ids: + for locator in self.db.image_tag_locators(tag_ids): entry = located.get(locator) - if entry is not None and locator not in candidates: - if _image_tags_relevant(names, wanted, query): - candidates[locator] = entry - finally: - records.close() + if entry is not None: + candidates[locator] = entry stats = {} for locator, entry in candidates.items(): fingerprint = _current_fingerprint(entry.path) diff --git a/tests/browser/workspace-tagging.spec.mjs b/tests/browser/workspace-tagging.spec.mjs index a5e6c989..eaefe7ba 100644 --- a/tests/browser/workspace-tagging.spec.mjs +++ b/tests/browser/workspace-tagging.spec.mjs @@ -513,6 +513,41 @@ test('search finds an image by its image tag', async ({ page, request }) => { await expect(card(page, UNLINKED)).toBeVisible(); }); +// #379 CUJ 8: the Tags filter finds images by their own image tags through the tag index; no prompt carries them, and +// selecting a match shows its prompt. A linked prompt's tag still combines with the image tags. +test('the Tags filter finds images by image tags only, and selecting one shows its prompt (#379)', async ({ page, request }) => { + await saveImageTags(request, FIRST, ['beach', '1girl', 'waves']); + await saveImageTags(request, SECOND, ['beach', 'waves']); + await saveImageTags(request, UNLINKED, ['beach', '1girl', 'waves', 'gulls']); + await page.goto(images); + await expect(page.locator('.gallery-body article[data-image-key]')).toHaveCount(manifest.files.length); + const filters = page.locator('#workspace-main search.toolbar').getByRole('button', { name: /^Filters(?: ยท \d+)?$/ }); + const refine = page.getByRole('dialog', { name: 'Refine this view', exact: true }); + const applyTags = async value => { + await filters.click(); + await refine.getByLabel('Tags', { exact: true }).fill(value); + const listing = page.waitForRequest(item => item.url().includes('/images/output?') + && new URL(item.url()).searchParams.get('tags') !== null); + await refine.getByRole('button', { name: 'Apply filters', exact: true }).click(); + await listing; + }; + await applyTags('beach, 1girl, waves'); + // Every listed image whose image tags hold all three; SECOND lacks 1girl. + await expect(page.locator('.gallery-body article[data-image-key]')).toHaveCount(2); + await expect(card(page, FIRST)).toBeVisible(); + await expect(card(page, UNLINKED)).toBeVisible(); + await expect(card(page, SECOND)).toHaveCount(0); + // The newest match is selected; the inspector shows its linked prompt. + await infoButton(page, FIRST).click(); + await expect(card(page, FIRST)).toHaveClass(/\bselected\b/); + await expect(panel(page).locator('.panel-heading h2')).toHaveText(manifest.title); + // The linked positive's own tag combines: SECOND has beach and waves itself and minimal through its prompt. + await applyTags(`beach, waves, ${manifest.prompt_tags[0]}`); + await expect(page.locator('.gallery-body article[data-image-key]')).toHaveCount(2); + await expect(card(page, FIRST)).toBeVisible(); + await expect(card(page, SECOND)).toBeVisible(); +}); + // Task C4 rulings: a save re-reads the selected file's details without a loading flash, and under a Tags filter or a // search the loaded listing is re-read in place (scroll and selection kept while the file still matches). const cards = page => page.locator('.gallery-body article[data-image-key]'); diff --git a/tests/test_annotations.py b/tests/test_annotations.py index a5aa6002..0d3257cf 100644 --- a/tests/test_annotations.py +++ b/tests/test_annotations.py @@ -430,7 +430,9 @@ class ReplaceImageTagsTests(AnnotationTestCase): self.assertTrue( self.db.refresh_image_fingerprint(loc, DIGEST_A, SIZE, MTIME + 1) ) - list(self.db.iter_image_tag_records()) + self.assertEqual( + self.db.image_tag_locators(self.db.image_tag_ids(["img"], "")), {loc} + ) self.assertEqual(self.db.get_prompt_by_id(prompt)["tags"], ["prompt-tag"]) @@ -551,13 +553,15 @@ class CurrentImageTagsTests(AnnotationTestCase): self.assertEqual(current, {locators[0]: ["z", "y"], locators[502]: ["x"]}) lookups = [s for s in statements if "media_annotation_files" in s] self.assertEqual(len(lookups), 2) + # The tag search finds every record's file by tag id, the empty set's + # file never, and each set keeps its position order. self.assertEqual( - list(self.db.iter_image_tag_records()), - [ - (locators[0], SIZE, MTIME, ["z", "y"]), - (locators[250], SIZE, MTIME + 1, []), - (locators[502], SIZE, MTIME + 2, ["x"]), - ], + self.db.image_tag_locators(self.db.image_tag_ids(["z", "x"], "")), + {locators[0], locators[502]}, + ) + self.assertEqual( + self.db.current_image_tags({locators[250]: (SIZE, MTIME + 1)}), + {locators[250]: []}, ) diff --git a/tests/test_api_workspace_images.py b/tests/test_api_workspace_images.py index 60110dbb..9fb3b7cf 100644 --- a/tests/test_api_workspace_images.py +++ b/tests/test_api_workspace_images.py @@ -1831,6 +1831,39 @@ class TestWorkspaceCursorEncoding(unittest.TestCase): self.assertEqual(_workspace_cursor_key(token, "scope", order), key) +# The pre-#379 candidate read, kept as the differential reference for the +# indexed lookup (`image_tag_ids` + `image_tag_locators`): every byte record +# streamed with its tag names, a locator kept when any record's names are +# relevant (`_image_tags_relevant`). +_REFERENCE_RECORDS = ( + "SELECT f.locator,b.id,t.name FROM media_annotation_files f " + "JOIN media_annotation_bytes b ON b.file_id=f.id " + "LEFT JOIN media_image_tags mit ON mit.byte_id=b.id " + "LEFT JOIN tags t ON t.id=mit.tag_id " + "ORDER BY f.locator,b.first_seen_at,b.id,mit.position" +) + + +def _image_tags_relevant(names, wanted, query): + if wanted and not wanted.isdisjoint(names): + return True + return bool(query) and any(query in name.casefold() for name in names) + + +def reference_candidates(db, wanted, query): + """Locators the old full stream admitted for (``wanted``, ``query``).""" + records = {} + for locator, byte_id, name in db.model.get_connection().execute(_REFERENCE_RECORDS): + names = records.setdefault((locator, byte_id), []) + if name is not None: + names.append(name) + return { + locator + for (locator, _byte), names in records.items() + if _image_tags_relevant(names, set(wanted), query) + } + + class TestWorkspaceImageTags(ImageAPITestCase): """B3: image tags in the Tags filter and search (spec amendment S1).""" @@ -2038,7 +2071,7 @@ class TestWorkspaceImageTags(ImageAPITestCase): leak = "no such table: media_annotation_files" with patch.object( self.api.db, - "iter_image_tag_records", + "image_tag_locators", side_effect=sqlite3.OperationalError(leak), ): # As a failed link read does under a Tags filter: a fixed coded @@ -2056,6 +2089,119 @@ class TestWorkspaceImageTags(ImageAPITestCase): self.assertEqual(self.names(body), ["dunes_by_name.png"]) self.assertIn("image tags", "\n".join(logged.output)) + def seed_differential(self): + """Mixed-case and underscore names, a stale record, an unlisted file.""" + a = self.media("a.png", mtime=101) + b = self.media("b.png", mtime=102) + c = self.media("c.png", mtime=103) + self.tag(a, ["Beach", "long_hair", "1girl"]) + self.tag(b, ["beach", "waves"]) + self.tag(c, ["Sunset"]) + # c's bytes change: its old record (Sunset) is stale, the new one current. + make_png(c, size=(5, 5)) + os.utime(c, (104, 104)) + self.tag(c, ["night_sky", "BEACH"]) + outside = Path(self._output_tmp.name) / "elsewhere" / "x.png" + outside.parent.mkdir(parents=True, exist_ok=True) + make_png(outside) + self.tag(outside, ["beach", "outside_only"]) + + async def test_indexed_candidates_equal_the_full_stream(self): + self.seed_differential() + cases = [ + (["beach"], ""), + (["Beach"], ""), + (["long_hair", "waves"], ""), + ([], "beach"), + ([], "hair"), + ([], "_"), + ([], "sun"), + (["nope"], "zzz"), + (["outside_only"], ""), + (["beach"], "night"), + ] + db = self.api.db + for wanted, query in cases: + with self.subTest(wanted=wanted, query=query): + indexed = set(db.image_tag_locators(db.image_tag_ids(wanted, query))) + self.assertEqual(indexed, reference_candidates(db, wanted, query)) + # The API answers are the indexed lookup's: exact names under Tags, a + # case-folded substring under q, current records only. + _, body = await self.get(tags="beach") + self.assertEqual(sorted(self.names(body)), ["b.png"]) + _, body = await self.get(q="beach") + self.assertEqual(sorted(self.names(body)), ["a.png", "b.png", "c.png"]) + _, body = await self.get(q="sunset") + self.assertEqual(self.names(body), []) + + async def test_tag_search_reads_image_tags_only_by_tag_id(self): + self.seed_differential() + statements = [] + original = self.api.db.model.get_connection + + def trace_connection(): + connection = original() + connection.set_trace_callback(statements.append) + return connection + + with patch.object( + self.api.db.model, "get_connection", side_effect=trace_connection + ): + for params in ({"tags": "beach"}, {"q": "hair"}): + response, _ = await self.get(**params) + self.assertEqual(response.status, 200) + reads = [ + sql + for sql in statements + if "media_image_tags" in sql and sql.lstrip().upper().startswith("SELECT") + ] + self.assertTrue(reads) + for sql in reads: + # Only a tag-id lookup, an EXISTS probe on the tag index, or the + # current records' own tags (byte_id IN) read the junction. + self.assertTrue( + "tag_id IN" in sql or "EXISTS" in sql or "byte_id IN" in sql, sql + ) + + @unittest.skipUnless(os.environ.get("PM_PERF") == "1", "timing run (PM_PERF=1)") + async def test_tag_search_timing(self): + """Linux timing for the PR body: 23,600 records, 10 tags each.""" + import hashlib + import statistics + import time + + db = self.api.db + epoch = db.get_execution_epoch() + names = [f"name_{n:04d}" for n in range(2000)] + root = os.path.realpath(self.output_dir) + for n in range(23_600): + path = self.output_dir / "perf" / f"{n:05d}.png" + path.parent.mkdir(exist_ok=True) + path.write_bytes(b"x") + os.utime(path, (n + 1, n + 1)) + stat = path.stat() + db.replace_image_tags( + locator=os.path.normcase(os.path.join(root, "perf", path.name)), + sha256=hashlib.sha256(str(n).encode()).hexdigest(), + size=stat.st_size, + mtime_ns=stat.st_mtime_ns, + expected_epoch=epoch, + expected_tag_revision=0, + tags=[names[(n * 7 + k) % 2000] for k in range(10)], + ) + await self.get(limit=1) # the listing, warm + for label, params in ( + ("one tag", {"tags": "name_0042"}), + ("text", {"q": "e_1"}), + ): + times = [] + for _ in range(5): + started = time.perf_counter() + response, _ = await self.get(limit=100, **params) + times.append(time.perf_counter() - started) + self.assertEqual(response.status, 200) + print(f"PM_PERF {label}: median {statistics.median(times) * 1000:.0f} ms") + async def test_legacy_output_failure_logs_its_traceback(self): self.media("legacy.png", mtime=100) with (