-
-
Notifications
You must be signed in to change notification settings - Fork 125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deduce model field names from custom prefetches #473
base: main
Are you sure you want to change the base?
Deduce model field names from custom prefetches #473
Conversation
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Type: Enhancement
PR Summary: The pull request introduces an enhancement to the QuerySet optimizer by enabling it to understand and optimize queries involving custom prefetch_related
configurations. This is achieved by deducing model field names from custom prefetches, allowing the optimizer to continue its optimization process beyond fields with custom prefetches, thereby addressing potential N+1 issues in nested relations.
Decision: Comment
📝 Type: 'Enhancement' - not supported yet.
- Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
✅ Small diff: the diff is small enough to approve with confidence.
No details provided.
General suggestions:
- Consider adding more inline documentation or comments explaining the rationale behind key changes, especially for complex logic around handling custom prefetches. This will aid future maintainability.
- Review the use of suppression comments like
# noqa: PLW2901
to ensure their necessity is clear, possibly by adding a brief explanation next to each suppression. - Ensure thorough testing of the new logic, particularly in scenarios with multiple custom prefetches that might lead to ambiguous model field deductions.
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
f_prefetch = Prefetch(path, queryset=f_qs) | ||
f_prefetch._optimizer_sentinel = _sentinel # type: ignore | ||
store.prefetch_related.append(f_prefetch) | ||
if custom_prefetches: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (llm): This block introduces a significant change in how prefetches are handled based on the presence of custom_prefetches
. It's a complex addition that could benefit from a bit more inline documentation to explain the rationale behind this approach, especially for future maintainers.
Some guidance on the test failures would be great. I am not 100% sure what the correct approach would be for the changed GraphQL schema. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, sorry for taking to long to review this. For some reason I started reviewing and stopped, had a some comments drafted...
Some guidance on the test failures would be great. I am not 100% sure what the correct approach would be for the changed GraphQL schema.
Most of the issues I'm seeing here are related to typing. Try to run pyright
locally and it will help you fix those. If you need specific guidance feel free to reach me on discord :)
There are some related to features not supported on older python versions. e.g. X | Y
cannot be used yet because we are targeting python 3.8+, Annotated
needs to be imported from typing_extensions
(it was introduced on 3.9), etc
About the schema, just run pytest with --snapshot-update
and it should update the schema file :)
b2799fc
to
16ff873
Compare
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #473 +/- ##
==========================================
+ Coverage 87.90% 88.21% +0.30%
==========================================
Files 37 37
Lines 3166 3215 +49
==========================================
+ Hits 2783 2836 +53
+ Misses 383 379 -4 ☔ View full report in Codecov by Sentry. |
…nto issue/389/optimizer-custom-pf
@bellini666 I think all the linting and type errors should be resolved now and the snapshot test fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The solution don't seem to be working for django 3.2 and 4.0 (see the tests that broke)
ProjectFeedItem = Annotated[ | ||
Union[IssueType, MilestoneType], strawberry.union("ProjectFeedItem") | ||
] | ||
|
||
|
||
@strawberry.type | ||
class ProjectFeedConnection(relay.Connection[ProjectFeedItem]): | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a left over from a test attempt? =P
This is not being used, so it should be removed
Description
Currently the QuerySet optimizer is oblivious to custom
prefetch_related
like the following:The current optimizer cannot understand how to optimize "through" this prefetch, because it cannot find the model field (
milestones
inProject
) that the field uses.This results in the optimization essentially stopping at the
next_milestones
field and any further nested relations will cause N+1 issues.This pull request attempts to address this shortcoming.
Types of Changes
Now the optimizer first inspects the field configuration to find any custom
Prefetch
. A custom prefetch is one which has a customto_attr
set. The optimizer then uses these custom Prefetches to perform further optimization instead of stopping here.Issues Fixed or Closed by This PR
Checklist