Merge lp:~cjwatson/storm/is-and-is-not-operators into lp:storm

Proposed by Colin Watson
Status: Needs review
Proposed branch: lp:~cjwatson/storm/is-and-is-not-operators
Merge into: lp:storm
Diff against target: 233 lines (+173/-0)
3 files modified
NEWS (+1/-0)
storm/expr.py (+70/-0)
storm/tests/expr.py (+102/-0)
To merge this branch: bzr merge lp:~cjwatson/storm/is-and-is-not-operators
Reviewer Review Type Date Requested Status
Andrey Fedoseev (community) Approve
Review via email: mp+431754@code.launchpad.net

Commit message

Add storm.expr.Is and storm.expr.IsNot operators.

Description of the change

Launchpad has had custom `IsTrue` and `IsFalse` operators for a while, because there are a few places where `expr` or `expr == True` (or the equivalents for False) don't quite do what we need: for example, if you're working with a nullable boolean column, then SQL NULL semantics mean that `expr = TRUE` or `expr = FALSE` return NULL when `expr` is NULL, which can be undesirable at times.

In addition, `==` and `!=` comparisons with True/False/None are typically picked up as errors by modern linters, since in normal Python code they should use `is` or `is not` or simple boolean tests instead. Having to override this is an annoyance.

Compare https://docs.sqlalchemy.org/en/20/core/operators.html#identity-comparisons, although SQLAlchemy is more permissive; it seems to be happy to compile anything on the right-hand-side of `IS` or `IS NOT` and leave it up to the database. Since we already have to take some care with how booleans are rendered, and there's no guarantee that the spelling of `IS TRUE` etc. is going to match the way that boolean values themselves are spelled, I thought it better to limit the set of right-hand-side expressions that we will compile.

I've checked that current versions of all the databases currently supported by Storm (PostgreSQL, MySQL, and SQLite) support all the resulting expressions, although in the case of SQLite it does need at least version 3.23.0 (released in 2018).

To post a comment you must log in.
Revision history for this message
Andrey Fedoseev (andrey-fedoseev) :
review: Approve

Unmerged revisions

577. By Colin Watson

Add storm.expr.Is and storm.expr.IsNot operators.

Launchpad has had custom `IsTrue` and `IsFalse` operators for a while,
because there are a few places where `expr` or `expr == True` (or the
equivalents for False) don't quite do what we need: for example, if
you're working with a nullable boolean column, then SQL NULL semantics
mean that `expr = TRUE` or `expr = FALSE` return NULL when `expr` is
NULL, which can be undesirable at times.

In addition, `==` and `!=` comparisons with True/False/None are
typically picked up as errors by modern linters, since in normal Python
code they should use `is` or `is not` or simple boolean tests instead.
Having to override this is an annoyance.

Compare
https://docs.sqlalchemy.org/en/20/core/operators.html#identity-comparisons,
although SQLAlchemy is more permissive; it seems to be happy to compile
anything on the right-hand-side of `IS` or `IS NOT` and leave it up to
the database. Since we already have to take some care with how booleans
are rendered, and there's no guarantee that the spelling of `IS TRUE`
etc. is going to match the way that boolean values themselves are
spelled, I thought it better to limit the set of right-hand-side
expressions that we will compile.

