Skip to content

fix: Song検索のパフォーマンス改善#889

Merged
izumin2000 merged 2 commits intomainfrom
feature/850-search-performance
Apr 5, 2026
Merged

fix: Song検索のパフォーマンス改善#889
izumin2000 merged 2 commits intomainfrom
feature/850-search-performance

Conversation

@izumin2000
Copy link
Copy Markdown
Owner

Summary

  • prefetch_related('authors') を追加し、song_card レンダリング時の song.authors.all() N+1クエリ(50曲/ページ)を解消
  • distinct() パスで新規 queryset を作成する際に prefetch_related('links', 'authors') を維持(author/keyword/url 等のフィルター使用時に links N+1 が発生していた問題を修正)
  • filter_by_keyword の links JOIN を :// を含むURLキーワードのみに限定(PR Feature/756 songlink db #775 で無条件化されたことで全keyword検索に不要なM2M JOINが走っていた)

Test plan

  • sort=-post_time&page=1 でデフォルトの一覧表示が正常に動作すること
  • keyword=xxx でキーワード検索が正常に動作すること
  • keyword=https://youtu.be/xxx でURL検索が正常に動作すること
  • author=xxx / url=xxx / mediatypes=youtube などフィルター使用時に song_card が正常に表示されること
  • 本番環境でのクエリ数・レスポンスタイムが改善されること

🤖 Generated with Claude Code

- prefetch_related('authors')を追加しauthorsのN+1を解消
- distinct()パス後もprefetch_related('links', 'authors')を維持
- filter_by_keywordのlinks JOIN をURL系キーワードのみに限定

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@izumin2000 izumin2000 linked an issue Apr 5, 2026 that may be closed by this pull request
@claude
Copy link
Copy Markdown

claude bot commented Apr 5, 2026

PR Review: Song検索のパフォーマンス改善

全体的に意図が明確でよい改善です。いくつかフィードバックを残します。


良い点

  • N+1の修正が的確: デフォルトパス・distinctパス両方に prefetch_related('authors') を追加しているのは正しいアプローチです。
  • JOINの最適化: filter_by_keyword でURL検索条件を :// 含む場合のみに限定したことで、全キーワード検索に不要なM2M JOINが走っていた問題が解消されています。

気になる点

1. clean_url の挙動次第でURL検索が機能しない可能性

clean_url がスキーム(https://)を除去する実装であれば、チェック if '://' in url_keyword が常にFalseになり、URL検索が一切走らなくなります。clean_url の実装を確認し、必要であれば元の keyword に対してチェックを行うよう修正してください。

# もし clean_url がスキームを除去するなら:
if '://' in keyword:
    url_keyword = clean_url(keyword)
    q |= Q(links__url__icontains=url_keyword)

2. :// なしの部分URLは検索不可になった

以前は youtu.be/xxx のような部分URLもリンク検索にヒットしていましたが、今回の変更で非ヒットになります。意図的な変更であればOKですが、ユーザー体験の変化として明示しておくとよいと思います。

3. distinct パスの prefetch_related について

song_search.pySong.objects.prefetch_related('links', 'authors') を渡し、song_filterset.pyqs プロパティで distinct パスに入ると Song.objects.prefetch_related('links', 'authors').filter(...) で新規 queryset を作成しています。Djangoは Song.objects.filter(...) から作った queryset は prefetch を引き継がないので正しい実装ですが、コードを読む際に混乱しやすいです。コメントで補足するとよいかもしれません。


テストについて

テストコードの追加がありません。特に以下のケースをカバーするテストが今後あると安心です:

  • 通常キーワード検索で links テーブルへの JOIN が発生しないこと
  • :// を含むURLキーワードで links 検索が実行されること
  • distinct パスでも authors が N+1 なく取得されること

総じて方向性は正しく、本番でのパフォーマンス改善が期待できます。clean_url の挙動確認だけ念のためお願いします。

- prefetch_related('authors')を追加しauthorsのN+1を解消
- distinct()パス後もprefetch_related('links', 'authors')を維持し
  フィルター使用時のlinks N+1を解消(コメントで意図を補足)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude bot commented Apr 5, 2026

Code Review

Summary

This PR addresses real N+1 query problems. The fixes are correct and the added comment in song_filterset.py clearly explains the non-obvious behavior. Overall a solid change with a few things worth discussing.


Discrepancy: PR summary vs. actual diff

The summary mentions a third fix:

filter_by_keyword の links JOIN を :// を含むURLキーワードのみに限定

But query_filters.py is not changed in this PR. The current code still unconditionally includes Q(links__url__icontains=url_keyword):

def filter_by_keyword(keyword):
    url_keyword = clean_url(keyword)
    return (
        Q(title__contains=keyword) |
        Q(authors__name__contains=keyword) |
        Q(lyrics__contains=keyword) |
        Q(links__url__icontains=url_keyword)  # always JOINs links
    )

Either this fix was intentionally deferred or was accidentally left out. Worth clarifying — if deferred, it should be tracked as a follow-up issue since it is a real performance concern for non-URL keyword searches.


Code quality

song_search.py — straightforward and correct.

song_filterset.py — the fix is correct, but the prefetch_related list is now hardcoded in two places: song_search.py and the qs property distinct branch. If a future caller passes a queryset with different/additional prefetches, the distinct branch silently drops them.

A minor improvement to make it resilient:

# Carry over from the existing queryset instead of hardcoding:
queryset = Song.objects.prefetch_related(*queryset._prefetch_related_lookups).filter(id__in=Subquery(ids))

Optional for now, but worth keeping in mind if more call sites are added.


Pre-existing issue (not introduced here, but worth noting)

In the distinct branch, sorting is only re-applied for sorts in DISTINCT_SORT_MAP (random, author, -author). Sorts like post_time, view, etc. lose their ordering when the new queryset is created, which could silently produce wrong page ordering for those sort keys.


Test coverage

The test plan is manual-only. Automated regression tests using assertNumQueries would prevent N+1 regressions from being reintroduced. Not a blocker, but a good follow-up.


Bottom line

The two actual code changes (prefetch_related('authors') added in both files) are correct and safe. The comment is clear and helpful. Main action items:

  1. Clarify whether the filter_by_keyword URL-only JOIN optimization was intentionally omitted from this PR
  2. Consider adding automated query count assertions for these paths

@izumin2000 izumin2000 merged commit 93840dd into main Apr 5, 2026
1 check passed
@izumin2000 izumin2000 deleted the feature/850-search-performance branch April 5, 2026 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

検索ページのパフォーマンス調査

1 participant