fix(tagging): Images tag search goes through the tag index (#379, PR 3) (#395)

* 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 * test(search): pin prompt-only tags and multi-chunk tag lookups (#379) Reads the tag names and the locators in the same annotation read helper.
2026-10-27 22:07:59 +00:00
parent d2ea705a44
commit 37e4e08327
6 changed files with 282 additions and 47 deletions
@@ -4380,8 +4380,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
@@ -110,12 +110,16 @@ _MARKED_LOCATORS = (
    f"SELECT DISTINCT f.locator FROM {AUTOTAG_TABLE} m "  # nosec B608
    f"JOIN {BYTES_TABLE} b ON b.id=m.byte_id JOIN {FILES_TABLE} f ON f.id=b.file_id"
 )
-_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}})"
 )


@@ -516,20 +520,32 @@ class AnnotationOperations:
        with self._annotation_read() as conn:
            return {row[0] for row in conn.execute(_MARKED_LOCATORS)}

-    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.
        """
-        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]
+        wanted = set(wanted)
+        with self._annotation_read() as conn:
+            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
@@ -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)
@@ -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]');
@@ -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,14 +553,26 @@ 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, and the empty
+        # set's file never.
        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]: []},
+        )
+
+    def test_tag_locators_cover_more_ids_than_one_chunk(self):
+        # A broad text search can match more than 500 names: every id's file
+        # comes back across the chunks.
+        locators = [self.locator(f"t{i:04}.png") for i in range(503)]
+        for i, loc in enumerate(locators):
+            self.replace(loc, tags=[f"name_{i:04}"])
+        ids = self.db.image_tag_ids([], "name_")
+        self.assertEqual(len(ids), 503)
+        self.assertEqual(self.db.image_tag_locators(ids), set(locators))


 class AutotagMarkTests(AnnotationTestCase):
@@ -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,140 @@ 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
+            )
+
+    async def test_the_image_tag_lookup_never_reads_prompt_tags(self):
+        self.seed_differential()
+        linked = self.media("linked.png", mtime=105)
+        prompt = self._save_prompt(
+            "Prompt with its own tag", tags=["prompt_only_beach"]
+        )
+        self.api.db.link_image_to_prompt(prompt, str(linked))
+        db = self.api.db
+        # The tag is only on a prompt: the image-tag lookup never yields it,
+        # under the Tags filter or the text search.
+        self.assertEqual(db.image_tag_ids(["prompt_only_beach"], ""), [])
+        self.assertEqual(db.image_tag_ids([], "prompt_only"), [])
+        self.assertEqual(
+            db.image_tag_locators(db.image_tag_ids([], "beach")),
+            reference_candidates(db, [], "beach"),
+        )
+        # The search still matches through the linked positive's prompt tags
+        # (the design keeps that, #379 "Behavior that does not change").
+        _, body = await self.get(q="prompt_only")
+        self.assertEqual(self.names(body), ["linked.png"])
+
+    @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 (