Code

Opened 6 years ago

Closed 6 years ago

#7895 closed (wontfix)

Split Query add_filter and setup_joins into smaller methods for easier subclassing

Reported by: fabiocorneti Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Keywords: query pluggables
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Projects such as Django Multilingual use an highly customized django.db.models.sql.Query subclass to support advanced custom filters; in order to do so we need to copy the entire setup_joins and add_filter methods and apply our custom code inside them.

The proposed patch splits add_filter and setup_joins in slightly smaller methods, allowing subclasses to inject custom code into filtering procedures in an easier and more maintainable way.

Tested with Django trunk using the standard test suite.

Attachments (1)

query_splitted_methods.diff (16.9 KB) - added by fabiocorneti 6 years ago.

Download all attachments as: .zip

Change History (5)

Changed 6 years ago by fabiocorneti

comment:1 follow-up: Changed 6 years ago by Alex

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I haven't had a chance to review this patch in any detail, but you'll probably want to apply whatever changes you make here to the OracleQuery class as well.

comment:2 in reply to: ↑ 1 Changed 6 years ago by fabiocorneti

Replying to Alex:

I haven't had a chance to review this patch in any detail, but you'll probably want to apply whatever changes you make here to the OracleQuery class as well.

The patch splits the setup_joins and add_filter methods which are not overridden by OracleQuery.

comment:3 Changed 6 years ago by Alex

Ah, ok.

comment:4 Changed 6 years ago by mtredinnick

  • Resolution set to wontfix
  • Status changed from new to closed

setup_joins(), in particular, is very performance critical. I originally wrote a few versions that split out the inner loops, but the difference in all the function calls if very noticeable. I'm not particularly inclined to make this change at the moment, since it's not really clear that poking at the internals of setup_join() is a good plan. Your change is predicated on the assumption that django-multilingual's approach is a good idea (somehow adding extra joins without being explicit) and I'd rather this was done through more natural filter usage. I think this really needs to first be a design discussion about the approach to constructing the query, since there are alternative approaches. It really is one method for a reason.

It certainly may well be possible to add extra hooks into the join setup and/or filter creation and it might be possible to split up add_filter(), although, again, it sounds fragile to inject into the middle of that, since it's a fairly self-contained operation.

Therefore, this is a case of first working out the problems you are trying to solve and all of use coming up with a solution that meets all requirements, rather than going straight to this particular solution implementation, which has some drawbacks will real impact on the common cases. Going to wontfix for now for those reasons.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.