[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.