Opened 6 years ago

Closed 4 years ago

#10515 closed New feature (wontfix)

Add __rand__ and __ror__ to QuerySet [PATCH]

Reported by: sebastian_noack Owned by: nobody
Component: Database layer (models, ORM) Version: 1.0
Severity: Normal Keywords:
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by lukeplant)

It would be nice, if the QuerySet would respect __rand__ and __ror__, when combining it with other QuerySets. Note following example:

class PublicArticleQuerySet(QuerySet):
	def iterator(self):
		for obj in  super(VisibleQuerySet, self).iterator():
			obj._is_public = True
			yield obj
	
	def __or__(self, other):
		self._merge_sanity_check(other)
		# If a PublicArticleQuerySet is or'ed with an other QuerySet
		# we have to clone the other instead of this. So a QuerySet
		# is returned instead of a PublicArticleQuerySet, unless both
		# are PublicArticleQuerySets.
		combined = other._clone()
		combined.query.combine(self.query, sql.OR)
		return combined

	def __and__(self, other):
		self._merge_sanity_check(other)
		# If a PublicArticleQuerySet is and'ed with an other QuerySet
		# we have to clone this instead of the other. So allways a
		# PublicArticleQuerySet is returned.
		combined = self._clone()
		combined.query.combine(other.query, sql.AND)
		return combined

	# We want the behaviour described, also when combining vice versa.
	__ror__ = __or__
	__rand__ = __rand__

class PublicArticleManager(models.Manager):
	def get_query_set(self):
		return PublicArticleQuerySet(self.model).filter(...)

class Article(models.Model):
	...

	objects = models.Manager()
	public_objects = PublicArticleManager()

	def is_public(self):
		if hasattr(self, '_is_public'):
			return self._is_public
		...

I have attached a patch which enables this feature.

Attachments (1)

added-__rand__-and-__ror__-support-to-QuerySet.patch (1.9 KB) - added by sebastian_noack 6 years ago.

Download all attachments as: .zip

Change History (8)

Changed 6 years ago by sebastian_noack

comment:1 Changed 6 years ago by lukeplant

  • Description modified (diff)
  • Needs documentation set
  • Needs tests set
  • Patch needs improvement unset

Some regression tests, which also show why this useful, need to be added. A simplified example would probably be better, if possible.

There is of course the problem about what happens when you combine two custom QuerySets, both with __ror (or __rand) defined - which one should be used? Perhaps that is telling us something...

comment:2 Changed 6 years ago by lukeplant

  • Description modified (diff)

comment:3 Changed 6 years ago by jacob

  • Triage Stage changed from Unreviewed to Design decision needed

If I'm reading the patch correctly, this would make it easier for a custom queryset subclass to ensure that and/or operations properly return the subclass instead of the root queryset or vice versa -- think Model.objects.public() & Model.objects.filter(name='foo'), where public() returns some queryset subclass that should "persist" when combined with other querysets.

It's a bit esoteric, frankly, and easily enough done in the custom queryset itself, so I'm probably -0 on the change if there's not a better reason than the one I can come up with.

comment:4 Changed 6 years ago by mtredinnick

If the goal is as Jacob mentions (although I think Jacob's got it backwards: the patch allows the case where public() is on the rhs of the "&" and dominates the resulting type), then this isn't the right solution: it's kind of making it work by accident. I don't like the approach in the patch, as it's overriding the natural __and__ and __or__ behaviour (it's normal to expect "&" and "|" to evalute the lhs argument first and act on that appropriately, this is changing that to make the rhs argument more important).

The problem of a "persistent" or preferred base class QuerySet during a sequence of operations would be nice to solve, as it would simplify some uses in, say, GeoDjango. That problem needs a different solution and is something I want to look at more in the 1.2 timeframe.

As with Jacob, if the there's no use-case other than what he describes, I'm also against adding this. It adds complexity without a huge benefit (sans the problem we should solve a bit more holistically). If it's about that particular use-case, then I'm not confident this is the correct solution, so we should hold off applying for a bit.

comment:5 Changed 6 years ago by sebastian_noack

Maybe you guys are right, that there might be a better way to go. But if you look at EmptyQuerySet, there are hooks to handle it differently in QuerySet's or and and method. And sometimes there is a need to have similar logic when combining custom QuerySets, too. So it might be nice to have an API for modifying the behaviour of combining QuerySets. The ror/rand approach, was just the best idea I came up with, because this way the same problem is solved in Python itself. But maybe somebody else have a better idea.

comment:6 Changed 4 years ago by SmileyChris

  • Severity set to Normal
  • Type set to New feature

comment:7 Changed 4 years ago by aaugustin

  • Easy pickings unset
  • Resolution set to wontfix
  • Status changed from new to closed
  • UI/UX unset

Jacob and Malcolm expressed some reluctance on this idea, and then not much happened.


The problem of a "persistent" or preferred base class QuerySet during a sequence of operations would be nice to solve

This problem is more likely to be resolved by making it easier to provide unified custom Managers and QuerySets, as discussed in [this thread http://groups.google.com/group/django-developers/browse_thread/thread/ad1af7d783317302].

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