Merge lp:~radix/storm/count-limited-union into lp:storm

[Christopher Armstrong]

Can someone explain to me what this branch breaks?

I have some code that I would love to be able to do a result1.union(result2).order_by(c).config(limit=n).count() in, but there's an arbitrary restriction preventing it from working. Given the comment, it sounds like there's some case in which it might not be supported, but the simple case I have works just fine if I remove the assertion.

By the way, the error message it raised was really confusing; it looks like replace_columns assumes it's only being called in a __contains__ code path, when it's also called elsewhere.

[Thomas Herve (therve)]

The message is definitely outdated, by only mentioning __contains__, but it seems to me that's it's still valid: the order_by value is not passed along, so it can't be used by the new expression? Your test doesn't check that as there is only one value.

[lp:~radix/storm/count-limited-union]

455. By Christopher Armstrong on 2012-12-14

try a different strategy for avoiding the error.

[Christopher Armstrong (radix)]

The ordering doesn't matter for this case. I've talked to therve a bit and have changed the strategy used in the branch a bit to be a bit more explicit and less far-reaching.

[Thomas Herve (therve)]

It looks good to me. Maybe add another test for another aggregation function to make sure everything is fine. +1!

review: Approve

[lp:~radix/storm/count-limited-union]

456. By Christopher Armstrong on 2012-12-18

add a test for avg() as well, to ensure the fix works for all aggregates.

[Christopher Armstrong (radix)]

Done. I'm not a storm committer, can someone take care of merging this?

=== modified file 'storm/store.py'
--- storm/store.py	2012-06-12 10:00:51 +0000
+++ storm/store.py	2012-12-18 21:23:38 +0000
@@ -1244,7 +1244,11 @@
                 alias = Alias(expr, "_expr")
                 columns.append(alias)
                 aggregate = aggregate_func(alias)
-            subquery = replace_columns(self._get_select(), columns)
+            # Ordering probably doesn't matter for any aggregates, and since
+            # replace_columns() blows up on an ordered query, we'll drop it.
+            select = self._get_select()
+            select.order_by = Undef
+            subquery = replace_columns(select, columns)
             select = Select(aggregate, tables=Alias(subquery, "_tmp"))
         result = self._store._connection.execute(select)
         value = result.get_one()[0]

=== modified file 'tests/store/base.py'
--- tests/store/base.py	2012-04-06 08:19:07 +0000
+++ tests/store/base.py	2012-12-18 21:23:38 +0000
@@ -5583,6 +5583,35 @@
 
         self.assertEquals(result3.count(), 2)
 
+    def test_result_union_limit_count(self):
+        """
+        It's possible to count the result of a union that is limited.
+        """
+        result1 = self.store.find(Foo, id=30)
+        result2 = self.store.find(Foo, id=30)
+
+        result3 = result1.union(result2, all=True)
+        result3.order_by(Foo.id)
+        result3.config(limit=1)
+
+        self.assertEquals(result3.count(), 1)
+        self.assertEquals(result3.count(Foo.id), 1)
+
+    def test_result_union_limit_avg(self):
+        """
+        It's possible to average the result of a union that is limited.
+        """
+        result1 = self.store.find(Foo, id=10)
+        result2 = self.store.find(Foo, id=30)
+
+        result3 = result1.union(result2, all=True)
+        result3.order_by(Foo.id)
+        result3.config(limit=1)
+
+        # Since 30 was left off because of the limit, the only result will be
+        # 10, and the average of that is 10.
+        self.assertEquals(result3.avg(Foo.id), 10)
+
     def test_result_difference(self):
         if self.__class__.__name__.startswith("MySQL"):
             return