Opened 12 years ago
Closed 12 years ago
#19184 closed Cleanup/optimization (fixed)
Remove EmptyQuerySet
Reported by: | Anssi Kääriäinen | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 1.4 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
qs.none() is currently implemented by returning an EmptyQuerySet which has no-op operators for most of the QuerySet methods.
This approach has several weaknesses:
- Doesn't work well with subclasses of QuerySet (#17271)
- Doesn't work well when Django uses subclasses internally to implement some functionality (values() for example, #15959)
- The EmptyQS doesn't actually work like QuerySet - for example qs.none().filter(nonexisting_field=someval). Also: #19173.
- Code duplication: the EmptyQS has around 150 lines of code duplicating the methods of QS. The duplicated methods just do return self. If you add a method to QS and forget it from EmptyQS strange things can happen.
Problems if removing EmptyQS:
- We have documented it in qs.none() docs. And, I can't see any way to have a deprecation period for this.
- There will be some performance penalty mainly from queryset cloning as the qs.none() methods now actually do something.
I am obviously +1 to just removing the !EmptyQS. The question is if people are relying on isinstance(someqs, EmptyQuerySet) calls in their code. If so, we could add "known_empty" property so that users could check if it is possible for this queryset to return any values.
A work-in-progress patch available from branch akaariai/django/rm_emptyqs.
Change History (6)
comment:1 by , 12 years ago
comment:2 by , 12 years ago
Not too familiar with ABC - how would this work with custom/inbuilt QuerySet subclasses?
Another possibility: could we add EmptyQuerySet to the bases in cloning if we know the QS is an empty one?
comment:3 by , 12 years ago
Sorry I meant using __instancecheck__
. Something along
class EmptyQuerySet(object): def __instancecheck__(self, instance): # Look for NothingNode
comment:4 by , 12 years ago
Looks perfect, thanks for pointing this out.
Now the only problem left is speed penalty due to cloning - but this gives me a good chance to advertise #16759... :)
comment:5 by , 12 years ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Design decision needed → Ready for checkin |
OK, patch updated (available from https://github.com/akaariai/django/compare/rm_emptyqs).
The EmptyQuerySet isn't instantiable any more, but that is more like an added feature than backwards compatibility problem. However the EmptyQuerySet still exists as a way to check if a queryset is from .none() call.
So, we don't have a backwards compatibility problem, and I think the performance regression caused by this change falls into category "if you don't have to be correct it is easy to be fast".
I will leave this ticket open for review & comments for some time, then commit.
comment:6 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Committed in a2396a4c8f2ccd7f91adee6d8c2e9c31f13f0e3f but I didn't reference this ticket in the commit message.
Maybe this would add too much boilerplate but
EmptyQuerySet
could be redefined as an ABC?