[GrandComicsDatabase/gcd-django] Fix/advanced story credit search (PR #728)
Josef Andersson
A proposal of further cleaning up the area as mentioned in #727, as proposed by Jochen (The six story-credit fields each carried a near-copy of the same logic).
This merges them into one path, fixing three inconsistencies on the way:
- "linked credits only" returned text credits for creators with no linked
credit yet - If that is choosen I guess we dont' want text credits results - Searching a bare ";" matched stories. Did not seem intentional, and buggish.
- "A; B" under "both" ignored B when B had no linked credit. Same,
And adds more tests, which is really the most of the PR. I hope I got my understanding right of how it should work there ( I understand there is road ahead of converting text credits to linked ones over time).
You can view, comment on, or merge this pull request online at:
https://github.com/GrandComicsDatabase/gcd-django/pull/728
Commit Summary
- cea631c fix(gcd): unify story credit search and honour linked-credits-only
- 594c182 test(gcd): cover advanced search credit behaviour
File Changes
(4 files)
- M apps/gcd/tests/test_search.py (52)
- A apps/gcd/tests/test_search_credit_sources.py (131)
- A apps/gcd/tests/test_search_linked_only.py (134)
- M apps/gcd/views/search.py (85)
Patch Links:
- https://github.com/GrandComicsDatabase/gcd-django/pull/728.patch
- https://github.com/GrandComicsDatabase/gcd-django/pull/728.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
gemini-code-assist[bot]
@gemini-code-assist[bot] commented on this pull request.
Code Review
This pull request refactors the story credit search logic in apps/gcd/views/search.py by extracting helper functions _linked_story_ids and _linked_story_credit_filters to simplify the querying of linked credits. It also adds comprehensive test coverage for various credit search scenarios, including linked versus text credits, deleted credits, and semicolon-separated creators. The feedback suggests a performance optimization in the database query within _linked_story_ids to avoid unnecessary joins and leverage subqueries by directly referencing foreign key IDs.
> + return list(Story.objects.filter(
+ credits__creator__id__in=creators,
+ credits__deleted=False,
+ credits__credit_type__id=CREDIT_TYPES[credit_field])
+ .values_list('id', flat=True))
Performance Optimization
We can optimize this query to avoid unnecessary database joins and reduce the number of queries executed:
- Avoid Unnecessary Joins: By using
credits__creator_id__inandcredits__credit_type_id, we directly query the foreign key columns (creator_idandcredit_type_id) on theStoryCredittable. This avoids joining theCreatorNameDetailandCreditTypetables in the SQL query. - Leverage Subqueries: Since
creatorsis a lazy QuerySet, passing it tocredits__creator_id__inallows Django to compile it as a single SQL query with a subquery, rather than executing two separate database queries.
- return list(Story.objects.filter(
- credits__creator__id__in=creators,
- credits__deleted=False,
- credits__credit_type__id=CREDIT_TYPES[credit_field])
- .values_list('id', flat=True))
+ return list(Story.objects.filter(
+ credits__creator_id__in=creators,
+ credits__deleted=False,
+ credits__credit_type_id=CREDIT_TYPES[credit_field])
+ .values_list('id', flat=True))
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Josef Andersson
janderssonse left a comment (GrandComicsDatabase/gcd-django#728)2 After fix
adv-2b-after-semicolon-empty.png (view on web) adv-3a-before-multicreator-drops-absent.png (view on web)3 After fix
adv-3b-after-multicreator-empty.png (view on web)—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Josef Andersson
@janderssonse commented on this pull request.
> + return list(Story.objects.filter(
+ credits__creator__id__in=creators,
+ credits__deleted=False,
+ credits__credit_type__id=CREDIT_TYPES[credit_field])
+ .values_list('id', flat=True))
Checked the generated SQL both ways and it's identical. Django
already compiles credits__creator__id/credit_type__id to the raw
creator_id/credit_type_id columns, so no join to CreatorNameDetail or
CreditType is added and creators is already a single subquery. Same query
plan either way, so I'll leave the current form.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JochenGCD
@jochengcd commented on this pull request.
Thanks for picking up on my comment.
> @@ -2216,6 +2216,35 @@ def handle_numbers(field, data, prefix):
return reduce(lambda x, y: x | y, q_or_only)
+def _linked_story_ids(creator, credit_field, op):
+ creator_q_obj = Q(**{'name__%s' % op: creator})
+ creator_q_obj |= Q(**{
+ 'creator__gcd_official_name__%s' % op: creator,
+ })
+ creators = CreatorNameDetail.objects.filter(creator_q_obj).values_list(
I think I had a reason for using a list here. If not, I think it stays a QuerySet and is lazy evaluated, which can be a large time burden on some queries. Since you know how to check the SQL, maybe see if there is a difference.
> + credits__credit_type__id=CREDIT_TYPES[credit_field])
+ .values_list('id', flat=True))
+
+
+def _linked_story_credit_filters(search_value, credit_field, prefix, op):
+ creator_names = [creator.strip()
+ for creator in search_value.split(';')
+ if creator.strip()]
+ if not creator_names:
+ return [Q(**{'%sid__in' % prefix: [-1]})]
+
+ filters = []
+ for creator in creator_names:
+ # Keep unmatched terms represented so AND searches cannot drop them.
+ stories = _linked_story_ids(creator, credit_field, op) or [-1]
+ filters.append(Q(**{'%sid__in' % prefix: stories}))
Before we filtered out if no stories were found ?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Josef Andersson
@janderssonse pushed 2 commits.
- 25439b1 fix(gcd): unify story credit search and honour linked-credits-only
- d7af3e8 test(gcd): cover advanced search credit behaviour
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Josef Andersson
@janderssonse commented on this pull request.
> @@ -2216,6 +2216,35 @@ def handle_numbers(field, data, prefix):
return reduce(lambda x, y: x | y, q_or_only)
+def _linked_story_ids(creator, credit_field, op):
+ creator_q_obj = Q(**{'name__%s' % op: creator})
+ creator_q_obj |= Q(**{
+ 'creator__gcd_official_name__%s' % op: creator,
+ })
+ creators = CreatorNameDetail.objects.filter(creator_q_obj).values_list(
Yes, thanks, you are completely right, I verified in docs and real life test witch docker now. The Django docs warn about exactly this for MySQL named specifically: https://docs.djangoproject.com/en/5.2/ref/models/querysets/#nested-queries-performance - "Some database backends, most notably MySQL, don’t optimize nested queries very well. It is more efficient, in those cases, to extract a list of values and then pass that into the second query. That is, execute two queries instead of one:"
Restored the list(), left a comment with the link, and added a test that fails if it gets dropped again. Rebased
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Josef Andersson
> + credits__credit_type__id=CREDIT_TYPES[credit_field])
+ .values_list('id', flat=True))
+
+
+def _linked_story_credit_filters(search_value, credit_field, prefix, op):
+ creator_names = [creator.strip()
+ for creator in search_value.split(';')
+ if creator.strip()]
+ if not creator_names:
+ return [Q(**{'%sid__in' % prefix: [-1]})]
+
+ filters = []
+ for creator in creator_names:
+ # Keep unmatched terms represented so AND searches cannot drop them.
+ stories = _linked_story_ids(creator, credit_field, op) or [-1]
+ filters.append(Q(**{'%sid__in' % prefix: stories}))
The code built one filter per searched name, but skipped the filter when a name had no linked credits. Skipping a filter meant "don't restrict at all", as if that name had never been typed. So:
- "linked credits only" for an unlinked creator: the linked filter disappeared and one got text-credit results instead of an empty list; see image adv-1a-before-linkedonly-returns-text vs adv-1b-after-linkedonly-empty in comment above
- "A; B": if B wasn't linked, B's filter disappeared and you got all of A's stories, as if you'd only searched A; see
adv-3a-before-multicreator-drops-absent vs adv-3b-after-multicreator-empty
The or [-1] gives those names a filter that can never match (no story has id -1), so "nothing found for this name" now really means "no results". "Both" mode still returns text matches like before.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JochenGCD
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.