I've checked that current versions of all the databases currently
supported by Storm (PostgreSQL, MySQL, and SQLite) support all the
resulting expressions, although in the case of SQLite it does need at
least version 3.23.0 (released in 2018).

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2022-03-16 16:59:21 +0000
3+++ NEWS 2022-10-18 17:38:52 +0000
4@@ -6,6 +6,7 @@
5
6 - Clarify exception when creating a property with both allow_none=False and
7 default=None.
8+- Add storm.expr.Is and storm.expr.IsNot operators.
9
10 Bug fixes
11 ---------
12
13=== modified file 'storm/expr.py'
14--- storm/expr.py 2020-12-07 22:38:36 +0000
15+++ storm/expr.py 2022-10-18 17:38:52 +0000
16@@ -1047,6 +1047,74 @@
17 return compile(expr.exprs, state, join=expr.oper.lower())
18
19
20+class Is(BinaryOper):
21+ """The SQL C{IS ...} operators, e.g. C{IS NULL}.
22+
23+ C{Is(expr, None)} is synonymous with C{expr == None}, but is less likely
24+ to trip up linters.
25+
26+ Unlike C{expr} or C{expr == True}, C{Is(expr, True)} returns C{FALSE}
27+ when C{expr} is C{NULL}.
28+
29+ Unlike C{Not(expr)} or C{expr == False}, C{Is(expr, False)} returns
30+ C{FALSE} when C{expr} is C{NULL}.
31+ """
32+ __slots__ = ()
33+ oper = " IS "
34+
35+@compile.when(Is)
36+def compile_is(compile, is_, state):
37+ tokens = [compile(is_.expr1, state), "IS"]
38+ if is_.expr2 is None:
39+ tokens.append("NULL")
40+ elif is_.expr2 is True:
41+ tokens.append("TRUE")
42+ elif is_.expr2 is False:
43+ tokens.append("FALSE")
44+ else:
45+ raise CompileError("expr2 must be None, True, or False")
46+ return " ".join(tokens)
47+
48+@compile_python.when(Is)
49+def compile_is(compile, is_, state):
50+ return "%s is %s" % (compile(is_.expr1, state), compile(is_.expr2, state))
51+
52+
53+class IsNot(BinaryOper):
54+ """The SQL C{IS NOT ...} operators, e.g. C{IS NOT NULL}.
55+
56+ C{IsNot(expr, None)} is synonymous with C{expr != None}, but is less
57+ likely to trip up linters.
58+
59+ Unlike C{Not(expr)} or C{expr != True}, C{IsNot(expr, True)} returns
60+ C{TRUE} when C{expr} is C{NULL}.
61+
62+ Unlike C{expr} or C{expr != False}, C{IsNot(expr, False)} returns
63+ C{TRUE} when C{expr} is C{NULL}.
64+ """
65+ __slots__ = ()
66+ oper = " IS NOT "
67+
68+@compile.when(IsNot)
69+def compile_is_not(compile, is_not, state):
70+ tokens = [compile(is_not.expr1, state), "IS NOT"]
71+ if is_not.expr2 is None:
72+ tokens.append("NULL")
73+ elif is_not.expr2 is True:
74+ tokens.append("TRUE")
75+ elif is_not.expr2 is False:
76+ tokens.append("FALSE")
77+ else:
78+ raise CompileError("expr2 must be None, True, or False")
79+ return " ".join(tokens)
80+
81+@compile_python.when(IsNot)
82+def compile_is_not(compile, is_not, state):
83+ return "%s is not %s" % (
84+ compile(is_not.expr1, state), compile(is_not.expr2, state)
85+ )
86+
87+
88 class Eq(BinaryOper):
89 __slots__ = ()
90 oper = " = "
91@@ -1553,6 +1621,7 @@
92 compile.set_precedence(20, SQL)
93 compile.set_precedence(30, Or)
94 compile.set_precedence(40, And)
95+compile.set_precedence(45, Is, IsNot)
96 compile.set_precedence(50, Eq, Ne, Gt, Ge, Lt, Le, Like, In)
97 compile.set_precedence(60, LShift, RShift)
98 compile.set_precedence(70, Add, Sub)
99@@ -1560,6 +1629,7 @@
100
101 compile_python.set_precedence(10, Or)
102 compile_python.set_precedence(20, And)
103+compile_python.set_precedence(25, Is, IsNot)
104 compile_python.set_precedence(30, Eq, Ne, Gt, Ge, Lt, Le, Like, In)
105 compile_python.set_precedence(40, LShift, RShift)
106 compile_python.set_precedence(50, Add, Sub)
107
108=== modified file 'storm/tests/expr.py'
109--- storm/tests/expr.py 2020-12-07 22:38:36 +0000
110+++ storm/tests/expr.py 2022-10-18 17:38:52 +0000
111@@ -247,6 +247,16 @@
112 like_expr = expr.contains_string(u"abc!!_%", case_sensitive=False)
113 self.assertIs(False, like_expr.case_sensitive)
114
115+ def test_is(self):
116+ expr = Is(elem1, elem2)
117+ self.assertEqual(expr.expr1, elem1)
118+ self.assertEqual(expr.expr2, elem2)
119+
120+ def test_is_not(self):
121+ expr = IsNot(elem1, elem2)
122+ self.assertEqual(expr.expr1, elem1)
123+ self.assertEqual(expr.expr2, elem2)
124+
125 def test_eq(self):
126 expr = Eq(elem1, elem2)
127 self.assertEqual(expr.expr1, elem1)
128@@ -1101,6 +1111,56 @@
129 self.assertEqual(statement, "?")
130 self.assertVariablesEqual(state.parameters, [Variable("value")])
131
132+ def test_is_null(self):
133+ expr = Is(Func1(), None)
134+ state = State()
135+ statement = compile(expr, state)
136+ self.assertEqual(statement, "func1() IS NULL")
137+ self.assertEqual(state.parameters, [])
138+
139+ def test_is_true(self):
140+ expr = Is(Func1(), True)
141+ state = State()
142+ statement = compile(expr, state)
143+ self.assertEqual(statement, "func1() IS TRUE")
144+ self.assertEqual(state.parameters, [])
145+
146+ def test_is_false(self):
147+ expr = Is(Func1(), False)
148+ state = State()
149+ statement = compile(expr, state)
150+ self.assertEqual(statement, "func1() IS FALSE")
151+ self.assertEqual(state.parameters, [])
152+
153+ def test_is_invalid(self):
154+ expr = Is(Func1(), "x")
155+ self.assertRaises(CompileError, compile, expr)
156+
157+ def test_is_not_null(self):
158+ expr = IsNot(Func1(), None)
159+ state = State()
160+ statement = compile(expr, state)
161+ self.assertEqual(statement, "func1() IS NOT NULL")
162+ self.assertEqual(state.parameters, [])
163+
164+ def test_is_not_true(self):
165+ expr = IsNot(Func1(), True)
166+ state = State()
167+ statement = compile(expr, state)
168+ self.assertEqual(statement, "func1() IS NOT TRUE")
169+ self.assertEqual(state.parameters, [])
170+
171+ def test_is_not_false(self):
172+ expr = IsNot(Func1(), False)
173+ state = State()
174+ statement = compile(expr, state)
175+ self.assertEqual(statement, "func1() IS NOT FALSE")
176+ self.assertEqual(state.parameters, [])
177+
178+ def test_is_not_invalid(self):
179+ expr = IsNot(Func1(), "x")
180+ self.assertRaises(CompileError, compile, expr)
181+
182 def test_eq(self):
183 expr = Eq(Func1(), Func2())
184 state = State()
185@@ -2273,6 +2333,48 @@
186 self.assertEqual(py_expr, "_0")
187 self.assertEqual(state.parameters, ["value"])
188
189+ def test_is_null(self):
190+ expr = Is(Variable(True), None)
191+ state = State()
192+ statement = compile_python(expr, state)
193+ self.assertEqual(statement, "_0 is None")
194+ self.assertEqual(state.parameters, [True])
195+
196+ def test_is_true(self):
197+ expr = Is(Variable(True), True)
198+ state = State()
199+ statement = compile_python(expr, state)
200+ self.assertEqual(statement, "_0 is _1")
201+ self.assertEqual(state.parameters, [True, True])
202+
203+ def test_is_false(self):
204+ expr = Is(Variable(True), False)
205+ state = State()
206+ statement = compile_python(expr, state)
207+ self.assertEqual(statement, "_0 is _1")
208+ self.assertEqual(state.parameters, [True, False])
209+
210+ def test_is_not_null(self):
211+ expr = IsNot(Variable(True), None)
212+ state = State()
213+ statement = compile_python(expr, state)
214+ self.assertEqual(statement, "_0 is not None")
215+ self.assertEqual(state.parameters, [True])
216+
217+ def test_is_not_true(self):
218+ expr = IsNot(Variable(True), True)
219+ state = State()
220+ statement = compile_python(expr, state)
221+ self.assertEqual(statement, "_0 is not _1")
222+ self.assertEqual(state.parameters, [True, True])
223+
224+ def test_is_not_false(self):
225+ expr = IsNot(Variable(True), False)
226+ state = State()
227+ statement = compile_python(expr, state)
228+ self.assertEqual(statement, "_0 is not _1")
229+ self.assertEqual(state.parameters, [True, False])
230+
231 def test_eq(self):
232 expr = Eq(Variable(1), Variable(2))
233 state = State()

Subscribers

People subscribed via source and target branches

to status/vote changes: