using negated F()-expression in update query

[Walter Doekes]

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.

[anonymous]

This is so indispensable !

[Walter Doekes]

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.

[Ramiro Morales]

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

[Walter Doekes]

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.')

[Walter Doekes]

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

[anonymous]

Bump. Why is this not merged yet?

[Aymeric Augustin]

For starterts the patch needs documentation and tests.

[anonymous]

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

[Anssi Kääriäinen]

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__.

[Walter Doekes]

clarify test and add partial docs

[anonymous]

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.

[Anssi Kääriäinen]

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.

[anonymous]

Who would need to review the patch?

[Anssi Kääriäinen]

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.

[Koen Biermans]

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.

[Koen Biermans]

wdoekes patch but applying cleanly on current trunk

[Simon Charette]

@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.

[twoolie]

koenb's patch, but documentation and compat added.

[twoolie]

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

[Anssi Kääriäinen]

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

[Anssi Kääriäinen <akaariai@…>]

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.

[Anssi Kääriäinen]

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.

[Anssi Kääriäinen]

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

[twoolie]

@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.

[Anssi Kääriäinen]

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__.

[Anssi Kääriäinen]

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.

[Walter Doekes]

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.

[Anssi Kääriäinen]

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.

[Walter Doekes]

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 ;)

[Claude Paroz]

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

[Anssi Kääriäinen]

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

[twoolie]

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/ ?? )

[Walter Doekes]

@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.

[twoolie]

@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.

[Anssi Kääriäinen]

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.

[Anssi Kääriäinen <akaariai@…>]

In a8b1861fc4d0a48b4879af803bba094eef145017:

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

This reverts commit 28abf5f0ebc9d380f25dd278d7ef4642c4504545.

Conflicts:

docs/releases/1.5.txt

[Anssi Kääriäinen <akaariai@…>]

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.

[Anssi Kääriäinen]

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).

[Anssi Kääriäinen <akaariai@…>]

In 041ef9ed68722fa5f8c38c9e39fad67714f35014:

Removed some uses of F() expression & and |

Refs #16211

[Michael Manfre]

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'".

[Anssi Kääriäinen]

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?

[twoolie]

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.

[Anssi Kääriäinen]

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

[Simon Charette]

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.

[David Wobrock]

Started working on a patch: ​PR

[Mariusz Felisiak]

Fixed in a320aab5129f4019b3c1d28b7a3b509582bc56f9.

[Andreas Galazis]

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.