Opened 13 years ago

Closed 18 months ago

Last modified 14 months ago

#16211 closed New feature (fixed)

using negated F()-expression in update query

Reported by: Walter Doekes Owned by: David Wobrock
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: walter+django@…, s.angel@…, charette.s@…, David Wobrock 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

Hi,

as far as I can tell, there is currently no way to do this with the ORM:

UPDATE myapp_mymodel SET is_enabled = NOT is_enabled;

Using this:

MyModel.objects.update(is_enabled=(not F('is_enabled')))

.. becomes "is_enabled = true". And this:

MyModel.objects.update(is_enabled=~F('is_enabled'))

.. doesn't work, because the bitwise not operator is not defined on the ExpressionNode.

I've done a patch against 1.2 to fix add that.

It adds a unary "NOT"-connector to the combine_expression in BaseDatabaseOperations and it adds the __invert__ method to the ExpressionNode.

If you like this, I'll be happy to do a patch against trunk.

Regards,
Walter Doekes
OSSO B.V.

Attachments (6)

django-query-expression-negate.patch (1.4 KB ) - added by Walter Doekes 13 years ago.
issue16211-query-expression-logical-operators.patch (9.9 KB ) - added by Walter Doekes 13 years ago.
issue16211+14029-query-expression-extra-operators.patch (10.6 KB ) - added by Walter Doekes 13 years ago.
issue16211+14029-query-expression-extra-operators2.patch (11.4 KB ) - added by Walter Doekes 12 years ago.
clarify test and add partial docs
issue16211+14029-query-expression-extra-operators3.diff (11.6 KB ) - added by Koen Biermans 12 years ago.
wdoekes patch but applying cleanly on current trunk
issue16211+14029-query-expression-extra-operators4.diff (12.3 KB ) - added by twoolie 12 years ago.
koenb's patch, but documentation and compat added.

Download all attachments as: .zip

Change History (60)

by Walter Doekes, 13 years ago

comment:1 by Aymeric Augustin, 13 years ago

Has patch: set
Needs documentation: set
Needs tests: set
Triage Stage: UnreviewedAccepted

comment:2 by Aymeric Augustin, 13 years ago

Component: UncategorizedDatabase layer (models, ORM)
Type: UncategorizedNew feature

comment:3 by anonymous, 13 years ago

This is so indispensable !

comment:4 by s.angel@…, 13 years ago

Cc: s.angel@… added

comment:5 by Walter Doekes, 13 years ago

Thanks for the motivation, anonymous :)

New patch is against trunk, added other logical operators as well and added a proper testcase.

Now this should work too:

MyModel.objects.update(is_enabled=((F('error_count') * 2) < F('success_count')))

I did have to replace an '==' check with an 'is' check in utils/tree. I'm assuming it has no negative side-effects.

comment:6 by Ramiro Morales, 13 years ago

#14029 had reported this before. I closed it in favor of this one that has a patch.

comment:7 by Walter Doekes, 13 years ago

Added issue16211+14029-query-expression-extra-operators.patch which fixes incorrect comments about logical operators when referring to comparisons and which addresses the need for a TypeError raised in #14029 when trying to do boolean operations on F() (by raising on __nonzero__).

Now not F() will raise TypeError('Boolean operators should be avoided. Use bitwise operators.')

comment:8 by Walter Doekes, 12 years ago

Bump. The patch might be getting a bit stale after 5 months.

comment:9 by anonymous, 12 years ago

Version: 1.21.4

Bump. Why is this not merged yet?

comment:10 by Aymeric Augustin, 12 years ago

For starters the patch needs documentation and tests.

Last edited 12 years ago by Aymeric Augustin (previous) (diff)

comment:11 by anonymous, 12 years ago

Looks like it has tests. If docs were written and patch rebased, would it get merged?

comment:12 by Anssi Kääriäinen, 12 years ago

I quickly skimmed the patch and I do think this is useful, and that the patch is mostly ready. So, it is likely that this would get merged, although there can not be any guarantee.

My biggest worry is the __eq__ and __ne__ methods. I have a feeling defining these methods to return another expression could lead to weird situations. Basically any f expression always returns True from ==, and !=, so if people are using these comparisons (or we happen to use these comparisons internally in Django) things will break.

Maybe the __eq__ and __ne__ should be implemented as methods instead, so, instead of writing F('foo') == F('bar'), you would write F('foo').eq(F('bar')). It is a bit uglier, but should be safe re the above issue.

I don't know too well how Python expects these methods to behave, so it is possible that this is an acceptable way to use __eq__ and __ne__.

by Walter Doekes, 12 years ago

clarify test and add partial docs

comment:13 by anonymous, 12 years ago

If you have overridden all of the other operators, people will expect the == and != to work the same. If you specify .eq or .neq, then someone is going to write .update(field = F('predicate') == F('value') ) and it will set False every time.

