Merge lp:~jamesh/storm/bug-242813 into lp:storm

Proposed by James Henstridge on 2009-08-21
Status: Merged
Merged at revision: not available
Proposed branch: lp:~jamesh/storm/bug-242813
Merge into: lp:storm
Diff against target: 132 lines
2 files modified
storm/expr.py (+10/-0)
tests/expr.py (+87/-0)
To merge this branch: bzr merge lp:~jamesh/storm/bug-242813
Reviewer Review Type Date Requested Status
Jamu Kakar (community) 2009-08-21 Approve on 2009-10-07
Review via email: mp+10508@code.launchpad.net
To post a comment you must log in.
James Henstridge (jamesh) wrote :

Flatten nested set expressions as they are built so that we are less likely to hit Python's recursion limit when compiling the expression to SQL.

This relies on the left associativity of all the set expressions. (i.e. that Union(Union(a, b), c) is equivalent to Union(a, b, c)).

Jamu Kakar (jkakar) wrote :

Nice and simple, +1.

review: Approve
GabrielGrant (gabrielgrant) wrote :

The tests pass, and this fixes a problem I've run into, so I'm eager to see this pushed into mainline.

A few questions:

Should there be a test to explicitly check that heterogeneous set expressions are not flattened? (ie exercising the negative branch of the second if statement)

Is it necessary to skip expressions containing an order_by? If the order_by is the same on each expr, shouldn't the result of the nested and un-nested expressions be equivalent? (I may very well be totally wrong about this, but I can't seem to cook up a counter-example at the moment)

It appears that in addition to addressing https://bugs.edge.launchpad.net/storm/+bug/242813 this patch also should fix some of https://bugs.edge.launchpad.net/storm/+bug/309276 (which is a problem I'm having), however I don't think it addresses it fully. The proposed solution doesn't tackle heterogeneous nested set queries, which will still fail on MySQL. http://bugs.mysql.com/bug.php?id=3347 http://bugs.mysql.com/bug.php?id=2435 and http://bugs.mysql.com/bug.php?id=7360 seem to imply that there is a problem with MySQL that basically makes some such nested queries impossible to express, however simpler ones like (SELECT ...) UNION (SELECT ...) INTERSECT (SELECT ...) are supported in MySQL, but (I believe) can't be achieved through Storm itself.

