Opened 4 years ago

Last modified 2 years ago

#16211 new New feature

using negated F()-expression in update query

Reported by: wdoekes Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: walter+django@…, s.angel@…, charette.s@… Triage Stage: Accepted
Has patch: no 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 wdoekes 4 years ago.
issue16211-query-expression-logical-operators.patch (9.9 KB) - added by wdoekes 4 years ago.
issue16211+14029-query-expression-extra-operators.patch (10.6 KB) - added by wdoekes 4 years ago.
issue16211+14029-query-expression-extra-operators2.patch (11.4 KB) - added by wdoekes 3 years ago.
clarify test and add partial docs
issue16211+14029-query-expression-extra-operators3.diff (11.6 KB) - added by koenb 3 years ago.
wdoekes patch but applying cleanly on current trunk
issue16211+14029-query-expression-extra-operators4.diff (12.3 KB) - added by twoolie 2 years ago.
koenb's patch, but documentation and compat added.

Download all attachments as: .zip

Change History (50)

Changed 4 years ago by wdoekes

comment:1 Changed 4 years ago by aaugustin

  • Has patch set
  • Needs documentation set
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 4 years ago by aaugustin

  • Component changed from Uncategorized to Database layer (models, ORM)
  • Type changed from Uncategorized to New feature

comment:3 Changed 4 years ago by anonymous

This is so indispensable !

comment:4 Changed 4 years ago by s.angel@…

  • Cc s.angel@… added

comment:5 Changed 4 years ago by wdoekes

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 Changed 4 years ago by ramiro

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

comment:7 Changed 4 years ago by wdoekes

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 Changed 3 years ago by wdoekes

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

comment:9 Changed 3 years ago by anonymous

  • Version changed from 1.2 to 1.4

Bump. Why is this not merged yet?

comment:10 Changed 3 years ago by aaugustin

For starters the patch needs documentation and tests.

Last edited 3 years ago by aaugustin (previous) (diff)

comment:11 Changed 3 years ago by anonymous

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

comment:12 Changed 3 years ago by akaariai

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

Changed 3 years ago by wdoekes

clarify test and add partial docs

comment:13 Changed 3 years ago by 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.

comment:14 Changed 3 years ago by akaariai

  • 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 Changed 3 years ago by anonymous

Who would need to review the patch?

comment:16 Changed 3 years ago by akaariai

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 Changed 3 years ago by koenb

  • Triage Stage changed from Accepted to Ready 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.

Changed 3 years ago by koenb

wdoekes patch but applying cleanly on current trunk

comment:18 Changed 3 years ago by charettes

  • Cc charette.s@… added
  • Needs documentation set
  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted
  • Version changed from 1.4 to master

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

Changed 2 years ago by twoolie

koenb's patch, but documentation and compat added.

comment:19 Changed 2 years ago by twoolie

  • Needs documentation unset
  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

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

comment:20 Changed 2 years ago by akaariai

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 Changed 2 years ago by Anssi Kääriäinen <akaariai@…>

  • Resolution set to fixed
  • Status changed from new to closed

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 Changed 2 years ago by akaariai

  • Has patch unset
  • Resolution fixed deleted
  • Severity changed from Normal to Release blocker
  • Status changed from closed to reopened

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 Changed 2 years ago by akaariai

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

comment:24 Changed 2 years ago by 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.

comment:25 Changed 2 years ago by akaariai

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 Changed 2 years ago by akaariai

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 Changed 2 years ago by wdoekes

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 Changed 2 years ago by akaariai

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 Changed 2 years ago by wdoekes

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 Changed 2 years ago by claudep

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

comment:31 Changed 2 years ago by akaariai

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

comment:32 Changed 2 years ago by 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/ ?? )

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

comment:33 Changed 2 years ago by wdoekes

@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 Changed 2 years ago by 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.

comment:35 Changed 2 years ago by akaariai

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 Changed 2 years ago by Anssi Kääriäinen <akaariai@…>

  • Resolution set to fixed
  • Status changed from reopened to closed

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 Changed 2 years ago by 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.

comment:38 Changed 2 years ago by akaariai

  • Resolution fixed deleted
  • Severity changed from Release blocker to Normal
  • Status changed from closed to reopened
  • Triage Stage changed from Ready for checkin to Accepted

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 Changed 2 years ago by Anssi Kääriäinen <akaariai@…>

In 041ef9ed68722fa5f8c38c9e39fad67714f35014:

Removed some uses of F() expression & and |

Refs #16211

comment:40 Changed 2 years ago by 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'".

comment:41 Changed 2 years ago by akaariai

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 Changed 2 years ago by 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.

comment:43 Changed 2 years ago by akaariai

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

comment:44 Changed 2 years ago by aaugustin

  • Status changed from reopened to new
Note: See TracTickets for help on using tickets.
Back to Top