Opened 17 years ago
Closed 16 years ago
#7210 closed (fixed)
Added expression support for QuerySet.update
Reported by: | Sebastian Noack | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Keywords: | ||
Cc: | Russell Keith-Magee, Malcolm Tredinnick, Nicolas Lara, gav@…, Gonzalo Saavedra, Joel Watts, simon@…, Reflejo@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | yes |
Needs tests: | yes | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
I think QuerySet.update works to inflexible. In SQL you can define expressions containing the current value or the value of an other column in the update clause, for Example to incrementing a value in the result set. This is not possible by the ORM at the moment. I wrote a possible patch, with which you can do following:
from django.db.models.sql.expressions import * # Equivalent to model.all().update(foo=42) model.all().update(foo=LiteralExpr(42)) # Increment column 'foo' by one. model.all().update(foo=CurrentExpr() + LiteralExpr(1)) # Swap the value of the column 'foo' and 'bar'. model.all().update(foo=ColumnExpr('bar'), bar=ColumnExpr('foo'))
Attachments (12)
Change History (27)
by , 17 years ago
Attachment: | 0001-Added-expression-support-for-QuerySet.update.patch added |
---|
comment:1 by , 17 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
Triage Stage: | Unreviewed → Design decision needed |
by , 17 years ago
Attachment: | 0001-Added-expression-support-for-QuerySet.update.2.patch added |
---|
by , 17 years ago
Attachment: | update_tests.py added |
---|
by , 17 years ago
Attachment: | 0001-Added-expression-support-for-QuerySet.update.3.patch added |
---|
comment:2 by , 17 years ago
Cc: | added |
---|
by , 17 years ago
Attachment: | 0001-Added-expression-support-for-QuerySet.update.4.patch added |
---|
by , 17 years ago
Attachment: | 0001-Added-expression-support-for-QuerySet.update.5.patch added |
---|
comment:4 by , 17 years ago
Yes and exclude() too. But it is not called ColumnExpr anymore. See the discussion at the mailing list (link above).
comment:5 by , 17 years ago
Cc: | added |
---|
by , 16 years ago
Attachment: | 0001-Added-F-support-to-filter-exclude-and-update.patch added |
---|
by , 16 years ago
Attachment: | 7210-F-support.2.patch added |
---|
fixed patch - last one was missing two files
comment:6 by , 16 years ago
I've read through this patch in detail for the first time. It smells a bit over-engineered in places, so I'm going to have to think about that (there seem to be too many extra classes involved). For now, though, there are some more fundamental problems I'd like to bring up:
- You've somewhat arbitrarily removed the
get_placeholder()
stuff. That is there because it's needed by certain extensions (in particular, geo-django, but it will also be useful in other cases). Remember that this is all new code; it's not like things are hanging around for historical reasons. Basically the "placeholder" is either"%s"
in the normal case or some other format string (e.g."SOME_FUNC(%s)"
in cases like the GIS situations. So treat the placeholder as an opaque string that you use instead of%s
to indicate parameters. - The
WhereNode
class is now a proper tree of otherWhereNode
classes (after [7835]), so that might affect some things in this patch. In particular, converting things passed in to values should be done inWhereNode.add()
so that no references to fields or models are stored in theWhereNode
class. This avoids infinite loops and pickling problems. - Calling
curry()
inmake_atom()
doesn't looks useful. You just want to save it to use as a function later when you still have the pieces of information you use. Query construction takes long enough without extra overhead like this, so just call the function directly at the right moment. - Having to pass
opts
toas_sql()
-- which will now beadd()
after [7835] -- feels wrong. The where-class itself shouldn't need to care about that, it's purely for the benefit of the "smart objects", so they should contain the information they need. This feeds into the next point (and below) that, in general, these classes should know how to convert themselves to SQL fragments. - Having to convert normal values to
LiteralValues
just to convert them back looks like unneeded overhead to me. Follow the pattern elsewhere: the default case will be normal values (the things we do now). That will be by far the most common stuff and it gets handled directly inmake_atom()
. Anything else, such as any smart objects, should convert themselves via a common method (their interface) and return the resulting string and list. They should have something like their ownas_sql()
ormake_atom()
method that is called if we detect the "value" has such a method. So it will be similar to the approach inas_sql()
: if the thing we're processing has its ownas_sql()
method, call that; otherwise, it's a basic piece of data and we'll handle it viamake_atom()
. In this case withExpression
derivatives, maybemake_atom()
needs to check ifvalue()
has its ownmake_atom()
ormake_value()
method or something. The idea here is to keep the level of coupling between the normalWhereNode
code and any objects likeF()
to an absolute minimum.F
andExpression
don't need to be treated specially. They are examples of a class of smart objects that should know how to prepare themselves.
I realise points 4 and 5 above sounds a bit less concrete than the others, but they're actually pretty major. Right now, this patch introduces some pretty tight coupling between these new classes and the query construction code. My intuition is that this coupling is not necessary. If you have to mention something like Expression
in WhereNode
, you've probably got a leaky abstraction. Use the interface that something like Expression
should have to call it and that way we're not tied to only using the Expression
class.
I suspect portions of this could probably be broken up into separate steps. I like the idea of an F()
object; that's certainly necessary. All this stuff to allow additions and subtraction and other manipulations of values might be useful at some point, but it's probably less important than the ability to refer to a field in an expression and working out the correct way to call smart objects as values. It can certainly be added later and, if the changes I'm talking about above are done right, doesn't have to be part of core immediately, which buys us some room to experiment. So don't try to do too much at once here.
One thought I had is that the bit in make_atom()
that currently says
if isinstance(params, QueryWrapper): ...
could take over the general role of smart objects here and become something like
if hasattr(params, 'make_value'): ...
and then we have to work out how to weave general return values into the result (the current (extra, params)
tuple probably won't be enough). In any case, that's the sort of lines I'd work along to make things work like the rest of the code throughout the query construction: we provide the means to "shell out" to advanced objects and handle only the base case in the core code.
Note that this shows we've already got this slightly leaky abstraction in the code, as a once-off to handle SQL subqueries as values. Designing this particular patch correctly should actually help plug that leak by possibly removing the need for WhereNode
to know or care about QueryWrapper
.
That's where I'm sitting at the moment. The idea is certainly worth pursuing. There are some implementation issues that need to be solved, as well as some broader design ones before we can go further. I'm not completely convinced it's "1.0-beta" material. Nice to have, but not a showstopper if we don't have it in 1.0. We can certainly add this stuff at any point without breaking the external API. Getting it right is therefore more important than getting it done quickly.
comment:7 by , 16 years ago
Another thing I've noticed with this patch is that it handles the fields based on the model's _meta attribute. This is a problem if F (or any expression) is going to be used with extra(), related fields or aggregates. I wanted to re-write it to avoid the dependency of the model at least for the quering case.
Also, the latest trunk revision introduces some changes that heavily conflict with this patch. Mostly the in "where.py" and "sql/query.py".
The only problem I see with the objects converting themselves to SQL fragments is that sometimes the objects don't have all the necessary information to do that and would require information of the state of the query class (e.g. aliases). The same problem would arrise if we want to allow relation spaning fields to be used without explicitly joining the tables beforehand. The second point (relation spaning fields) is a far fetched case and I'm not sure it should even be allowed, but the first one is a common case that could easily be solved by setting a placeholder for the field information and letting the queryset code handle the propper name selection.
I am not sure either that this belongs in "1.0-beta" but I have been working on it and would like to get it working after EuroPython's sprint.
comment:8 by , 16 years ago
milestone: | 1.0 beta |
---|---|
Patch needs improvement: | set |
Triage Stage: | Design decision needed → Accepted |
Officially taking this off the 1.0 beta list. Nicolas has been working on this as part of the aggregation work, but it won't be ready for a merge.
comment:9 by , 16 years ago
Cc: | added |
---|
comment:10 by , 16 years ago
A typo in the patch at db-api.txt:1754
"When filtering you can refer to you can refer to other attributes of " -- double "you can refer to".
P.S. The trick with F-objects overloading math operators is very clever!
comment:11 by , 16 years ago
Cc: | added |
---|
comment:12 by , 16 years ago
Cc: | added |
---|
comment:13 by , 16 years ago
Cc: | added |
---|
comment:14 by , 16 years ago
Cc: | added |
---|
comment:15 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
(In [9792]) Fixed #7210 -- Added F() expressions to query language. See the documentation for details on usage.
Many thanks to:
- Nicolas Lara, who worked on this feature during the 2008 Google Summer of Code.
- Alex Gaynor for his help debugging and fixing a number of issues.
- Malcolm Tredinnick for his invaluable review notes.
Some quick tests, not a ton, but tests basic funcionality, should be a good starting point if anyone wants to help out.