8000 If order_with_respect_to is a foreign key of a foreign key it generates aditional queries. · Issue #293 · django-ordered-model/django-ordered-model · GitHub
[go: up one dir, main page]

Skip to content

If order_with_respect_to is a foreign key of a foreign key it generates aditional queries. #293

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

Open
rafammpp opened this issue Mar 28, 2023 · 5 comments

Comments

@rafammpp
Copy link
rafammpp commented Mar 28, 2023

Example

class Site(models.Model):
    ...

class Category(models.Model):
    site = models.ForeignKey(Site)

class Article(OrderedModel):
    order_with_respect_to = 'category__site'
    category = models.ForeignKey(Category)

If I do a queryset of Articles, it make an aditional query for every article, even if I use Article.objects.select_related('category__site').

@ibaguio manage to fix this problem in c4630dc for a simple foreign key, but if you order with respect to a foreign key of a foreign key it makes a query to retrieve the category.

If I understand it well, you save the initial value of the wrt_map to the instance at init time to check for changes later on save, and then do the proper arragments if needed. wrt_map is a dict of every item in order_with_respect_to with it's respective values. But you only need it on save. My proposal is to get the saved value from the db, and then check if it's different from the current instance to be saved. Yes, we make an aditional query, but only when we need it (on save) and can be the cheapest query if we defer all values other than the pk.

I've a view that list articles. Currently it makes over a thousand queries, and with this patch implemented, or with the previous version of this package, it gets down to eight.

I'm gonna make a pull request with my proposal, see you there.

@shuckc
Copy link
Member
shuckc commented Mar 29, 2023

I'm aiming for it not be an additional query because those fk values have already been fetched from the DB when the object was queried. This does not currently hold with a 'double underscore' relation, which looks to be your case.

We spoke about how to move forward during review of that PR. It seems to me that there should be a modification to the QuerySet to dynamically add the extra relation, perhaps adding Article.objects.select_related('category__site_id'), then a query-counting test case around it to make sure it holds.

At the moment the admin does multiple subqueries whilst constructing a similar, but different, map of the ordered set for use in constructing the admin change links. It is with optimising this that I do not want to defer the lookup of order fields until save occurs on the model.

8000

@ibaguio
Copy link
Contributor
ibaguio commented Mar 31, 2023

@shuckc, i'm personally always cautious when using select_related as the default solution to an FK problem, coz it "downloads" the entire related table, where you only need one field. My years of django experience has taught me to always default to use annotations, and only use select_related when you can't help it.

For @rafammpp 's problem, i think this can be solved by annotating the values on the queryset, top of mind here's how i would implement it:

class OrderModelQuerySet:
    def annotate_wrt(self):
        owrt = self.model.order_with_respect_to

        if any("__" in wrt for wrt in owrt):
            return self.annotate(
                **{
                    field_path: F(f"{field_path}__id")
                    for field_path in owrt
                    if "__" in field_path
                }
            )
        
        # no dunder relation found in order_with_respect_to
        # no annotations needed      
        return self

class OrderModelManager:
    def get_queryset(self):
        # Annotate the WRT by default
        return super().get_queryset().annotate_wrt()


# Sample model
class Answer(OrderdModel):
    order_with_respect_to = ("question__asker")


a = Answer.objects.all()
a.question__asker  # this will print a.question.asker_id, without any additional SQL query, since it's annotated

Here we are annotating the values of the related fields by default.
I can write the PR for this if you think this is a viable solution.

Notes:

  • Code above is untested, but should give you the idea.
  • We need to also consider RelatedManagers, i.e question.answers.first() will not trigger the get_queryset afaik.
  • We will need to add a fallback, when the value is not annotated, it has to go through the inefficient method of following the relation

@shuckc
Copy link
Member
shuckc commented Mar 31, 2023
8000

Yes that's the idea you mentioned previously, I put it together (using annotate())two days ago on #295
https://github.com/django-ordered-model/django-ordered-model/pull/295/files#diff-62376346d0f6565502ee25a8a0af0478fc35c504178a023026ca9bf462e2ab0dR100

I annotated non-dunder FK's too, as that is presumably done at no cost and keeps the handling similar.

@shuckc
Copy link
Member
shuckc commented Mar 31, 2023

You have a good point about needing a better fallback for RelatedManagers - if you modify then save the object derived from one of those, it will not be annotated. My code assumes it is new object in this case and will not upshuffle the departing wrt set. I'll add a test for it.

During model __init__() the annotations are not yet available (and you don't know if they ever will be...), so I read the annotations during save(). However at that point its too late to examine the 'prior' value of double underscore relations, as they may have been changed, and repeating the DB query into a shadow object to inspect (as OP @rafammpp does in #294) is itself not entirely without issues.

riconnon pushed a commit to riconnon/django-ordered-model that referenced this issue Jul 23, 2023
amineau added a commit to bimdata/django-ordered-model that referenced this issue Sep 13, 2023
* add failing test for django-ordered-model#293

* Query ForeignKey id values during load to optimise order_with_respect_to check during save()

This favours a smaller overall query count over simpler queries.

By adding attributes during evaluation of the queryset, we can store the prior value of
ForeignKey relations, for the cost of a join for each entry within
order_with_respect_to (nested joins if LOOKUP_SEPARATOR (__) is used).

The fetched attibutes are not available during OrderedModel.__init__() so we defer creation
of self._original_wrt_map until save(). Since this reads from attributes it no longer
causes any additional queries.

This seems to balance correctness in the case of modifying the fields you are ordered
with respect to, against minimising extra queries during either get() or save()
operations, and does not require the full related object to be fetched to detect the change.

---------

Co-authored-by: Chris Shucksmith <chris@shucksmith.co.uk>
riconnon pushed a commit to riconnon/django-ordered-model that referenced this issue Oct 8, 2023
amineau added a commit to bimdata/django-ordered-model that referenced this issue Jan 4, 2024
* add failing test for django-ordered-model#293

* Query ForeignKey id values during load to optimise order_with_respect_to check during save()

This favours a smaller overall query count over simpler queries.

By adding attributes during evaluation of the queryset, we can store the prior value of
ForeignKey relations, for the cost of a join for each entry within
order_with_respect_to (nested joins if LOOKUP_SEPARATOR (__) is used).

The fetched attibutes are not available during OrderedModel.__init__() so we defer creation
of self._original_wrt_map until save(). Since this reads from attributes it no longer
causes any additional queries.

This seems to balance correctness in the case of modifying the fields you are ordered
with respect to, against minimising extra queries during either get() or save()
operations, and does not require the full related object to be fetched to detect the change.

---------

Co-authored-by: Chris Shucksmith <chris@shucksmith.co.uk>
@ZipBrandon
Copy link

Can we resurrect this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants
0