Reducing number of db queries by using select_related data #216
Conversation
Add django-composite-foreignkey to deps
Codecov Report
@@ Coverage Diff @@
## master #216 +/- ##
==========================================
+ Coverage 86.19% 87.72% +1.52%
==========================================
Files 30 31 +1
Lines 1797 2037 +240
==========================================
+ Hits 1549 1787 +238
- Misses 248 250 +2
Continue to review full report at Codecov.
|
|
Wow, thanks for this work! I'll take some time to evaluate this. A few questions:
|
parler/fields.py
Outdated
| from parler.utils.i18n import get_language | ||
|
|
||
| if (1, 8) <= django.VERSION < (2, 0): | ||
| from compositefk.fields import RawFieldValue, CompositeOneToOneField |
There was a problem hiding this comment.
I'm fine with dropping Django 1.7 support, but would love to have Django 2.0 support in.
There was a problem hiding this comment.
Added support Django 2 to django-composite-foreignkey recently. So can remove the 'if' here.
parler/fields.py
Outdated
| if 'to_fields' in kwargs: | ||
| kwargs['to_fields'] = {'master_id': None, 'language_code': None} # hack: Need always the same dict | ||
| if "on_delete" in kwargs: | ||
| kwargs['on_delete'] = DONOTHING # hack: Need always the same global object with __module__ attr |
There was a problem hiding this comment.
What is the purpose of this hack?
There was a problem hiding this comment.
The hack is need because repitative migrations problem, here
https://github.com/Andrey-Skvortsov/django-composite-foreignkey/blob/91c093c8c81b38a8267557597c6bc0870e4315c3/compositefk/fields.py#L48
they are doing overriding on_delete and Django migrations produced a new migration then do makemigrations with the same code even there are no any changes. It's do such because it compare the models's field and found diff in on_delete as it different instances here.
It's already fixed here:
https://github.com/Andrey-Skvortsov/django-composite-foreignkey/blob/91c093c8c81b38a8267557597c6bc0870e4315c3/compositefk/fields.py#L196
but not in PyPi yet, so it needs bumped version in django-composite-foreignkey.
| return new_model | ||
|
|
||
|
|
||
| class TranslatableModel(six.with_metaclass(TranslatableModelBase, TranslatableModelMixin, models.Model)): |
There was a problem hiding this comment.
Note to self: It would be good to point out in the CHANGES that TranslatableModel uses a meta class now. Projects that define a meta class for any inherited class need to take note of this.
There was a problem hiding this comment.
Yep, there are no way to escape to do meta-class for TranslatableModel if we want to support django model inheritance. And we want - we have test_inherited_model in tests.
| @@ -1085,6 +1187,7 @@ def __init__(self, base, shared_model, translations_model, related_name): | |||
|
|
|||
| self.base = base | |||
| self.inherited = False | |||
| self._local_extensions = [] | |||
There was a problem hiding this comment.
What is "local extensions" doing? I find the name a bit vague
There was a problem hiding this comment.
It needs to know which extensions are belong to model or come from parent model through model inheritance.
We create additional attributes only for new extensions. Agree, the name is not clear. I was following the logic we have in django's model._meta: we have fields, concrete_fields, many_to_many attributes to access to all fields of the model and we have local_fields, local_concrete_fields, local_many_to_many to access only to current model fields without inherited fields.
parler/tests/test_query_count.py
Outdated
| from unittest import skipIf | ||
| except ImportError: | ||
| # python<2.7 | ||
| from django.utils.unittest import skipIf |
There was a problem hiding this comment.
Python 2.6 compatibility is not required. It's EOL and obsolete and parler doesn't support it anymore
parler/utils/fields.py
Outdated
| raise NotRelationField | ||
|
|
||
|
|
||
| def get_extra_related_translalation_paths(model, path): |
There was a problem hiding this comment.
typoo in the function name!
There was a problem hiding this comment.
Also needs test coverage for this function (TODO)
| pass | ||
|
|
||
|
|
||
| def get_model_from_relation(field): |
There was a problem hiding this comment.
You can keep this function private, and use a standard ValueError exception
| """ | ||
|
|
||
| force_select_related_translations = True |
There was a problem hiding this comment.
Naming this select_related_translations seems clear to me too
| @@ -119,6 +131,47 @@ class TranslationDoesNotExist(AttributeError, ObjectDoesNotExist): | |||
| _lazy_verbose_name = lazy(lambda x: ugettext("{0} Translation").format(x._meta.verbose_name), six.text_type) | |||
|
|
|||
|
|
|||
| def create_translations_composite_fk(shared_model, related_name, translated_model): | |||
There was a problem hiding this comment.
What is this function doing and how is it related to the "select related" logic?
There was a problem hiding this comment.
This function adds 2 attributes to the main model with virtual composite foreign key to model with translations. It let us be able to join main model with translations as one-to-one twice with default and current languages. The QuerySet class adds automatically these attributes to select_related so it let have an access to translations in 2 languages reducing DB hitting.
| @@ -1057,6 +1157,8 @@ def __init__(self, shared_model, translations_model, related_name): | |||
| self.shared_model = shared_model | |||
| self.model = translations_model | |||
| self.rel_name = related_name | |||
| self.rel_name_active = None | |||
| self.rel_name_default = None | |||
There was a problem hiding this comment.
What are rel_name_default and rel_name_active doing?
There was a problem hiding this comment.
This the names for new 2 attributes to the main model with virtual composite foreign key. Refer to translations in default and active languages.
|
Answering to the questions:
|
Andrey-Skvortsov
force-pushed
the
master
branch
2 times, most recently
from
bd75610
to
5d7ac0e
Compare
| current_path = '' | ||
| extra_paths = [] | ||
| for piece in pieces: | ||
| field = parent._meta.get_field(piece) |
There was a problem hiding this comment.
Here, we are iterating through the chain of fields by fetching the Field instance is from parent._meta.get_field, and then update parent from get_model_from_relation. Why can't we directly iterate the options like:
option = model._meta
for piece in pieces:
field = option.get_field(piece)
option = field.get_path_info()[-1].to_opts
...
...and then no need to get the model of each field in the chain.
| parent = get_model_from_relation(field) | ||
| current_path += LOOKUP_SEP + piece if current_path else piece | ||
| if issubclass(parent, TranslatableModel): | ||
| for extension in parent._parler_meta: |
There was a problem hiding this comment.
As I understand, get_extra_related_translation_paths is to facilitate SelectRelatedTranslationsQuerySetMixin.
And SelectRelatedTranslationsQuerySetMixin is designed for the case that, if we have the following models:
class SimpleModel(TranslatableModel):
shared = models.CharField()
translations = TranslatedFields(
tr_title=models.CharField()
)
class SimpleRelatedModelManager(SelectRelatedTranslationsQuerySetMixin, models.Manager):
pass
class SimpleRelatedModel(models.Model):
some_attribute = models.CharField()
some_reference = models.ForeignKey(SimpleModel, on_delete=models.CASCADE)
objects = SimpleRelatedModelManager()
...then we we do
SimpleRelatedModel.objects.select_related('some_reference')
the queryset will actually do
SimpleRelatedModel.objects.select_related(
'some_reference',
'some_reference__translations_active',
'some_reference__translations_default',
)
However there is a problem in this approach. Say now we have these models instead:
class SimpleModel(TranslatableModel):
shared = models.CharField()
translations = TranslatedFields(
tr_title=models.CharField()
)
class SimpleRelatedModelManager(SelectRelatedTranslationsQuerySetMixin, models.Manager):
pass
class TranslatedSimpleRelatedModel(TranslatableModel):
translations = TranslatedFields(
tr_attribute = models.CharField()
)
some_reference = models.ForeignKey(SimpleModel, on_delete=models.CASCADE)
class AnotherRelatedModel(models.Model):
another_attribute = models.CharField()
another_reference = models.OneToOneField(TranslatedSimpleRelatedModel, on_delete=models.CASCADE)
objects = SimpleRelatedModelManager()
...then when we do
AnotherRelatedModel.objects.select_related('another_reference__some_reference')
...the logic in SelectRelatedTranslationsQuerySetMixin will actually do
AnotherRelatedModel.objects.select_related(
'another_reference__some_reference, '
`another_reference__translations_active`,
`another_reference__translations_default`,
`another_reference__some_reference__translations_active`,
`another_reference__some_reference__translations_default`,
)
As we only want to select_related the active and default translations of SimpleModel, another_reference__translations_active and another_reference__translations_default are unnecessary extra paths and we will end up select more related translation paths than we need.
I am working on a branch to address some problems in this PR, and it contains a different approach as a fix for the problem I describe above.
https://github.com/Andrey-Skvortsov/django-parler/pull/1/files#diff-f2bbe1670b2ebcf7b29f969e89fb3540R25
Andrey-Skvortsov
force-pushed
the
master
branch
from
5d7ac0e
to
ff90ddd
Compare
|
Hey guy, The dumpdata fails for a TranslatableModel However dumpdata for Translations succeed |
vdboor
force-pushed
the
master
branch
4 times, most recently
from
2bf75d2
to
577ca2f
Compare
Overview
Currently to get translated data django-parler do query to db for each record in main, default and fallback languages. It can be cached, but it needs the cache to be filled. It can be reduced by using prefetch_related. Disadvantages using prefetch related:
What is new
The PR introduces automatic addition virtual composite foreign keys from shared model to translated model to access to translations in default and active languages and automatic addition such relations to select_related for models' queryset. This optimisation covers main 99% use cases when you just need data in current active language.
The addition happens if you work under Django ver 1.8+
For Django 1.7 it works as early.
What is used
It used the django-composite-foreignkey module
https://pypi.python.org/pypi/django-composite-foreignkey
This module works under Django 1.8+
How it works
You use as usual Translated models, it will join query (OUTER JOIN) model twice with translations models in 2 languages.
JOIN not happening if it queries only non-translatable fields
If you use
.only()and all the fields are not translatable there are no additional JOINs with translations models. In all other cases translated fields are expected and do JOIN to prevent having additional queries.Do not want have additional joins
You could disable this by adding:
force_select_related_translations = Falseto queryset or useLightTranslatableManageras a base manager for model. (but you still can join with translations manually call.select_related('translations'))More complex usage translatable models
If you have a normal(not translatable) model which is joining with translatable models eg.: you have mode1 joining with model2 and model3 like
.select_related('model2__model3')and for not having additional queries for translations for model2 and model3 you have to also add.select_related( 'model2__model3', 'model2__translations_active', 'model2__translations_default', 'model2__model3__translations_active', 'model2__model3__translations_default', )So if you do not want worry about which additional translation models you need add to select_related you can:
SelectRelatedTranslationsQuerySetMixinto base classes for your models' queryset or use:Then it adds
.select_related( ... 'model2__translations_active', 'model2__translations_default', 'model2__model3__translations_active', 'model2__model3__translations_default', )automatically. So you will have one SQL-query for everything do not carry about which models is translatable ones.
..also it works with translatable model
In the last example if model1 is translatable you can use:
Simplify usage translatable fields in .only()
In translatable model you can still use
.only('field1')even if the field1 is translatable and do not belong to the model. In that case it will be automatically replaced withtranslations_activeandtranslations_defaultfields in.onlyfields list and by that in will be automatically queried select related as base functionality of TranslatableQuerySet.