Skip to content
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

select_related and prefetch_related for inherited models #198

Open
eriktelepovsky opened this issue Feb 12, 2016 · 25 comments · May be fixed by #545
Open

select_related and prefetch_related for inherited models #198

eriktelepovsky opened this issue Feb 12, 2016 · 25 comments · May be fixed by #545

Comments

@eriktelepovsky
Copy link

Regarding the documentation:

select_related() works just as usual, but it can not (yet) be used to select relations in inherited models (like ModelA.objects.select_related('ModelC___fieldxy') )

Is there a chance you will implement this? I mean select_related and prefetch_related for the inherited models, It would be really helpful. Because I don't know how to optimize some requests now. Thank you.

@vdboor
Copy link
Collaborator

vdboor commented May 4, 2016

Thanks for the report! You've identified a spot where polymorphic isn't present yet. I'm not sure this can be solved; the .filter(ModelC___fieldxy=..) already is somewhat of a hack. I'm not sure we can hook into the select-related logic but you're welcome to give it a try - granted that you'l add unit tests for it.

also see #37 for a related issue.

@gregmuellegger
Copy link

gregmuellegger commented Jul 22, 2016

I played around with select related in the child models, and figured I need to create a manager for the base_objects attribute on the child models, that performs a select_related for every queryset, like:

class ChildManager(models.Manager):
  def get_queryset(self):
    return super(ChildManager, self).get_queryset().select_related('some_field')

class ChildModel(ParentModel):
  some_field = models.ForeignKey(...)

  base_objects = ChildManager()

However the select_related config for the queryset on the childmodel will get overriden by this line: https://github.com/django-polymorphic/django-polymorphic/blob/master/polymorphic/query.py#L351

I think that if this line didn't exist, we would have a good chance so we can do select_related on child models.

I traced the history to 2010 and previous of this line. It seems to be there since the beginning. Could we do something like:

if not real_objects.query.select_related:
    real_objects.query.select_related = self.query.select_related

So that we don't override the select related config here?

@tleguijt
Copy link

tleguijt commented Sep 4, 2016

Are there any plans on supporting this and/or is any help needed?
Supporting select_related and prefetch_related would greatly improve the performance of some of our code that uses polymorphic models.

@vdboor
Copy link
Collaborator

vdboor commented Sep 12, 2016

I like the assignment of the select_related. We'll have to make sure though that it doesn't trigger Django errors when that field doesn't exist in a particular subclass. A well tested pull request on this is welcome!

@gregmuellegger
Copy link

Actually I think it shall trigger any Django errors if something is wrong. The real_objects queryset comes from the base_objects manager. And the base_objects is used specifically for retrieving objects of the subclass itself. So polymorphic shouldn't do any magic in this place.

@pablolmedorado
Copy link

You would make me the happiest man in the world if there was a solution to this. I have many relationships in child models and the number of queries is too high.

Thank you for your work @vdboor!

@WhyNotHugo
Copy link
Member

While working on #244, I uncovered something that might be useful, assume these models:

class Parent(PolymorphicModel):
    pass

class ChildA(Parent):
    name = models.CharField()

class ChildB(Parent):
    height = models.IntegerField()

This works:

obj = Parent.objects.non_polymorphic().select_related('childa__name')
obj.childa.name  # return the name!

Although this doesn't:

Parent.objects.select_related('childa__name')
# childa is an invalid field name here

So we clearly don't need to fiddle with django's internals to get this issue fixed. I believe that we can actually deal with the select_related when casting the polymorphic instances to their real child class. It's just that the API select_related(*fields) doesn't feel right. Maybe polymorphic querysets need something different, like a mapping of child_class: related_fields.

@smcoll
Copy link

smcoll commented Jul 27, 2017

Project maintainers: my employer is interested in putting a bounty on this ticket, for supporting both select_related and prefetch_related for inherited models. Is that alright by you? Can we lock down the requirements, such that a PR is likely to be accepted? For example, deciding on the syntax of the lookup.

@smcoll
Copy link

smcoll commented Jul 31, 2017

@vdboor ^^

@WhyNotHugo
Copy link
Member

FWIW, I'd chime in with a few dollars to a bounty for this -- I'd be able to make huge optimizations if this feature works. The same goes for #244.

@AlanCoding
Copy link

I want to press the question that @smcoll asked here, and express my own strong interest in getting this problem solved.

Can we lock down the requirements, such that a PR is likely to be accepted? For example, deciding on the syntax of the lookup.

Frankly I don't have a problem with the syntax Parent.objects.select_related('childa__name'). Is there any use-case this would not address? Would a solution of this form be accepted?

@csdenboer
Copy link

I would love a solution for this as well... I have to find a solution for the relationships in my child models in order to get a reasonable performance...

@lorinkoz
Copy link

lorinkoz commented May 7, 2018

Also interested in putting some dollars in a bounty.
Please notice that we need to make this work also with relations that span from other models that are not polymorphic, like:

NonPolymorphicModel.objects.select_related('relation_to_polymorphic_base__childA__name')

@AfrazHussain
Copy link

Any updates on this? I would love to put in a bounty for this as well.

@WhyNotHugo
Copy link
Member

Maybe we can use something like Bountysource to actually make these bounties a reality?

There's plenty of us really wanting this implemented!

@smcoll
Copy link

smcoll commented Oct 18, 2018

@vdboor what factors would need to be considered for a PR to be merged (like the lookup syntax)? That could be the scope of the bounty ticket. There has been plenty of interest in funding a solution- we just need coordination from a project maintainer so we know the funded work will be accepted into the codebase.

