Merge pull request 'fix: distinguish a title collision from an attribution miss, and index for it' (#8) from fix/title-collisions-and-index into main

Reviewed-on: #8

README.md

@@ -110,17 +110,21 @@ unmatched by an artist the library holds: N pairs, M plays
 ```
 
 That is a track that was played by an artist the library holds. It is then
-split again, because owning an artist is a weak proxy for owning a track:
+split three ways, because owning an artist is a weak proxy for owning a track
+and a shared title is a weak proxy for a shared song:
 
-- **the title exists under another artist** — an attribution disagreement, a
+- **the library's own title credits the scrobbled artist** — `Voodoo People
-  remixer or a guest billed as the artist. These are the genuine misses, and
+  (Pendulum Remix)` against a play credited to Pendulum. Same song, filed
-  each is a candidate for being wrongly called cold in stage four. The report
+  under the original artist. These are the genuine misses.
-  names the artist the library files them under.
+- **the same title under an unrelated artist** — a collision, not a miss.
-- **the title is nowhere in the library** — never bought. No amount of matching
+  Across fifty thousand tracks these are constant: `Everyday` is Rusko and
-  conjures a file that does not exist.
+  also Def Leppard, `Kaleidoscope` is Delta Heavy and also Chappell Roan.
+  Matching on title alone would be far worse than missing them, which is why
+  there is no such tier.
+- **the title is nowhere in the library** — never bought.
 
-Counting both as matcher failures overstates the problem and would over-block
+Only the first is worth chasing. Counting all three as matcher failures
-the cull. The report lists the worst of each by play count.
+overstates the problem and would over-block the cull.
 
 ## How the ingest works
 

music_curator.py

@@ -141,6 +141,11 @@ CREATE TABLE IF NOT EXISTS lidarr_track (
 );
 CREATE INDEX IF NOT EXISTS lidarr_track_recording ON lidarr_track (recording_mbid);
 CREATE INDEX IF NOT EXISTS lidarr_track_norm ON lidarr_track (norm_artist, norm_title);
+-- Separate from the composite above, which a lookup by title alone cannot use:
+-- its leading column is the artist. The report searches by title on its own, and
+-- without this it scans every track for every unmatched key -- forty-seven
+-- seconds on a library of eighty-four thousand.
+CREATE INDEX IF NOT EXISTS lidarr_track_title ON lidarr_track (norm_title);
 CREATE INDEX IF NOT EXISTS lidarr_track_album ON lidarr_track (album_id);
 
 -- One row per distinct thing listened to, with the verdict on whether it could
@@ -1125,47 +1130,68 @@ def coverage_report(store):
     )
 
     # Owning an artist is a weak proxy for owning a track, so that figure alone
-    # overstates the matcher's failings. Split it. A title the library holds
+    # overstates the matcher's failings. Split it -- but not on the title alone.
-    # under some other artist is an attribution disagreement -- a remixer
+    # Across fifty thousand tracks, titles collide constantly: "Everyday" is
-    # credited as the artist, a guest billed as one -- and is a real miss. A
+    # Rusko and also Def Leppard, "Kaleidoscope" is Delta Heavy and also
-    # title the library does not hold at all was simply never bought, and no
+    # Chappell Roan. Matching those would be worse than missing them.
-    # amount of matching will conjure it.
+    #
+    # The signal for a genuine attribution miss is that the library's own title
+    # credits the artist the scrobble is filed under -- "Voodoo People (Pendulum
+    # Remix)" against a play credited to Pendulum. The normalised title has that
+    # suffix stripped, which is exactly what let them meet, so the raw one has to
+    # be searched for the name.
     attribution = store.connection.execute(
+        "SELECT COUNT(*) AS pairs, COALESCE(SUM(plays), 0) AS plays FROM scrobble_key k"
+        " WHERE k.track_id IS NULL"
+        "   AND EXISTS (SELECT 1 FROM lidarr_artist a WHERE a.norm_name = k.norm_artist)"
+        "   AND EXISTS (SELECT 1 FROM lidarr_track t"
+        "                WHERE t.norm_title = k.norm_track"
+        "                  AND instr(lower(t.title), lower(k.artist)) > 0)"
+    ).fetchone()
+    collision = store.connection.execute(
         "SELECT COUNT(*) AS pairs, COALESCE(SUM(plays), 0) AS plays FROM scrobble_key k"
         " WHERE k.track_id IS NULL"
         "   AND EXISTS (SELECT 1 FROM lidarr_artist a WHERE a.norm_name = k.norm_artist)"
         "   AND EXISTS (SELECT 1 FROM lidarr_track t WHERE t.norm_title = k.norm_track)"
     ).fetchone()
     logger.info(
-        "  of those, the title exists under another artist: %d pairs, %d plays"
+        "  the library's title credits the scrobbled artist: %d pairs, %d plays"
-        " -- attribution disagreements, and the genuine misses",
+        " -- remixes and guest spots, and the genuine misses",
         attribution["pairs"],
         attribution["plays"],
     )
+    logger.info(
+        "  same title under an unrelated artist: %d pairs, %d plays"
+        " -- title collisions, not misses; matching these would be a mistake",
+        collision["pairs"] - attribution["pairs"],
+        collision["plays"] - attribution["plays"],
+    )
     logger.info(
         "  the rest, %d pairs, %d plays: you own the artist but not the track",
-        suspect["pairs"] - attribution["pairs"],
+        suspect["pairs"] - collision["pairs"],
-        suspect["plays"] - attribution["plays"],
+        suspect["plays"] - collision["plays"],
     )
 
     mismatched = store.connection.execute(
         "SELECT k.artist, k.track, k.plays,"
-        "       (SELECT a.name FROM lidarr_track t"
+        "       (SELECT t.title FROM lidarr_track t"
-        "          JOIN lidarr_artist a ON a.id = t.artist_id"
+        "         WHERE t.norm_title = k.norm_track"
-        "         WHERE t.norm_title = k.norm_track LIMIT 1) AS filed_under"
+        "           AND instr(lower(t.title), lower(k.artist)) > 0 LIMIT 1) AS library_title"
         "  FROM scrobble_key k"
         " WHERE k.track_id IS NULL"
         "   AND EXISTS (SELECT 1 FROM lidarr_artist a WHERE a.norm_name = k.norm_artist)"
-        "   AND EXISTS (SELECT 1 FROM lidarr_track t WHERE t.norm_title = k.norm_track)"
+        "   AND EXISTS (SELECT 1 FROM lidarr_track t"
+        "                WHERE t.norm_title = k.norm_track"
+        "                  AND instr(lower(t.title), lower(k.artist)) > 0)"
         " ORDER BY k.plays DESC, k.artist LIMIT 10"
     ).fetchall()
     for position, row in enumerate(mismatched, start=1):
         logger.info(
-            "  attribution %2d: %-45s %4d plays, filed under %s",
+            "  attribution %2d: %-45s %4d plays, library has %r",
             position,
             f"{row['artist']} - {row['track']}"[:45],
             row["plays"],
-            row["filed_under"],
+            row["library_title"],
         )
 
     with_files = store.scalar("SELECT COUNT(*) FROM lidarr_track WHERE has_file = 1")

tests/test_music_curator.py

@@ -55,8 +55,22 @@ LIBRARY = [
     {
         "name": "The Prodigy",
         "albums": [
-            {"title": "The Fat of the Land", "tracks": [{"title": "Breathe (Remastered)"}]}
+            {
+                "title": "The Fat of the Land",
+                "tracks": [
+                    {"title": "Breathe (Remastered)"},
+                    # Credits its remixer in the title, which is the only signal
+                    # separating a real attribution miss from a title collision.
+                    {"title": "Voodoo People (Pendulum Remix)"},
                 ],
+            }
+        ],
+    },
+    # Held by the library in its own right, which is what puts its scrobbles
+    # inside the "artist the library holds" filter at all.
+    {
+        "name": "Pendulum",
+        "albums": [{"title": "Immersion", "tracks": [{"title": "Watercolour"}]}],
     },
 ]
 
@@ -393,7 +407,7 @@ def test_keys_carry_the_play_count_and_the_span(tmp_path):
 def test_re_indexing_drops_what_lidarr_no_longer_has(tmp_path):
     """The index is Lidarr's mirror, not an accumulation of everything ever seen."""
     store = indexed(tmp_path, [scrobble_of("AC/DC", "Hells Bells")])
-    assert store.scalar("SELECT COUNT(*) FROM lidarr_artist") == 3
+    assert store.scalar("SELECT COUNT(*) FROM lidarr_artist") == 4
 
     music_curator.index_library(
         music_curator.Lidarr("http://lidarr", "key", transport=FakeLidarr(LIBRARY[:1])), store
@@ -512,7 +526,7 @@ def test_albums_come_from_the_unfiltered_endpoint(tmp_path):
 
     album_calls = [query for path, query in api.calls if path == "album"]
     assert album_calls == [{}]
-    assert store.scalar("SELECT COUNT(*) FROM lidarr_album") == 3
+    assert store.scalar("SELECT COUNT(*) FROM lidarr_album") == 4
 
 
 def test_a_bad_album_falls_back_to_asking_per_artist(tmp_path):
@@ -526,7 +540,7 @@ def test_a_bad_album_falls_back_to_asking_per_artist(tmp_path):
 
     assert store.scalar("SELECT COUNT(*) FROM lidarr_album WHERE artist_id = 2") == 0
     # The other two artists keep their albums.
-    assert store.scalar("SELECT COUNT(*) FROM lidarr_album") == 2
+    assert store.scalar("SELECT COUNT(*) FROM lidarr_album") == 3
     assert store.get_state("index_albums_skipped") == "1"
 
 
@@ -549,10 +563,10 @@ def test_an_artist_lidarr_cannot_serve_does_not_kill_the_index(tmp_path):
 
     music_curator.index_library(music_curator.Lidarr("http://lidarr", "key", transport=api), store)
 
-    assert store.scalar("SELECT COUNT(*) FROM lidarr_artist") == 3
+    assert store.scalar("SELECT COUNT(*) FROM lidarr_artist") == 4
     assert store.scalar("SELECT COUNT(*) FROM lidarr_track WHERE artist_id = 2") == 0
     # The other two artists are indexed in full.
-    assert store.scalar("SELECT COUNT(*) FROM lidarr_track") == 3
+    assert store.scalar("SELECT COUNT(*) FROM lidarr_track") == 5
     assert store.get_state("index_skipped") == "1"
 
 
@@ -763,3 +777,51 @@ def test_the_report_survives_the_attribution_split(tmp_path):
     store = indexed(tmp_path, [scrobble_of("Yellowcard", "Hells Bells")])
 
     music_curator.report(store, NOW)
+
+
+def attribution_pairs(store):
+    """Unmatched pairs where the library's own title credits the scrobbled artist."""
+    return [
+        row["artist"]
+        for row in store.connection.execute(
+            "SELECT k.artist FROM scrobble_key k WHERE k.track_id IS NULL"
+            "   AND EXISTS (SELECT 1 FROM lidarr_artist a WHERE a.norm_name = k.norm_artist)"
+            "   AND EXISTS (SELECT 1 FROM lidarr_track t"
+            "                WHERE t.norm_title = k.norm_track"
+            "                  AND instr(lower(t.title), lower(k.artist)) > 0)"
+        )
+    ]
+
+
+def test_a_shared_title_is_not_an_attribution_miss(tmp_path):
+    """Across fifty thousand tracks, titles collide constantly: "Everyday" is
+    Rusko and also Def Leppard. Matching those would be worse than missing."""
+    store = indexed(tmp_path, [scrobble_of("Yellowcard", "Hells Bells")])
+
+    # The library holds "Hells Bells", by AC/DC, and its title says nothing
+    # about Yellowcard. A collision, not a miss.
+    assert verdict(store, "Yellowcard", "Hells Bells") == ("none", None)
+    assert attribution_pairs(store) == []
+
+
+def test_a_remix_credited_in_the_library_title_is_an_attribution_miss(tmp_path):
+    """The library has "Voodoo People (Pendulum Remix)" under The Prodigy; the
+    scrobble credits Pendulum. Same song, different filing."""
+    store = indexed(tmp_path, [scrobble_of("Pendulum", "Voodoo People")])
+
+    assert attribution_pairs(store) == ["Pendulum"]
+
+
+def test_the_title_index_is_used_for_the_report_lookup(tmp_path):
+    """Without it the report scans every track for every unmatched key: forty-
+    seven seconds on a real library."""
+    store = indexed(tmp_path, [])
+
+    plan = "\n".join(
+        row[-1]
+        for row in store.connection.execute(
+            "EXPLAIN QUERY PLAN SELECT 1 FROM scrobble_key k WHERE k.track_id IS NULL"
+            " AND EXISTS (SELECT 1 FROM lidarr_track t WHERE t.norm_title = k.norm_track)"
+        )
+    )
+    assert "lidarr_track_title" in plan, plan