Make Query.set_limits a setter

[jorgecarleitao]

Query.set_limits(low=None, high=None) is used to create LIMIT low OFFSET high - low.

However, as pointed out in ​this thread, it *adds* to the limits and doesn't *set* the limits.

Since Query is responsible for storing the SQL expression to be generated, I propose Query.set_limits to be the interface to set the limits of the query, and make QuerySet responsible for adding the limits if required. I.e. I propose that set_limits overrides the previous limits. In particular the following becomes equivalent:

Query.set_limits(a, b).set_limits(c, d) 
Query.set_limits(c, b) 

Currently this method is used in:

• QuerySet.__getitem__,
• QuerySet._earliest_or_latest (calls clone.query.set_limits(high=1))
• Query.has_results (calls clone.set_limits(high=1))

and QuerySet doesn't seem to be using the add part anyway: neither _earliest_or_latest (i.e. the methods that use it) nor __getitem__ return a new QuerySet.

[Russell Keith-Magee]

It's worth pointing out that the referenced thread is from 2007 - when the internals of QuerySet were being massively refactored - and then revived for some reason in 2013. The discussion comes from a time where we were sorting out the basic semantics of the QuerySet API. That API is now stable, well established, and has a firm set of community expectations.

Slicing is a terminal operation for a reason - there's no other reasonable way to interpret it otherwise. Consider the following query:

Author.objects.filter(name_startswith='a')[:10].filter(name__contains='e')

That requires you to find the first 10 authors whose name starts with a, and then filter that list of 10 down to only those that have an 'e' in their name as well. It's a completely different query to:

Author.objects.filter(name_startswith='a').filter(name__contains='e')[:10]

which finds the first 10 authors that start with an 'a' *and* have an e in their name.

It's *possible* to issue the former query in SQL, but it certainly isn't simple - it requires a join, even though you're only dealing with one table.

Alternatively, you'd need to come up with an interpretation of set_limit() that is inconsistent with all the other operations you can perform on a query set.

If there was a pressing use case for this particular construct, I might be inclined to explore the options further, but the ticket doesn't provide any example of *why* this construct is needed. Closing wontfix; if you want to make the case for this feature, please start a discussion on django-dev.

[jorgecarleitao]

Maybe the ticket was not clear enough, but it proposes a minor change of the interface of Query, which is not part of the public API; it does not proposes a new (public) interface of QuerySet. Is just that Query.set_limits is adding to the limits, when IMO should be setting the limits. The fix of this ticket would not change any public behavior (thus being a cleanup/optimization).

[Russell Keith-Magee]

Ok - but I'm still left with the question "why?"

As you've said, it's internal an internal API. Making a feature request on an internal API doesn't make much sense to me - you must have a use case in mind, which isn't clear at all from this ticket.

Alternatively, have you found a bug? If so, this isn't a feature request - it's a bug report - in which case, we need to know how the bug manifests.

[jorgecarleitao]

IMO, it improves code readability and simplifies Query's interface; that is why is marked has a cleanup/optimization and not as a feature request.

The whole point of this ticket, ​this merged PR and ​this open PR is that I'm not in position to propose a refactor to Query, also because I still don't have a solid proposal of an interface to Query to interact with SQLCompiler, SQLEvaluator and QuerySet. Since I cannot tell if I will be able to create such proposal, I'm proposing minor changes that clarify this interaction.

The reason why IMO such proposal is needed is that custom fields and the new custom lookups (1.7) are pushing the boundaries of "public API" too close to "private API". Specifically, the 1.7 documentation already ​suggests using private methods and ​private classes:

[...] SQLCompiler objects are not documented, but the only thing we need to know about them is that they have a compile() method which returns a tuple containing a SQL string, and the parameters to be interpolated into that string. In most cases, you don’t need to use it directly and can pass it on to process_lhs() and process_rhs().

IMO, this is already a risk we are taking and suggests that when #14030 lands (on 1.8?), Query, SQLCompiler and SQLEvaluator (and their interactions) will need to be addressed. IMO, either me or someone else will profit from having the way paved, e.g. by having Query's interface cleaned.

So, taking into account the contribution guidelines, this is more than a simple a typo, but is less than a solid proposal to be made on the mailing list.
If you think it's better, I can create a broader ticket explaining this, even if without a definitive proposal in mind.