[Django] #22885: Make Query.set_limits a setter
Django
----------------------------------------------+--------------------
Reporter: jorgecarleitao | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Database layer (models, ORM) | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
`Query.set_limits(low=None, high=None)` is used to create LIMIT low OFFSET
high - low.
However, as pointed out in [https://groups.google.com/d/msg/django-
developers/oqgtvFE5y84/gZK6ccOmjRcJ 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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/22885>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jorgecarleitao | Owner: nobody
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Database layer | Resolution: wontfix
(models, ORM) | Triage Stage:
Severity: Normal | Unreviewed
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by russellm):
* status: new => closed
* needs_better_patch: => 0
* resolution: => wontfix
* needs_tests: => 0
* needs_docs: => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22885#comment:1>
Django
Reporter: jorgecarleitao | Owner: nobody
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Database layer | Resolution: wontfix
(models, ORM) | Triage Stage:
Severity: Normal | Unreviewed
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by 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).
--
Ticket URL: <https://code.djangoproject.com/ticket/22885#comment:2>
Django
Reporter: jorgecarleitao | Owner: nobody
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Database layer | Resolution: wontfix
(models, ORM) | Triage Stage:
Severity: Normal | Unreviewed
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by russellm):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22885#comment:3>
Django
Reporter: jorgecarleitao | Owner: nobody
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Database layer | Resolution: wontfix
(models, ORM) | Triage Stage:
Severity: Normal | Unreviewed
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by 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,
[https://github.com/django/django/pull/2806 this merged PR] and
[https://github.com/django/django/pull/2812 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
[https://docs.djangoproject.com/en/dev/howto/custom-model-fields
/#converting-field-data-for-serialization suggests using private methods]
and [https://docs.djangoproject.com/en/dev/ref/models/custom-lookups/#a
-simple-lookup-example 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22885#comment:4>