-
Notifications
You must be signed in to change notification settings - Fork 299
Remove not always supported auto select_related
#651
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
Remove not always supported auto select_related
#651
Conversation
Codecov Report
@@ Coverage Diff @@ ## master #651 +/- ## ======================================= Coverage 96.13% 96.13% ======================================= Files 56 56 Lines 2869 2869 ======================================= Hits 2758 2758 Misses 111 111
Continue to review full report at Codecov.
|
Probably we want Polymorpihc stuff make using custom select/prefetch related config, but not removing auto_select_related just because it is not supported by Polymorphic. But backwards compatibility suffers in that case.
First I thought it would be a good idea to automatically choose select_related but when thinking about it again in some cases it might only be a very small improvement but in some others it might be a major slow down all depending on the database structure as explained here
So all in all also for backwards compatibility it is better to remove it. And in some case if someone has a data structure which is faster with select_related they can now use select_for_includes.
Polymorphic models do not always support `select_related` as of https://django-polymorphic.readthedocs.io/en/stable/advanced.html?highlight=select_related#about-queryset-methods and might actually be slower in some cases as explained in https://django-rest-framework-json-api.readthedocs.io/en/stable/usage.html?highlight=usage#performance-improvements Hence reverting change of AutoPreloadMixin as it has not been released yet.
bd8ed3f to
512846e
Compare
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.
Sorry I forgot this was assigned to me. I'm a little iffy on this as I've no experience using the polymorphic code, but if @sliverc and @Anton-Shutik are OK with this (as indicated) then I approve.
Addresses testing issue in #643
Description of the Change
Polymorphic models do not always support select_related and might actually be slower.
Hence reverting change of
AutoPreloadMixinas it has not been released yet.Checklist
CHANGELOG.mdupdated (only for user relevant changes)AUTHORS