The best way to tell if it will conflict with internal django comparisons is to patch it into trunk and run all the tests. If the tests pass, it's all good.

comment:14 by Anssi Kääriäinen, 12 years ago

Needs documentation: unset
Needs tests: unset

I discussed this on IRC with Alex, and he said overriding __eq__ in this way is common. I do agree with your point about unexpected results if == is defined differently than the rest of the operators.

So, I think there aren't any design decision to do in this patch any more. If you can get somebody to review the patch, that would be great. If not, I will try to find some time to do that myself.

comment:15 by anonymous, 12 years ago

Who would need to review the patch?

comment:16 by Anssi Kääriäinen, 12 years ago

Basically, a review by anybody else than patch author is OK. See https://docs.djangoproject.com/en/dev/internals/contributing/triaging-tickets/#ready-for-checkin. A review isn't mandatory, but it will help the committer, and thus also help you to get the patch in.

comment:17 by Koen Biermans, 12 years ago

Triage Stage: AcceptedReady for checkin

This looks good to me. I ran the test suite on sqlite and found no regressions.

The patch did not apply perfectly anymore, so I am attaching a new diff.

I think this is ready for checkin.

by Koen Biermans, 12 years ago

wdoekes patch but applying cleanly on current trunk

comment:18 by Simon Charette, 12 years ago

Cc: charette.s@… added
Needs documentation: set
Patch needs improvement: set
Triage Stage: Ready for checkinAccepted
Version: 1.4master

@koenb, this patch is not RFC. I spotted two things worth fixing:

  1. ExpressionNode.__nonzero__ should be replaced by __bool__ and __nonzero__ should alias the latter for python 2.X compatibility.
  2. The version added note in query documentation should be updated to 1.5
  3. A release note should also be added.

Moving back to Accepted.

by twoolie, 12 years ago

koenb's patch, but documentation and compat added.

comment:19 by twoolie, 12 years ago

Needs documentation: unset
Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

charettes: requested changes have been made. Should now be ready for checkin.

comment:20 by Anssi Kääriäinen, 12 years ago

I made some changes to the patch, mostly cosmetic changes to tests and docs.

However, there was one real problem shown by Python 3 tests - now that F() objects override __eq__ they are no longer hashable. This means they are not usable in sets or as keys in dicts. This can be a problem for users. If they are currently using F() expressions in sets, they can no longer do so. In addition this change required some refactoring in sql/expressions.py which used F() expressions in dicts.

This patch is giving me an uneasy feeling because of the __eq__ method. Otherwise I think this should be ready for checkin. We do not have the option to define other comparison methods except __eq__, users will try to use == anyways if other comparisons are implemented.

Tracked in https://github.com/django/django/pull/405

comment:21 by Anssi Kääriäinen <akaariai@…>, 12 years ago

Resolution: fixed
Status: newclosed

In 28abf5f0ebc9d380f25dd278d7ef4642c4504545:

Fixed #16211 -- Added comparison and negation ops to F() expressions

Work done by Walter Doekes and Trac alias knoeb. Reviewed by Simon
Charette.

comment:22 by Anssi Kääriäinen, 12 years ago

Has patch: unset
Resolution: fixed
Severity: NormalRelease blocker
Status: closedreopened

It seems I will have to revert this whole commit. The problem is we define '&' and '|' operators as bitwise and / or. But, other operators we define as boolean operators. This means (F('somefield') < 100) & (F'somefield') > 50) doesn't do what one expects, and downright fails on some databases.

I can see only one solutions apart of just reverting and wontfixing this issue: Create a subclass of F which redefines what '&' and '|' mean.

I will leave this open for a while to see if any ideas pop out. The most likely solution is a revert from 1.5. Then we can revisit this issue for 1.6 and see if we can create something working...

This is causing a failure in expressions tests at least on postgres.

comment:23 by Anssi Kääriäinen, 12 years ago

BTW I can't see the bitwise operators for F() expressions documented anywhere. I think we want to document those...

comment:24 by twoolie, 12 years ago

@akaariai: From the http://docs.python.org/reference/datamodel.html#object.__hash__,

Called by built-in function hash() and for operations on members of hashed collections including set, frozenset, and dict. hash() should return an integer.
User-defined classes have __cmp__() and __hash__() methods by default; with them, all objects compare unequal (except with themselves) and x.__hash__() returns id(x).

This means that even when overriding __eq__() we are not breaking the hashability of F objects, and hence do not break sets or dicts.

As far as using bitwise operators, we're forced to do it due to limitations in python's operator overloading. As far as i'm concerned, this is entirely consistent with Q objects and as long as it's explicitly documented, it should be fine. Really, the only way we can fix this is to make people do (F('somefield')<100).and(F('somefield') >50) every time they want to do && which will be confusing.

