Opened 3 years ago

Closed 3 years ago

#19184 closed Cleanup/optimization (fixed)

Remove EmptyQuerySet

Reported by: akaariai 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 Changed 3 years ago by charettes

Maybe this would add too much boilerplate but EmptyQuerySet could be redefined as an ABC?

comment:2 Changed 3 years ago by akaariai

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 Changed 3 years ago by charettes

Sorry I meant using __instancecheck__. Something along

class EmptyQuerySet(object):
    def __instancecheck__(self, instance):
        # Look for NothingNode

comment:4 Changed 3 years ago by akaariai

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 Changed 3 years ago by akaariai

  • Patch needs improvement unset
  • Triage Stage changed from Design decision needed to 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 Changed 3 years ago by akaariai

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

Committed in a2396a4c8f2ccd7f91adee6d8c2e9c31f13f0e3f but I didn't reference this ticket in the commit message.

Note: See TracTickets for help on using tickets.
Back to Top