#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 django-updates@googlegroups.com. To unsubscribe from this group, send email to django-updates+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-updates?hl=en.