I volunteer to write extra docs for the new operators if that's required.

(Random Thoughts) Perhaps what we need is an E object that can take a "normalized" (db independent) expression that we can build F objects from.

comment:25 by Anssi Kääriäinen, 12 years ago

The problem is that in 1.4 we already do define '&' and '|' to behave as bitwise operators. This is not documented, but the behaviour is there and tested. For this feature we need the same '&' and '|' operators to do boolean AND and OR operations in the DB. This is a conflict which I don't see any nice way to resolve. The best would be to have f1.bitand(f2) and f1.bitor(f2). But, if we do this we will be silently breaking existing code using the '&' and '|' operators.

As for __hash__ - from Python 3 docs:

Called by built-in function hash() and for operations on members of hashed collections
including set, frozenset, and dict. __hash__() should return an integer. The only
required property is that objects which compare equal have the same hash value...

The problem is all F-objects compare equal to each other by the __eq__ override, and for that reason we can't implement __hash__.

comment:26 by Anssi Kääriäinen, 12 years ago

Here is my idea how to move this issue forward:

  • Revert the commit
  • Remove '&' and '|' operators from 1.5, implement F.bitand() and F.bitor() instead
  • Reintroduce this feature into 1.6

The idea is that in 1.5 usage of '&' and '|' will fail loudly. If we just replace what '&' and '|' do then there is possibility that the operators still work for upgraders, but they just don't do what they did in 1.4. This is a data-loss issue.

In 1.6 we would have then boolean operators for F(), and '&' and '|' would work like they do for Q-objects.

The '&' and '|' operators aren't documented, or at least I can't find any reference to them. The question is if anybody is actually using them. If not, then we could just replace the meaning of '&' and '|' directly in 1.5. But, as said, I vote for the safe alternative of one release where you can't use '|' and '&' at all.

comment:27 by Walter Doekes, 12 years ago

I wholeheartedly agree that a loud error is preferred over unsafe behaviour changes.

I don't know if there is a precedent for this, but perhaps a temporary SETTING is possible? Have it error loudly, but you can get the future boolean operators enabled. Otherwise you'll have to change your code twice if you meant to have boolean logic.

if not getattr(settings, 'FUTURE_DJANGO16_F_EXPRESSION_BOOLEAN_AND_OR'):
    raise LoudError(("Use FUTURE_DJANGO16_F_EXPRESSION_BOOLEAN_AND_OR if you're not using "
                     "the bitwise capabilities; otherwise use .bitand/.bitor."))
else:
    # proceed as before
    pass

This would enable the majority of users to simply hit a switch.

comment:28 by Anssi Kääriäinen, 12 years ago

I am not a fan of that approach. This means that when you see

F('something')&F('something_else')

you don't actually know what the code is doing without looking at the settings.

comment:29 by Walter Doekes, 12 years ago

Well.. that would only be in 1.5. I would argue that you also wouldn't know what it does without looking at the Django version ;)

comment:30 by Claude Paroz, 12 years ago

I'm +1 with Anssi's plan, I think this is in agreement with the conservative Django stability policy.

comment:31 by Anssi Kääriäinen, 12 years ago

See pull https://github.com/django/django/pull/419 for suggested changes.

comment:32 by twoolie, 12 years ago

So if we want &/| to work in the same way for Q and F objects then this should be the goal?

SQL              | ORM
-----------------+------------------------------------
field1 &  field2 | F('field1').bitand(F('field2'))
field1 && field2 | F('field1') & F('field2')
field1 |  field2 | F('field1').bitor(F('field2'))
field1 || field2 | F('field1') | F('field2')

p.s. Did &/| even work in 1.4?

