binary 'and' and 'or' operators support

Bug #187047 reported by Thomas Herve
2
Affects Status Importance Assigned to Milestone
Storm
New
Undecided
Unassigned

Bug Description

I have a need for binary 'and' operator to match IP addresses stored in a database. The attached branch implements both operators, with tests. Please review.

Tags: review
Revision history for this message
James Henstridge (jamesh) wrote :

Overall, the patch looks okay to me. A few comments:

 * I wonder if Comparable.__and__() and __or__() doing boolean and/or will be confusing?
 * Would BitwiseAnd and BitwiseOr be better names? That seems to be the names for the operators used here:
    http://docs.python.org/ref/bitwise.html
 * Would it be worth rounding out the remaining bitwise operators? While and/or seem fairly consistently implemented, the others don't so might require per-backend compilation (e.g. xor is "^" in mysql and "#" in postgresql).

Revision history for this message
Thomas Herve (therve) wrote :

Thanks for the comments. I renamed to BitWiseAnd and BitWiseOr, that's indeed more clear. I also added BitWiseXor and BitWiseNot, with support for the 3 databases.

Revision history for this message
James Henstridge (jamesh) wrote :

As bitwise is one word, the names should be "Bitwise" rather than "BitWise".

Also, I am not sure about the precedences you've given to the new operators. You don't seem to set precedence for all operators, and the Python precedences do not match http://docs.python.org/ref/summary.html, which needs to be fixed. I'm not sure where to find precedence information for the SQL side of things (or whether it is the same for each DB).

Revision history for this message
Thomas Herve (therve) wrote :

OK, I had to workaround a bug of bitwisenot under sqlite, but that seems ok now. Thanks again.

Revision history for this message
James Henstridge (jamesh) wrote :

Looks a lot better. My tests on hardy indicate that the "~" operator has equal or lower precedence to "+" (but higher than boolean and/or), so the precedence value you've used seems plausible. Did you check the sqlite docs/source to find what it was set to?

Once someone else checks over your branch, we can get it merged.

Revision history for this message
Thomas Herve (therve) wrote :

Looking at SQLite source code, it's hard to tell what was actually fixed. I'll have to test further.

Revision history for this message
Thomas Herve (therve) wrote :

OK I looked and my first thoughts were good. I added some more tests to check precedence between operators.

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

Nice stuff. Three minor comments which we discussed:

[1]

We have to check the SQL precedence rules, as PostgreSQL seems
to have different rules than the one used in the branch.

[2]

    def compile_bitwise_xor_postgres(compile, expr, state):
        expr.oper = "#"

This changes the expression passed by the user, which may be unexpected.
It'd be nice to just return the compiled statement from this handler.

[3]

    BitwiseXor

BitwiseXOr is probably the most expected spelling, as we discussed live.

Thanks for the branch! +1!

guanlonghuang (jace833)
Changed in storm:
assignee: nobody → guanlonghuang (jace833)
status: New → Confirmed
status: Confirmed → Fix Committed
Colin Watson (cjwatson)
Changed in storm:
status: Fix Committed → New
assignee: guanlonghuang (jace833) → nobody
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.