Opened 9 years ago
Closed 8 years ago
#25912 closed New feature (fixed)
Add binary left/right shift operators to F expressions
Reported by: | anabelensc | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 1.9 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Hello,
I think is a good idea to add binary left shift operator and binary right shift operator to make queries.
So the F() objects in addition to bitand and bitor operators, will be able to support binary left shift operator (bitleftshift) and binary right shift operator (bitrightshift).
And then use (bitleftshift) and (bitrightshift) in the same way like the others operators, example the use:
F('somefield').bitleftshift(16)
F('somefield').bitrightshift(16)
I've done a patch to add that and I've run the test suite using SQLite, you can see the files to attach.
Thank you.
Attachments (2)
Change History (16)
by , 9 years ago
Attachment: | add_bitwise_operators.diff added |
---|
comment:1 by , 9 years ago
Component: | Uncategorized → Database layer (models, ORM) |
---|---|
Has patch: | set |
Needs documentation: | set |
Summary: | New feature Bitwise operators ( Binary Left Shift Operator and Binary Right Shift Operator) → Add binary left/right shift operators to F expressions |
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → New feature |
comment:2 by , 9 years ago
There's a slight problem here that we'll probably need to take to the mailing list. Oracle (surprise surprise) is the only supported database that does *not* have a bitshift operator. It doesn't even have a bitshift function that I can find.
We'll need to decide whether we want to leave a backend behind (NotSupportedError or whatever..), figure out a way to support bit shifting by doing the actual math (bitshift by doing [*/] 2**X
) in an as_oracle method, or abandon the idea of supporting bitshifting in django core and using a library to implement some monkey patching.
My preferences here are 2 (do the math for oracle), 3 (leave it out), 1 (no support). Doing the math will result in some overhead (a super() method call) for every F() expression on oracle, whether or not you're doing bitshifting. Unsure if doing the math is actually equivalent in all cases or not though, so we'd need some more robust testing for corner cases.
by , 9 years ago
Attachment: | added_bit_wise_shift_oprators.diff added |
---|
comment:4 by , 9 years ago
Hello,
I have done the same for .bitor() in Django.
.bitor() is not support in Oracle, and in the code of Django raise NotImplementedError, I think to do that with bit shift operators is a good option to try to be consistent with current design in Django.
Thank you.
comment:5 by , 9 years ago
What makes you think that bitor is not supported for Oracle? Where in the code do you see that?
comment:6 by , 9 years ago
Hello,
In django/django/db/backends/oracle/features.py
supports_bitwise_or = False
In django/django/db/backends/oracle/operations.py
def combine_expression(self, connector, sub_expressions): .... elif connector == '|': raise NotImplementedError("Bit-wise or is not supported in Oracle.")
Thank you.
comment:7 by , 9 years ago
Django has BITAND function see https://docs.oracle.com/cd/B19306_01/server.102/b14200/functions014.htm for further info.
BITAND can be used to form OR and XOR queries:
BITOR(X, Y) = X + Y - BITAND(X, Y)
BITXOR(X, Y) = BITOR(X,Y) - BITAND(X,Y)
comment:8 by , 9 years ago
Yes I was just looking at the same thing. Similarly, it's possible to implement the result of bit shifting by multiplication or division of 2^X
as previously mentioned.
I don't think it's right to raise NotImplemented if it's not too difficult to implement.
@anabelensc, could you please open a pull request with your patch as Tim requested? Additionally, take a look at PatchReviewChecklist because there's some tidying up and release notes that'll also need to accompany the patch.
At least if there's a pull request someone might step up to do the oracle implementation while it sits in queue. Ideally though, it'd be great if you could add the oracle support.
comment:9 by , 9 years ago
Needs documentation: | unset |
---|---|
Patch needs improvement: | set |
Pull request -- by the way, there's no need to also attach the patch in Trac.
comment:10 by , 9 years ago
I'm wondering if adding more operations to F() is a good idea. While I think an API like Equals(F('some_string_field').substr(0, 20).lower(), 'anssi') is great, and similarly F('bitfield').bitleftshift(16) is a great API, the problem is that F() objects aren't extensible by users. So, if we are going to add new operations to F(), this will result in requests to add other common operations to F(), too.
I know bitand() and bitor() are already implemented on F(), so that gives some precedent for adding shift operations, too. But maybe we should instead remove .bitand() and .bitor() and implement them as expressions instead.
The main question is this: if we were to design things from scratch, would we add bitwise operations to F(), but not the really common operations like string operations?
I am not opposing the patch, but I do wish that we think what operations we want to expose on F() *before* we commit this one. The choices seem to be:
1) Move bitand() and bitor() to expressions
2) Add bit shift methods, but stop there
3) Allow addition of other methods, too
I don't like 3), as that means Django core is controlling the list of official F-methods. If users were able to add their own methods to F (AKA callable transforms), then I wouldn't have a problem with 3).
comment:11 by , 8 years ago
I opened a thread on django-developers about this ticket, to try to verify if this is the desired implementation.
comment:12 by , 8 years ago
I made a new PR to add Oracle support, however, there's a test failure due to MySQL's unusual handling of negative numbers (resolution TBD).
Seems reasonable. A mention in
topics/db/queries.txt
and the 1.10 release notes are also needed. See the PatchReviewChecklist for tips. If you can provide the patch as a pull request, that's ideal for reviewing purposes.