launchpad-reviewers team mailing list archive

[Abel Deuring <abel.deuring@xxxxxxxxxxxxx>]

Abel Deuring has proposed merging lp:~adeuring/launchpad/bug-739052-3 into lp:launchpad with lp:~adeuring/launchpad/bug-739052-2 as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~adeuring/launchpad/bug-739052-3/+merge/70272

StormRangeFactory: This branch implements sort expressions like (col1, col2...) > (memo1, momo2...) if all sort columns use only ascending or only descending order.

./bin/test canonical -vvt canonical.launchpad.webapp.tests.test_batching.TestStormRangeFactory

no lint

-- 
https://code.launchpad.net/~adeuring/launchpad/bug-739052-3/+merge/70272
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/bug-739052-3 into lp:launchpad.

=== modified file 'lib/canonical/launchpad/webapp/batching.py'
--- lib/canonical/launchpad/webapp/batching.py	2011-08-03 10:21:35 +0000
+++ lib/canonical/launchpad/webapp/batching.py	2011-08-03 10:21:37 +0000
@@ -11,8 +11,10 @@
 from storm import Undef
 from storm.expr import (
     And,
+    compile,
     Desc,
     Or,
+    SQL,
     )
 from storm.properties import PropertyColumn
 from storm.zope.interfaces import IResultSet
@@ -26,6 +28,7 @@
     )
 
 from canonical.config import config
+from canonical.database.sqlbase import sqlvalues
 from canonical.launchpad.components.decoratedresultset import (
     DecoratedResultSet,
     )
@@ -342,7 +345,7 @@
                 return expression
         return [plain_expression(column) == memo for column, memo in limits]
 
-    def whereExpressions(self, limits):
+    def genericWhereExpressions(self, limits):
         """Generate WHERE expressions for the given sort columns and
         memos values.
 
@@ -371,14 +374,43 @@
             clauses = self.andClausesForLeadingColumns(start)
             clauses.append(last_limit)
             clause_for_last_column = reduce(And, clauses)
-            return [clause_for_last_column] + self.whereExpressions(start)
+            return (
+                [clause_for_last_column]
+                + self.genericWhereExpressions(start))
         else:
             return [last_limit]
 
+    def whereExpressions(self, sort_expressions, memos):
+        """WHERE expressions for the given sort columns and memos values."""
+        expression = sort_expressions[0]
+        descending = isinstance(expression, Desc)
+        consistent = True
+        for expression in sort_expressions[1:]:
+            if isinstance(expression, Desc) != descending:
+                consistent = False
+                break
+        if not consistent or len(sort_expressions) == 1:
+            return self.genericWhereExpressions(zip(sort_expressions, memos))
+
+        # If the columns are sorted either only ascending or only
+        # descending, we can specify a single WHERE condition
+        # (col1, col2...) > (memo1, memo2...)
+        if descending:
+            sort_expressions = [
+                expression.expr for expression in sort_expressions]
+        sort_expressions = map(compile, sort_expressions)
+        sort_expressions = ', '.join(sort_expressions)
+        memos = sqlvalues(*memos)
+        memos = ', '.join(memos)
+        if descending:
+            return [SQL('(%s) < (%s)' % (sort_expressions, memos))]
+        else:
+            return [SQL('(%s) > (%s)' % (sort_expressions, memos))]
+
     def getSliceFromMemo(self, size, memo):
         """Return a result set for the given memo values.
 
-        Note that at least two other implementatians are possible:
+        Note that at least two other implementations are possible:
         Instead of OR-combining the expressions returned by
         whereExpressions(), these expressions could be used for
         separate SELECTs which are then merged with UNION ALL.
