diff --git a/docs/scrobble_rule_optimization.md b/docs/scrobble_rule_optimization.md new file mode 100644 index 00000000..597eecb9 --- /dev/null +++ b/docs/scrobble_rule_optimization.md @@ -0,0 +1,219 @@ +# Scrobble Rule Application Optimization + +## Overview + +This document describes the optimization made to the scrobble rule application system to improve performance when applying multiple rules. + +## Problem + +Previously, when applying all enabled scrobble rules, each rule was applied independently with a separate database UPDATE query. With N rules, this resulted in N separate database operations, which became inefficient as the number of rules grew. + +### Previous Implementation + +```elixir +def apply_all_rules do + list_enabled_rules() + |> Enum.map(fn rule -> + case apply_rule(rule) do + {:ok, count} -> {:ok, {rule.type, rule.match_value, count}} + {:error, reason} -> {:error, {rule.type, rule.match_value, reason}} + end + end) +end +``` + +This approach meant: +- 10 rules = 10 separate UPDATE queries +- 100 rules = 100 separate UPDATE queries +- Each query had to scan the entire `scrobbled_tracks` table + +## Solution + +The optimization groups rules by type (album/artist) and applies all rules of each type in a single database query using SQLite's CASE statement. + +### New Implementation + +```elixir +def apply_all_rules do + enabled_rules = list_enabled_rules() + + # Group rules by type + {album_rules, artist_rules} = + Enum.split_with(enabled_rules, fn rule -> rule.type == :album end) + + # Apply all album rules in one query + apply_all_album_rules(album_rules) + + # Apply all artist rules in one query + apply_all_artist_rules(artist_rules) +end +``` + +This approach means: +- 10 album rules + 10 artist rules = 2 total UPDATE queries (one for albums, one for artists) +- 100 album rules + 100 artist rules = 2 total UPDATE queries +- Each query still scans the table once, but updates all matching records in a single pass + +## Technical Details + +### SQL Generation + +The optimization dynamically builds a CASE statement for all rules of the same type: + +**Example for 2 album rules:** + +```sql +UPDATE scrobbled_tracks +SET album = CASE + WHEN json_extract(album, '$.title') = 'Dark Side of the Moon' + THEN json_set(album, '$.musicbrainz_id', '12345678-1234-1234-1234-123456789012') + WHEN json_extract(album, '$.title') = 'Wish You Were Here' + THEN json_set(album, '$.musicbrainz_id', 'abcdef12-3456-7890-abcd-ef1234567890') + ELSE album +END +WHERE json_extract(album, '$.title') IN ('Dark Side of the Moon', 'Wish You Were Here') +``` + +The WHERE clause uses IN to filter only tracks that match any of the rules, avoiding unnecessary updates. + +### Functions Added + +#### 1. `apply_all_album_rules/1` +Applies all album rules in a single query. + +```elixir +@spec apply_all_album_rules([ScrobbleRule.t()]) :: {:ok, non_neg_integer()} | {:error, any()} +``` + +#### 2. `apply_all_artist_rules/1` +Applies all artist rules in a single query. + +```elixir +@spec apply_all_artist_rules([ScrobbleRule.t()]) :: {:ok, non_neg_integer()} | {:error, any()} +``` + +#### 3. `apply_all_album_rules/2` +Applies all album rules to a specific set of tracks in a single query. + +```elixir +@spec apply_all_album_rules([ScrobbleRule.t()], [Track.t()]) :: + {:ok, non_neg_integer()} | {:error, any()} +``` + +#### 4. `apply_all_artist_rules/2` +Applies all artist rules to a specific set of tracks in a single query. + +```elixir +@spec apply_all_artist_rules([ScrobbleRule.t()], [Track.t()]) :: + {:ok, non_neg_integer()} | {:error, any()} +``` + +### Use Cases + +The optimization handles two main use cases: + +#### 1. Periodic Full Application +The `ApplyScrobbleRules` Oban worker runs every 30 minutes and applies all rules to all tracks: + +```elixir +ScrobbleRules.apply_all_rules() +``` + +#### 2. Incremental Application on New Tracks +When new tracks are inserted via `LastFm.Feed.update/1`, rules are applied to just those tracks: + +```elixir +tracks +|> ScrobbleRules.apply_all_rules() +``` + +Both use cases now benefit from batched rule application. + +## Performance Impact + +### Expected Improvements + +For a database with: +- 10,000 scrobbled tracks +- 20 enabled rules (10 album + 10 artist) + +**Before:** +- 20 UPDATE queries +- Each query scans ~10,000 rows +- Total: ~200,000 row scans + +**After:** +- 2 UPDATE queries (1 for albums, 1 for artists) +- Each query scans ~10,000 rows +- Total: ~20,000 row scans + +**Result:** ~10x reduction in database operations for this scenario. + +The benefit scales with the number of rules: +- 5 rules: ~2.5x improvement +- 10 rules: ~5x improvement +- 20 rules: ~10x improvement +- 100 rules: ~50x improvement + +## Trade-offs + +### Return Value Semantics + +With the batched approach, all rules of the same type report the same aggregate count (total rows updated) rather than individual per-rule counts: + +```elixir +# Before: Each rule gets its own count +[ + {:ok, {:album, "Album 1", 5}}, # 5 tracks updated + {:ok, {:album, "Album 2", 3}}, # 3 tracks updated +] + +# After: Both rules report the total updated +[ + {:ok, {:album, "Album 1", 8}}, # 8 total tracks updated + {:ok, {:album, "Album 2", 8}}, # 8 total tracks updated +] +``` + +This is an acceptable trade-off because: +1. The logging still shows how many rules were applied +2. The total tracks updated is more meaningful for understanding impact +3. Getting individual counts would require separate queries, negating the optimization + +### Backward Compatibility + +The API remains fully backward compatible: +- All existing functions (`apply_rule/1`, `apply_album_rule/1`, etc.) still work +- Return value format is unchanged: `{:ok, {type, match_value, count}}` +- Error handling is unchanged +- Tests continue to pass + +## Testing + +New tests were added to verify the optimization: + +1. `apply_all_album_rules/1` applies multiple album rules in one query +2. `apply_all_artist_rules/1` applies multiple artist rules in one query +3. Empty list handling returns `{:ok, 0}` +4. `apply_all_rules/0` correctly batches rules by type +5. All existing tests continue to pass + +## Future Considerations + +### Potential Enhancements + +1. **Parallel Execution**: Album and artist rule applications could run in parallel since they're independent +2. **Query Optimization**: Could add indexes on `json_extract(album, '$.title')` and `json_extract(artist, '$.name')` if performance is still a concern +3. **Metrics**: Add telemetry events to track batch sizes and execution times +4. **Rule Ordering**: If rule priority matters in the future, the CASE statement naturally handles this (first match wins) + +### Scalability + +The current approach scales well up to ~100 rules per type. Beyond that, consider: +- SQL query length limits (SQLite's default limit is 1MB, which allows ~10,000 rules) +- Transaction size and memory usage +- Potential chunking if needed (apply rules in batches of N) + +## Conclusion + +This optimization significantly improves the performance of scrobble rule application by reducing database operations from O(N) to O(1) per rule type, where N is the number of rules. The implementation maintains backward compatibility while providing substantial performance benefits that scale with the number of rules. diff --git a/lib/music_library/scrobble_rules.ex b/lib/music_library/scrobble_rules.ex index f612618b..2c579b02 100644 --- a/lib/music_library/scrobble_rules.ex +++ b/lib/music_library/scrobble_rules.ex @@ -326,9 +326,229 @@ defmodule MusicLibrary.ScrobbleRules do apply_artist_rule(rule, tracks) end + @doc """ + Applies all album rules in a single query. + + Uses a CASE statement to update the musicbrainz_id for all matching albums + in one database operation, which is more efficient than applying rules individually. + + ## Examples + + iex> apply_all_album_rules([rule1, rule2]) + {:ok, 15} + + """ + def apply_all_album_rules([]), do: {:ok, 0} + + def apply_all_album_rules(rules) when is_list(rules) do + # Build CASE WHEN clauses dynamically + {case_clauses, case_params} = + rules + |> Enum.reduce({"", []}, fn rule, {sql_acc, params_acc} -> + clause = + "WHEN json_extract(album, '$.title') = ? THEN json_set(album, '$.musicbrainz_id', ?) " + + {sql_acc <> clause, params_acc ++ [rule.match_value, rule.target_musicbrainz_id]} + end) + + # Build complete UPDATE statement + case_sql = "CASE #{case_clauses}ELSE album END" + + # Build WHERE IN clause + match_values = Enum.map(rules, & &1.match_value) + in_placeholders = Enum.map_join(match_values, ", ", fn _ -> "?" end) + where_sql = "json_extract(album, '$.title') IN (#{in_placeholders})" + + # Complete SQL + sql = """ + UPDATE scrobbled_tracks + SET album = #{case_sql} + WHERE #{where_sql} + """ + + # All parameters: case params + where params + all_params = case_params ++ match_values + + # Execute the query + case Repo.query(sql, all_params) do + {:ok, %{num_rows: count}} -> {:ok, count} + {:error, reason} -> {:error, reason} + end + end + + @doc """ + Applies all album rules to a specific set of tracks in a single query. + + Uses a CASE statement to update the musicbrainz_id for all matching albums + in one database operation, filtering by the provided tracks. + + ## Examples + + iex> apply_all_album_rules([rule1, rule2], tracks) + {:ok, 3} + + """ + def apply_all_album_rules([], _tracks), do: {:ok, 0} + def apply_all_album_rules(_rules, []), do: {:ok, 0} + + def apply_all_album_rules(rules, tracks) when is_list(rules) and is_list(tracks) do + # Build CASE WHEN clauses dynamically + {case_clauses, case_params} = + rules + |> Enum.reduce({"", []}, fn rule, {sql_acc, params_acc} -> + clause = + "WHEN json_extract(album, '$.title') = ? THEN json_set(album, '$.musicbrainz_id', ?) " + + {sql_acc <> clause, params_acc ++ [rule.match_value, rule.target_musicbrainz_id]} + end) + + # Build complete UPDATE statement + case_sql = "CASE #{case_clauses}ELSE album END" + + # Build WHERE IN clause for album titles + match_values = Enum.map(rules, & &1.match_value) + album_placeholders = Enum.map_join(match_values, ", ", fn _ -> "?" end) + + # Build WHERE IN clause for track timestamps + track_scrobbled_at_uts = Enum.map(tracks, & &1.scrobbled_at_uts) + track_placeholders = Enum.map_join(track_scrobbled_at_uts, ", ", fn _ -> "?" end) + + where_sql = + "json_extract(album, '$.title') IN (#{album_placeholders}) AND " <> + "scrobbled_at_uts IN (#{track_placeholders})" + + # Complete SQL + sql = """ + UPDATE scrobbled_tracks + SET album = #{case_sql} + WHERE #{where_sql} + """ + + # All parameters: case params + album match values + track timestamps + all_params = case_params ++ match_values ++ track_scrobbled_at_uts + + # Execute the query + case Repo.query(sql, all_params) do + {:ok, %{num_rows: count}} -> {:ok, count} + {:error, reason} -> {:error, reason} + end + end + + @doc """ + Applies all artist rules in a single query. + + Uses a CASE statement to update the musicbrainz_id for all matching artists + in one database operation, which is more efficient than applying rules individually. + + ## Examples + + iex> apply_all_artist_rules([rule1, rule2]) + {:ok, 25} + + """ + def apply_all_artist_rules([]), do: {:ok, 0} + + def apply_all_artist_rules(rules) when is_list(rules) do + # Build CASE WHEN clauses dynamically + {case_clauses, case_params} = + rules + |> Enum.reduce({"", []}, fn rule, {sql_acc, params_acc} -> + clause = + "WHEN json_extract(artist, '$.name') = ? THEN json_set(artist, '$.musicbrainz_id', ?) " + + {sql_acc <> clause, params_acc ++ [rule.match_value, rule.target_musicbrainz_id]} + end) + + # Build complete UPDATE statement + case_sql = "CASE #{case_clauses}ELSE artist END" + + # Build WHERE IN clause + match_values = Enum.map(rules, & &1.match_value) + in_placeholders = Enum.map_join(match_values, ", ", fn _ -> "?" end) + where_sql = "json_extract(artist, '$.name') IN (#{in_placeholders})" + + # Complete SQL + sql = """ + UPDATE scrobbled_tracks + SET artist = #{case_sql} + WHERE #{where_sql} + """ + + # All parameters: case params + where params + all_params = case_params ++ match_values + + # Execute the query + case Repo.query(sql, all_params) do + {:ok, %{num_rows: count}} -> {:ok, count} + {:error, reason} -> {:error, reason} + end + end + + @doc """ + Applies all artist rules to a specific set of tracks in a single query. + + Uses a CASE statement to update the musicbrainz_id for all matching artists + in one database operation, filtering by the provided tracks. + + ## Examples + + iex> apply_all_artist_rules([rule1, rule2], tracks) + {:ok, 7} + + """ + def apply_all_artist_rules([], _tracks), do: {:ok, 0} + def apply_all_artist_rules(_rules, []), do: {:ok, 0} + + def apply_all_artist_rules(rules, tracks) when is_list(rules) and is_list(tracks) do + # Build CASE WHEN clauses dynamically + {case_clauses, case_params} = + rules + |> Enum.reduce({"", []}, fn rule, {sql_acc, params_acc} -> + clause = + "WHEN json_extract(artist, '$.name') = ? THEN json_set(artist, '$.musicbrainz_id', ?) " + + {sql_acc <> clause, params_acc ++ [rule.match_value, rule.target_musicbrainz_id]} + end) + + # Build complete UPDATE statement + case_sql = "CASE #{case_clauses}ELSE artist END" + + # Build WHERE IN clause for artist names + match_values = Enum.map(rules, & &1.match_value) + artist_placeholders = Enum.map_join(match_values, ", ", fn _ -> "?" end) + + # Build WHERE IN clause for track timestamps + track_scrobbled_at_uts = Enum.map(tracks, & &1.scrobbled_at_uts) + track_placeholders = Enum.map_join(track_scrobbled_at_uts, ", ", fn _ -> "?" end) + + where_sql = + "json_extract(artist, '$.name') IN (#{artist_placeholders}) AND " <> + "scrobbled_at_uts IN (#{track_placeholders})" + + # Complete SQL + sql = """ + UPDATE scrobbled_tracks + SET artist = #{case_sql} + WHERE #{where_sql} + """ + + # All parameters: case params + artist match values + track timestamps + all_params = case_params ++ match_values ++ track_scrobbled_at_uts + + # Execute the query + case Repo.query(sql, all_params) do + {:ok, %{num_rows: count}} -> {:ok, count} + {:error, reason} -> {:error, reason} + end + end + @doc """ Applies all enabled rules. + This optimized version groups rules by type and applies all rules of each type + in a single database query, which is much more efficient than applying each rule + individually. + ## Examples iex> apply_all_rules() @@ -337,14 +557,45 @@ defmodule MusicLibrary.ScrobbleRules do """ def apply_all_rules do :telemetry.span([:music_library, :scrobble_rules, :apply_all_rules], %{}, fn -> - result = - list_enabled_rules() - |> Enum.map(fn rule -> - case apply_rule(rule) do - {:ok, count} -> {:ok, {rule.type, rule.match_value, count}} - {:error, reason} -> {:error, {rule.type, rule.match_value, reason}} - end - end) + enabled_rules = list_enabled_rules() + + # Group rules by type + {album_rules, artist_rules} = + Enum.split_with(enabled_rules, fn rule -> rule.type == :album end) + + # Apply all album rules in one query + album_result = + case apply_all_album_rules(album_rules) do + {:ok, count} -> + # Return the count for each album rule (total updated) + # Note: this returns the same count for each rule since they're applied together + Enum.map(album_rules, fn rule -> + {:ok, {rule.type, rule.match_value, count}} + end) + + {:error, reason} -> + Enum.map(album_rules, fn rule -> + {:error, {rule.type, rule.match_value, reason}} + end) + end + + # Apply all artist rules in one query + artist_result = + case apply_all_artist_rules(artist_rules) do + {:ok, count} -> + # Return the count for each artist rule (total updated) + # Note: this returns the same count for each rule since they're applied together + Enum.map(artist_rules, fn rule -> + {:ok, {rule.type, rule.match_value, count}} + end) + + {:error, reason} -> + Enum.map(artist_rules, fn rule -> + {:error, {rule.type, rule.match_value, reason}} + end) + end + + result = album_result ++ artist_result {result, %{scrobble_track_count: :all}} end) @@ -353,6 +604,9 @@ defmodule MusicLibrary.ScrobbleRules do @doc """ Applies all enabled rules to a specific set of tracks. + This optimized version groups rules by type and applies all rules of each type + in a single database query, filtering by the provided tracks. + ## Examples iex> apply_all_rules(tracks) @@ -368,14 +622,41 @@ defmodule MusicLibrary.ScrobbleRules do def apply_all_rules(tracks) do :telemetry.span([:music_library, :scrobble_rules, :apply_all_rules], %{}, fn -> - result = - list_enabled_rules() - |> Enum.map(fn rule -> - case apply_rule(rule, tracks) do - {:ok, count} -> {:ok, {rule.type, rule.match_value, count}} - {:error, reason} -> {:error, {rule.type, rule.match_value, reason}} - end - end) + enabled_rules = list_enabled_rules() + + # Group rules by type + {album_rules, artist_rules} = + Enum.split_with(enabled_rules, fn rule -> rule.type == :album end) + + # Apply all album rules in one query + album_result = + case apply_all_album_rules(album_rules, tracks) do + {:ok, count} -> + Enum.map(album_rules, fn rule -> + {:ok, {rule.type, rule.match_value, count}} + end) + + {:error, reason} -> + Enum.map(album_rules, fn rule -> + {:error, {rule.type, rule.match_value, reason}} + end) + end + + # Apply all artist rules in one query + artist_result = + case apply_all_artist_rules(artist_rules, tracks) do + {:ok, count} -> + Enum.map(artist_rules, fn rule -> + {:ok, {rule.type, rule.match_value, count}} + end) + + {:error, reason} -> + Enum.map(artist_rules, fn rule -> + {:error, {rule.type, rule.match_value, reason}} + end) + end + + result = album_result ++ artist_result {result, %{scrobble_track_count: Enum.count(tracks)}} end) diff --git a/test/music_library/scrobble_rules_test.exs b/test/music_library/scrobble_rules_test.exs index 84145978..fca40518 100644 --- a/test/music_library/scrobble_rules_test.exs +++ b/test/music_library/scrobble_rules_test.exs @@ -287,5 +287,141 @@ defmodule MusicLibrary.ScrobbleRulesTest do assert ScrobbleRules.count_artist_matches(rule) == 2 end + + test "apply_all_album_rules/1 applies multiple album rules in one query" do + # Create two album rules + rule1 = scrobble_rule_fixture(@valid_album_attrs) + + rule2 = + scrobble_rule_fixture(%{ + match_value: "Wish You Were Here", + target_musicbrainz_id: "abcdef12-3456-7890-abcd-ef1234567890" + }) + + # Create tracks matching each rule + track1 = + scrobbled_track_fixture(%{ + album: %{musicbrainz_id: "", title: "Dark Side of the Moon"} + }) + + track2 = + scrobbled_track_fixture(%{ + scrobbled_at_uts: System.system_time(:second) + 1, + album: %{musicbrainz_id: "", title: "Wish You Were Here"} + }) + + # Create a non-matching track + _track3 = + scrobbled_track_fixture(%{ + scrobbled_at_uts: System.system_time(:second) + 2, + album: %{musicbrainz_id: "", title: "The Wall"} + }) + + # Apply both rules at once + assert {:ok, 2} = ScrobbleRules.apply_all_album_rules([rule1, rule2]) + + # Verify both tracks were updated by fetching them again + updated_track1 = Repo.get(Track, track1.scrobbled_at_uts) + assert updated_track1.album.musicbrainz_id == rule1.target_musicbrainz_id + + updated_track2 = Repo.get(Track, track2.scrobbled_at_uts) + assert updated_track2.album.musicbrainz_id == rule2.target_musicbrainz_id + end + + test "apply_all_artist_rules/1 applies multiple artist rules in one query" do + # Create two artist rules + rule1 = scrobble_rule_fixture(@valid_artist_attrs) + + rule2 = + scrobble_rule_fixture(%{ + type: :artist, + match_value: "Led Zeppelin", + target_musicbrainz_id: "fedcba98-7654-3210-fedc-ba9876543210" + }) + + # Create tracks matching each rule + track1 = + scrobbled_track_fixture(%{ + artist: %{musicbrainz_id: "", name: "Pink Floyd"} + }) + + track2 = + scrobbled_track_fixture(%{ + scrobbled_at_uts: System.system_time(:second) + 1, + artist: %{musicbrainz_id: "", name: "Led Zeppelin"} + }) + + # Create a non-matching track + _track3 = + scrobbled_track_fixture(%{ + scrobbled_at_uts: System.system_time(:second) + 2, + artist: %{musicbrainz_id: "", name: "The Beatles"} + }) + + # Apply both rules at once + assert {:ok, 2} = ScrobbleRules.apply_all_artist_rules([rule1, rule2]) + + # Verify both tracks were updated by fetching them again + updated_track1 = Repo.get(Track, track1.scrobbled_at_uts) + assert updated_track1.artist.musicbrainz_id == rule1.target_musicbrainz_id + + updated_track2 = Repo.get(Track, track2.scrobbled_at_uts) + assert updated_track2.artist.musicbrainz_id == rule2.target_musicbrainz_id + end + + test "apply_all_album_rules/1 with empty list returns 0" do + assert {:ok, 0} = ScrobbleRules.apply_all_album_rules([]) + end + + test "apply_all_artist_rules/1 with empty list returns 0" do + assert {:ok, 0} = ScrobbleRules.apply_all_artist_rules([]) + end + + test "apply_all_rules/0 batches rules by type" do + # Create multiple rules of each type + _album_rule1 = scrobble_rule_fixture(@valid_album_attrs) + + _album_rule2 = + scrobble_rule_fixture(%{ + match_value: "Wish You Were Here", + target_musicbrainz_id: "abcdef12-3456-7890-abcd-ef1234567890" + }) + + _artist_rule1 = scrobble_rule_fixture(@valid_artist_attrs) + + _artist_rule2 = + scrobble_rule_fixture(%{ + type: :artist, + match_value: "Led Zeppelin", + target_musicbrainz_id: "fedcba98-7654-3210-fedc-ba9876543210" + }) + + # Create tracks matching the rules + _track1 = + scrobbled_track_fixture(%{ + album: %{musicbrainz_id: "", title: "Dark Side of the Moon"}, + artist: %{musicbrainz_id: "", name: "Pink Floyd"} + }) + + _track2 = + scrobbled_track_fixture(%{ + scrobbled_at_uts: System.system_time(:second) + 1, + album: %{musicbrainz_id: "", title: "Wish You Were Here"}, + artist: %{musicbrainz_id: "", name: "Led Zeppelin"} + }) + + # Apply all rules + results = ScrobbleRules.apply_all_rules() + + # Should have 4 results (one for each rule) + assert length(results) == 4 + + # All results should be successful + Enum.each(results, fn result -> + assert {:ok, {_type, _match_value, count}} = result + # Count should be > 0 since we have matching tracks + assert count > 0 + end) + end end end