@WhyNotHugo
Copy link
Member

Ideally, the syntax should be the same as django's select_related, although it would only apply for subclasses that actually have the requested field.

Another alternative is to have something like:

queryset.select_polymorphic_related(
    SubClassA,
    'relation__to___subclass__a',
)

This allows more explicit checks that the field actually exists for some subclass.

@danielquinn
Copy link

danielquinn commented Oct 25, 2018

I chipped in $15 USD for this one.

@mikkothegeeko
Copy link

Any updates on this? I'm currently having this issue.

@Finndersen
Copy link

Finndersen commented Mar 10, 2021

EDIT: Nevermind, I think this addresses a different issue

Does this solve the problem for anyone?
https://gist.github.com/Finndersen/3a9647718b35d6776b0dc3808a501519
Adds new select_polymorphic_related() Queryset method. Might look into having it handle it seamlessly in standard select_related()

Based on work by AndySun25 from this discussion: #244

slemrmartin pushed a commit to slemrmartin/awx that referenced this issue Mar 30, 2022
It previously depended on a private Django internal class that changed
with Django 3.1.

I've switched here instead to disabling the django-polymorphic
accessors to get the underlying UnifiedJob object for a Job, which due
to the way they implement those was resulting in N+1 behavior on
deletes.  This gets us back most of the way to the performance gains
we achieved with the custom collector class.  See
jazzband/django-polymorphic#198.
@Finndersen
Copy link

By the way it appears doing a plain select_related() (without specifying fields) on the parent queryset works to select related instances on child classes, if this helps anyone

@mortenthansen
Copy link

This limitation made me switch to using the InheritanceManager from django-model-utils instead. I found this walkthrough of the alternatives helpful:
https://confuzeus.com/hub/django-web-framework/model-polymorphism/

@0legRadchenko
Copy link

#531

Seems like this guy solved this issue. Need more tests I think, but I've tested tons of cases and so far so good.

@pfcodes
Copy link
Contributor

pfcodes commented Feb 18, 2023

#531

Seems like this guy solved this issue. Need more tests I think, but I've tested tons of cases and so far so good.

if only this project wasn't abandoned...

edit:

This limitation made me switch to using the InheritanceManager from django-model-utils instead. I found this walkthrough of the alternatives helpful:
https://confuzeus.com/hub/django-web-framework/model-polymorphism/

thanks for this. InheritanceManager might be the move

@sbevc
Copy link

sbevc commented May 25, 2023

I was able to prefetch related polymorphic child subclass using Prefetch

Given this relations:

#models.py
class Respondent(PolymorphicModel):
    name = models.CharField()

class ChildRespondent(Respondent):
    extra = models.CharField()

class Answer(models.Model):
    respondent = models.ForeignKey(Respondent)
from django.db.models import Prefetch

# Get answers together with ChildRespondent instances
for answer in Answer.objects.prefetch_related(Prefetch("respondent", queryset=ChildRespondent.objects.all(), to_attr="child_respondent"):
    assert isinstance(answer.child_respondent, ChildRespondent)

Not the cleanest solution since polymorphism is lost, but at least the n+1 problem is solved.

pgammans added a commit to bva-icx/django-polymorphic that referenced this issue Jun 26, 2023
implement support for a single query for select related base fetches across
polymorphic models.

adds a polymorphic QuerySet Mixin to enable non polymorphic models to fetch
related models.

Fixes: jazzband#198 jazzband#436 jazzband#359 jazzband#244
Possible Fixes:
  jazzband#498: support for prefetch_related cannot fetch attributes not on all child models or via class names
Related: jazzband#531
pgammans added a commit to bva-icx/django-polymorphic that referenced this issue Jun 26, 2023
implement support for a single query for select related base fetches across polymorphic models.

adds a polymorphic QuerySet Mixin to enable non polymorphic models to fetch related models.

fixes: jazzband#198 jazzband#436 jazzband#359 jazzband#244
possible fixes:
    jazzband#498: support for prefetch_related cannot fetch attributes not on all child models or via class names
related: jazzband#531
andriilahuta pushed a commit to andriilahuta/django-polymorphic that referenced this issue Dec 19, 2023
implement support for a single query for select related base fetches across polymorphic models.

adds a polymorphic QuerySet Mixin to enable non polymorphic models to fetch related models.

fixes: jazzband#198 jazzband#436 jazzband#359 jazzband#244
possible fixes:
    jazzband#498: support for prefetch_related cannot fetch attributes not on all child models or via class names
related: jazzband#531
pgammans added a commit to bva-icx/django-polymorphic that referenced this issue Apr 4, 2024
implement support for a single query for select related base fetches across polymorphic models.

adds a polymorphic QuerySet Mixin to enable non polymorphic models to fetch related models.

fixes: jazzband#198 jazzband#436 jazzband#359 jazzband#244
possible fixes:
    jazzband#498: support for prefetch_related cannot fetch attributes not on all child models or via class names
related: jazzband#531
pgammans added a commit to bva-icx/django-polymorphic that referenced this issue Apr 4, 2024
implement support for a single query for select related base fetches across polymorphic models.

adds a polymorphic QuerySet Mixin to enable non polymorphic models to fetch related models.

fixes: jazzband#198 jazzband#436 jazzband#359 jazzband#244
possible fixes:
    jazzband#498: support for prefetch_related cannot fetch attributes not on all child models or via class names
related: jazzband#531
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.