Could/should there be a more graceful failure for these cases than a MySQL parse error? (Sorry if this is too far off track. Perhaps this discussion would be better continued on the appropriate bug? I've subscribed myself)

Cheers!

James Henstridge (jamesh) wrote :

> The tests pass, and this fixes a problem I've run into, so I'm eager to see
> this pushed into mainline.
>
> A few questions:
>
> Should there be a test to explicitly check that heterogeneous set expressions
> are not flattened? (ie exercising the negative branch of the second if
> statement)

Yep. There should be a test for that.

> Is it necessary to skip expressions containing an order_by? If the order_by is
> the same on each expr, shouldn't the result of the nested and un-nested
> expressions be equivalent? (I may very well be totally wrong about this, but I
> can't seem to cook up a counter-example at the moment)

I guess I wrote the code that way to be on the safe side: except in the case of MySQL's broken query parser, this branch is mainly designed to improve performance.

That said, the test can probably go. If the outer set expression has an order, then the inner set expression's ordering doesn't matter. If the outer expression has no order, then there is no guarantee on the ordering so we should be able to ignore it in that case too.

I'll remove the check.

> It appears that in addition to addressing
> https://bugs.edge.launchpad.net/storm/+bug/242813 this patch also should fix
> some of https://bugs.edge.launchpad.net/storm/+bug/309276 (which is a problem
> I'm having), however I don't think it addresses it fully. The proposed
> solution doesn't tackle heterogeneous nested set queries, which will still
> fail on MySQL. http://bugs.mysql.com/bug.php?id=3347
> http://bugs.mysql.com/bug.php?id=2435 and
> http://bugs.mysql.com/bug.php?id=7360 seem to imply that there is a problem
> with MySQL that basically makes some such nested queries impossible to
> express, however simpler ones like (SELECT ...) UNION (SELECT ...) INTERSECT
> (SELECT ...) are supported in MySQL, but (I believe) can't be achieved through
> Storm itself.

It might be possible to fix up that sort of chaining by playing with the operator precedences in the expression compiler.

> Could/should there be a more graceful failure for these cases than a MySQL
> parse error? (Sorry if this is too far off track. Perhaps this discussion
> would be better continued on the appropriate bug? I've subscribed myself)

I don't think it is worth the trouble. Writing code to detect expressions that will cause the MySQL query parser trouble is probably as much work as adjusting our expression generators to make expressions that don't trigger bugs in the MySQL query parser.

lp:~jamesh/storm/bug-242813 updated on 2009-10-21
331. By James Henstridge on 2009-10-21

Collapse set expressions with order_by set, and add some extra tests.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'storm/expr.py'
2--- storm/expr.py 2009-07-31 01:53:08 +0000
3+++ storm/expr.py 2009-10-21 09:48:13 +0000
4@@ -1099,6 +1099,16 @@
5 self.order_by = kwargs.get("order_by", Undef)
6 self.limit = kwargs.get("limit", Undef)
7 self.offset = kwargs.get("offset", Undef)
8+ # If the first expression is of a compatible type, directly
9+ # include its sub expressions.
10+ if len(self.exprs) > 0:
11+ first = self.exprs[0]
12+ if (isinstance(first, self.__class__) and
13+ first.all == self.all and
14+ first.limit is Undef and
15+ first.offset is Undef):
16+ self.exprs = first.exprs + self.exprs[1:]
17+
18
19 @compile.when(SetExpr)
20 def compile_set_expr(compile, expr, state):
21
22=== modified file 'tests/expr.py'
23--- tests/expr.py 2009-07-23 22:47:10 +0000
24+++ tests/expr.py 2009-10-21 09:48:13 +0000
25@@ -275,6 +275,35 @@
26 self.assertEquals(expr.limit, 1)
27 self.assertEquals(expr.offset, 2)
28
29+ def test_union_collapse(self):
30+ expr = Union(Union(elem1, elem2), elem3)
31+ self.assertEquals(expr.exprs, (elem1, elem2, elem3))
32+
33+ # Only first expression is collapsed.
34+ expr = Union(elem1, Union(elem2, elem3))
35+ self.assertEquals(expr.exprs[0], elem1)
36+ self.assertTrue(isinstance(expr.exprs[1], Union))
37+
38+ # Don't collapse if all is different.
39+ expr = Union(Union(elem1, elem2, all=True), elem3)
40+ self.assertTrue(isinstance(expr.exprs[0], Union))
41+ expr = Union(Union(elem1, elem2), elem3, all=True)
42+ self.assertTrue(isinstance(expr.exprs[0], Union))
43+ expr = Union(Union(elem1, elem2, all=True), elem3, all=True)
44+ self.assertEquals(expr.exprs, (elem1, elem2, elem3))
45+
46+ # Don't collapse if limit or offset are set.
47+ expr = Union(Union(elem1, elem2, limit=1), elem3)
48+ self.assertTrue(isinstance(expr.exprs[0], Union))
49+ expr = Union(Union(elem1, elem2, offset=3), elem3)
50+ self.assertTrue(isinstance(expr.exprs[0], Union))
51+
52+ # Don't collapse other set expressions.
53+ expr = Union(Except(elem1, elem2), elem3)
54+ self.assertTrue(isinstance(expr.exprs[0], Except))
55+ expr = Union(Intersect(elem1, elem2), elem3)
56+ self.assertTrue(isinstance(expr.exprs[0], Intersect))
57+
58 def test_except(self):
59 expr = Except(elem1, elem2, elem3)
60 self.assertEquals(expr.exprs, (elem1, elem2, elem3))
61@@ -287,6 +316,35 @@
62 self.assertEquals(expr.limit, 1)
63 self.assertEquals(expr.offset, 2)
64
65+ def test_except_collapse(self):
66+ expr = Except(Except(elem1, elem2), elem3)
67+ self.assertEquals(expr.exprs, (elem1, elem2, elem3))
68+
69+ # Only first expression is collapsed.
70+ expr = Except(elem1, Except(elem2, elem3))
71+ self.assertEquals(expr.exprs[0], elem1)
72+ self.assertTrue(isinstance(expr.exprs[1], Except))
73+
74+ # Don't collapse if all is different.
75+ expr = Except(Except(elem1, elem2, all=True), elem3)
76+ self.assertTrue(isinstance(expr.exprs[0], Except))
77+ expr = Except(Except(elem1, elem2), elem3, all=True)
78+ self.assertTrue(isinstance(expr.exprs[0], Except))
79+ expr = Except(Except(elem1, elem2, all=True), elem3, all=True)
80+ self.assertEquals(expr.exprs, (elem1, elem2, elem3))
81+
82+ # Don't collapse if limit or offset are set.
83+ expr = Except(Except(elem1, elem2, limit=1), elem3)
84+ self.assertTrue(isinstance(expr.exprs[0], Except))
85+ expr = Except(Except(elem1, elem2, offset=3), elem3)
86+ self.assertTrue(isinstance(expr.exprs[0], Except))
87+
88+ # Don't collapse other set expressions.
89+ expr = Except(Union(elem1, elem2), elem3)
90+ self.assertTrue(isinstance(expr.exprs[0], Union))
91+ expr = Except(Intersect(elem1, elem2), elem3)
92+ self.assertTrue(isinstance(expr.exprs[0], Intersect))
93+
94 def test_intersect(self):
95 expr = Intersect(elem1, elem2, elem3)
96 self.assertEquals(expr.exprs, (elem1, elem2, elem3))
97@@ -300,6 +358,35 @@
98 self.assertEquals(expr.limit, 1)
99 self.assertEquals(expr.offset, 2)
100
101+ def test_intersect_collapse(self):
102+ expr = Intersect(Intersect(elem1, elem2), elem3)
103+ self.assertEquals(expr.exprs, (elem1, elem2, elem3))
104+
105+ # Only first expression is collapsed.
106+ expr = Intersect(elem1, Intersect(elem2, elem3))
107+ self.assertEquals(expr.exprs[0], elem1)
108+ self.assertTrue(isinstance(expr.exprs[1], Intersect))
109+
110+ # Don't collapse if all is different.
111+ expr = Intersect(Intersect(elem1, elem2, all=True), elem3)
112+ self.assertTrue(isinstance(expr.exprs[0], Intersect))
113+ expr = Intersect(Intersect(elem1, elem2), elem3, all=True)
114+ self.assertTrue(isinstance(expr.exprs[0], Intersect))
115+ expr = Intersect(Intersect(elem1, elem2, all=True), elem3, all=True)
116+ self.assertEquals(expr.exprs, (elem1, elem2, elem3))
117+
118+ # Don't collapse if limit or offset are set.
119+ expr = Intersect(Intersect(elem1, elem2, limit=1), elem3)
120+ self.assertTrue(isinstance(expr.exprs[0], Intersect))
121+ expr = Intersect(Intersect(elem1, elem2, offset=3), elem3)
122+ self.assertTrue(isinstance(expr.exprs[0], Intersect))
123+
124+ # Don't collapse other set expressions.
125+ expr = Intersect(Union(elem1, elem2), elem3)
126+ self.assertTrue(isinstance(expr.exprs[0], Union))
127+ expr = Intersect(Except(elem1, elem2), elem3)
128+ self.assertTrue(isinstance(expr.exprs[0], Except))
129+
130 def test_auto_tables(self):
131 expr = AutoTables(elem1, [elem2])
132 self.assertEquals(expr.expr, elem1)

Subscribers

People subscribed via source and target branches

to status/vote changes: