django-polymorphic icon indicating copy to clipboard operation
django-polymorphic copied to clipboard

select_related and prefetch_related for inherited models

Open eriktelepovsky opened this issue 9 years ago • 25 comments

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.

eriktelepovsky avatar Feb 12 '16 18:02 eriktelepovsky

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.

vdboor avatar May 04 '16 09:05 vdboor

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?

gregmuellegger avatar Jul 22 '16 07:07 gregmuellegger

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.

tleguijt avatar Sep 04 '16 11:09 tleguijt

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!

vdboor avatar Sep 12 '16 08:09 vdboor

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.

gregmuellegger avatar Sep 21 '16 12:09 gregmuellegger

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!

pablolmedorado avatar Apr 13 '17 19:04 pablolmedorado

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.

WhyNotHugo avatar Jul 09 '17 06:07 WhyNotHugo

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 avatar Jul 27 '17 17:07 smcoll

@vdboor ^^

smcoll avatar Jul 31 '17 20:07 smcoll

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.

WhyNotHugo avatar Jul 31 '17 20:07 WhyNotHugo

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?

AlanCoding avatar Aug 30 '17 15:08 AlanCoding

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

csdenboer avatar Apr 05 '18 11:04 csdenboer

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')

lorinkoz avatar May 07 '18 14:05 lorinkoz

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

AfrazHussain avatar Oct 17 '18 12:10 AfrazHussain

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

There's plenty of us really wanting this implemented!

WhyNotHugo avatar Oct 18 '18 02:10 WhyNotHugo

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

smcoll avatar Oct 18 '18 14:10 smcoll

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.

WhyNotHugo avatar Oct 18 '18 18:10 WhyNotHugo

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

mbayabo avatar Oct 02 '19 17:10 mbayabo

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: https://github.com/django-polymorphic/django-polymorphic/issues/244

Finndersen avatar Mar 10 '21 07:03 Finndersen

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

Finndersen avatar Apr 27 '22 02:04 Finndersen

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/

mortenthansen avatar Nov 08 '22 22:11 mortenthansen

https://github.com/django-polymorphic/django-polymorphic/pull/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.

0legRadchenko avatar Nov 09 '22 23:11 0legRadchenko

#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

pfcodes avatar Feb 18 '23 10:02 pfcodes

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.

sbevc avatar May 25 '23 19:05 sbevc