@@ -390,7 +422,7 @@
         differ between different queries.
         """
         sort_expressions = self.getOrderBy()
-        where = self.whereExpressions(zip(sort_expressions, memo))
+        where = self.whereExpressions(sort_expressions, memo)
         where = reduce(Or, where)
         # From storm.zope.interfaces.IResultSet.__doc__:
         #     - C{find()}, C{group_by()} and C{having()} are really

=== modified file 'lib/canonical/launchpad/webapp/tests/test_batching.py'
--- lib/canonical/launchpad/webapp/tests/test_batching.py	2011-08-03 10:21:35 +0000
+++ lib/canonical/launchpad/webapp/tests/test_batching.py	2011-08-03 10:21:37 +0000
@@ -93,7 +93,7 @@
             StormRangeFactoryError, range_factory.getOrderValuesFor,
             resultset[0])
         self.assertEqual(
-            "StormRangeFactory supports only sorting by PropertyColumn, "
+            "StormRangeFactory only supports sorting by PropertyColumn, "
             "not by 'Person.id'.",
             str(exception))
 
@@ -106,7 +106,7 @@
             resultset[0])
         self.assertTrue(
             str(exception).startswith(
-                'StormRangeFactory supports only sorting by PropertyColumn, '
+                'StormRangeFactory only supports sorting by PropertyColumn, '
                 'not by <storm.expr.SQL object at'))
 
     def test_getOrderValuesFor__unordered_result_set(self):
@@ -358,7 +358,7 @@
         """
         resultset = self.makeStormResultSet()
         range_factory = StormRangeFactory(resultset, self.logError)
-        [where_clause] = range_factory.whereExpressions([(Person.id, 1)])
+        [where_clause] = range_factory.whereExpressions([Person.id], [1])
         self.assertEquals('Person.id > ?', compile(where_clause))
 
     def test_whereExpressions_desc(self):
@@ -368,15 +368,45 @@
         resultset = self.makeStormResultSet()
         range_factory = StormRangeFactory(resultset, self.logError)
         [where_clause] = range_factory.whereExpressions(
-            [(Desc(Person.id), 1)])
+            [Desc(Person.id)], [1])
         self.assertEquals('Person.id < ?', compile(where_clause))
 
-    def test_whereExpressions__two_sort_columns(self):
-        """If the sort columns and memo values (c1, m1) and (c2, m2)
-        are specified, whereExpressions() returns two expressions where
-        the first expression is
-
-            c1 == m1 AND c2 > m2
+    def test_whereExpressions__two_sort_columns_asc_asc(self):
+        """If the ascending sort columns c1, c2 and the memo values
+        m1, m2 are specified, whereExpressions() returns a WHERE
+        expressions comparing the tuple (c1, c2) with the memo tuple
+        (m1, m2):
+
+        (c1, c2) > (m1, m2)
+        """
+        resultset = self.makeStormResultSet()
+        range_factory = StormRangeFactory(resultset, self.logError)
+        [where_clause] = range_factory.whereExpressions(
+            [Person.id, Person.name], [1, 'foo'])
+        self.assertEquals(
+            "(Person.id, Person.name) > (1, 'foo')", compile(where_clause))
+
+    def test_whereExpressions__two_sort_columns_desc_desc(self):
+        """If the descending sort columns c1, c2 and the memo values
+        m1, m2 are specified, whereExpressions() returns a WHERE
+        expressions comparing the tuple (c1, c2) with the memo tuple
+        (m1, m2):
+
+        (c1, c2) < (m1, m2)
+        """
+        resultset = self.makeStormResultSet()
+        range_factory = StormRangeFactory(resultset, self.logError)
+        [where_clause] = range_factory.whereExpressions(
+            [Desc(Person.id), Desc(Person.name)], [1, 'foo'])
+        self.assertEquals(
+            "(Person.id, Person.name) < (1, 'foo')", compile(where_clause))
+
+    def test_whereExpressions__two_sort_columns_asc_desc(self):
+        """If the ascending sort column c1, the descending sort column
+        c2 and the memo values m1, m2 are specified, whereExpressions()
+        returns two expressions where  the first expression is
+
+            c1 == m1 AND c2 < m2
 
         and the second expression is
 
@@ -385,9 +415,9 @@
         resultset = self.makeStormResultSet()
         range_factory = StormRangeFactory(resultset, self.logError)
         [where_clause_1, where_clause_2] = range_factory.whereExpressions(
-            [(Person.id, 1), (Person.name, 'foo')])
+            [Person.id, Desc(Person.name)], [1, 'foo'])
         self.assertEquals(
-            'Person.id = ? AND Person.name > ?', compile(where_clause_1))
+            'Person.id = ? AND Person.name < ?', compile(where_clause_1))
         self.assertEquals('Person.id > ?', compile(where_clause_2))
 
     def test_getSlice__forward_without_memo(self):