( Side note: why is django not championing http://www.python.org/dev/peps/pep-0335/ ?? )

Last edited 12 years ago by twoolie (previous) (diff)

comment:33 by Walter Doekes, 12 years ago

@twoolie: not entirely.

|| is actually the concatenation operator in SQL. In MySQL it works (unless you run sql_mode = 'ANSI'), but in others it doesn't.

postgres:

=> select 'abc' || 'def';
abcdef
=> select (1 < 2) && (2 > 1);
ERROR:  operator does not exist: boolean && boolean

AND and OR should work in all:

=> select (1 < 2) and (2 > 1);
t
=> select (1 < 2) or (2 > 200);
t

@twoolie: championing a rejected PEP?

@akaariai: pull 419 looks ok to me.

comment:34 by twoolie, 12 years ago

@wdokes: Pep-335 looked like it would be revived end of last year when they were working on PY3. You must admit that it would be very handy to have exactly for situations such as this.

comment:35 by Anssi Kääriäinen, 12 years ago

Yeah, it is a little bit ugly that we need to use the bitwise operators for their boolean meaning. But, that seems the only way forward if we want to support boolean operators for F() expressions. And, it seems we want that, this will allow using .update() in new situations with little added code.

Even if we wouldn't want boolean operators for F() expressions it is questionable if using '&' for different meaning in Q() and F() expressions is a good API.

I believe the users of '&' and '|' in their bitwise meaning are extremely rare, so this shouldn't be a big issue for most users.

I will commit the removal of '&' and '|' soon.

comment:36 by Anssi Kääriäinen <akaariai@…>, 12 years ago

Resolution: fixed
Status: reopenedclosed

In a8b1861fc4d0a48b4879af803bba094eef145017:

Revert "Fixed #16211 -- Added comparison and negation ops to F() expressions"

This reverts commit 28abf5f0ebc9d380f25dd278d7ef4642c4504545.

Conflicts:

docs/releases/1.5.txt

comment:37 by Anssi Kääriäinen <akaariai@…>, 12 years ago

In b625e8272bd41714c838cfda3fb54e1f5177f009:

Moved F() '&' and '|' to .bitand() and .bitor()

Done for consistency with Q() expressions and QuerySet combining. This
will allow usage of '&' and '|' as boolean logical operators in the
future. Refs #16211.

comment:38 by Anssi Kääriäinen, 12 years ago

Resolution: fixed
Severity: Release blockerNormal
Status: closedreopened
Triage Stage: Ready for checkinAccepted

The commits above were reverts, so reopening...

The current status is that once 1.5 branch is created we can reapply the reverted patches (with minor modifications, of course).

comment:39 by Anssi Kääriäinen <akaariai@…>, 12 years ago

In 041ef9ed68722fa5f8c38c9e39fad67714f35014:

Removed some uses of F() expression & and |

Refs #16211

comment:40 by Michael Manfre, 12 years ago

For future reference, not all databases (mssql) support the requested behavior.

The usage of NOT in the reverted commit 28abf5f0ebc9d380f25dd278d7ef4642c4504545, UPDATE [expressions_company] SET [is_large] = NOT (([expressions_company].[num_employees] < ?)), is not valid for MSSQL. "Incorrect syntax near the keyword 'NOT'".

comment:41 by Anssi Kääriäinen, 12 years ago

Interesting. To me it seems this should be standard SQL. Does adding more parentheses affect this at all? Is there some other way to write the equivalent query in MSSQL?

comment:42 by twoolie, 12 years ago

SQLServer does not have a BOOLEAN datatype. The closest analogue is the bit field. (http://stackoverflow.com/a/1777277/234254).

This means we probably need something like
UPDATE [expressions_company] SET [is_large] = ~([expressions_company].[num_employees] < ?) to do a bitwise not for MSSQL.

comment:43 by Anssi Kääriäinen, 11 years ago

If anybody is willing to do a PR (or just attach a patch) we could now add the new behaviour to master.

comment:44 by Aymeric Augustin, 11 years ago

Status: reopenednew

comment:45 by Simon Charette, 20 months ago

If anyone is interested in implementing this feature now that the landscape has change quite a bit with the introduction of Expression I'd suggest doing the following.

  • Introduce a NegatedExpression(ExpressionWrapper) class that Combinable.__invert__(self) returns as NegatedExpression(self)
  • NegatedExpression.as_sql should return sql = "NOT {super().as_sql(...)}
  • NegatedExpression.__invert__ should return self.expression
  • NegatedExpression.output_field should be BooleanField
  • Possibly have Expression.__invert__ error out if not self.conditional?
  • Remove the custom Exists.__invert__
  • Add tests for the whole thing

All the other usage cases mentioned in this ticket seem to already be covered by Combinable additions in the past years.

comment:46 by David Wobrock, 19 months ago

Cc: David Wobrock added

comment:47 by David Wobrock, 19 months ago

Has patch: set
Needs tests: set
Owner: changed from nobody to David Wobrock
Patch needs improvement: set
Status: newassigned

Started working on a patch: PR

comment:48 by David Wobrock, 19 months ago

Needs tests: unset
Patch needs improvement: unset

comment:49 by Mariusz Felisiak, 19 months ago

Needs documentation: set
Needs tests: set

comment:50 by David Wobrock, 19 months ago

Needs documentation: unset
Needs tests: unset

comment:51 by Mariusz Felisiak, 18 months ago

Patch needs improvement: set

comment:52 by Mariusz Felisiak, 18 months ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:53 by Mariusz Felisiak, 18 months ago

Resolution: fixed
Status: assignedclosed

comment:54 by Andreas Galazis, 14 months ago

Just a quick update on this you can always use excludes to negate filters(not annotation) in versions before this fix and I think it's even better since as far as I can see the new negated expression uses case statement instead of negation. I one thing I would like to see, is the new negation expression without case statement.

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