#16187: Refactor lookup system
-------------------------------------+-------------------------------------
Reporter: UloPe | Owner: UloPe
Type: | Status: new
Cleanup/optimization | Version: 1.3
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
I didn't have time to do the promised review before. But here it is:
I think the big issue is how to restructure the code in a way that there
is not duplication of the lookup path resolving code. Currently you need
to first walk the path in resolving the final field and lookup part of the
path, and then again in setup_joins(). This means you have to do pretty
much the same code in two places, which is IMHO not good. This is not an
easy issue to solve, as setup_joins refactoring is needed, and that piece
of code isn't the easiest to refactor.
I think there are some mistakes in the patch:
- IMHO it would be good to have the lookup path resolving done in
different method than add_filter, if for no other reason than readability.
- I don't like this logic:
{{{
for ...:
try:
...
try:
...
except FieldDoesNotExist:
if really_not_exists:
raise
...
except FieldDoesNotExist:
do stuff
}}}
Related to that, you are looking for aggregates only after you have
walked the path, this results in
`Foo.objects.filter(id__someaggregate__exact=1)` being valid. I think
the aggregate resolving must be
done upfront.
As it happens, I had some code related to one of my projects lying around
which could be reused for this. I have attached it as a patch. It does
more than is needed (it actually tries to resolves the joins needed), and
because of that there is a lot of duplication of code to setup_joins. But
it does pass the test suite (well, there is one test failing, but it not
failing before is a bug in current implementation). I hope it gives some
ideas about how to solve this issue. It is incomplete and downright uqly
in places, though.
I don't understand the last step's problem Alex mentioned above. Why can't
you just add the lookup normally into the where / having clause, and
resolve it into SQL in the compiler in the normal way:
{{{
# compiler.py:72-73
where, w_params = self.query.where.as_sql(qn=qn,
connection=self.connection)
having, h_params = self.query.having.as_sql(qn=qn,
connection=self.connection)
}}}
Or is the idea that the lookup can return something else than things going
into the where / having parts of the query?
Last: I had some problems making this work with GenericRelations. Is there
some reason why they are using ManyToManyRel instead of ManyToOneRel? That
seems just wrong...
--
Ticket URL: <https://code.djangoproject.com/ticket/16187#comment:8>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/django-updates?hl=en.
