Merge lp:~adeuring/launchpad/bug-739052-8 into lp:launchpad

[Abel Deuring]

[r=abentley][bug=739052][incr] new property StormResultSet.rough_length; bump version number of lazr.batchnavigator

This branch is the stap preparatory step required to use StormRangeFactory. (At least I hope it...)

It increases the version number of lazr.batchnavigator to 1.2.10 (which I released and included in download-cache today.)

It also adds a new property StormRangeFactory.rough_length, which run an "EXPLAIN SELECT ..." query, as a surrogate of a sometimes very expensive "SELECT count(*)..." query.

lazr.batchnavigator uses this property only to display the last number in "6 -> 10 of 123 results", i.e., the exact value is not very important. (We might though consider to change the text to "6 -> 10 of approximately 123 results".)

The tests test_rough_length() and test_rough_length_first_sort_column_desc() would pass too with an assertEqual(5, estimated_length) (5 is the exact length of the result set), but the third test, test_rough_length_decorated_result_set(), is an example that rough_length is indeed not always excat: It return 4, while the real result length is 5.

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

no lint

[Aaron Bentley (abentley)]

In the future, it might make sense to move most of rough_length into a generic function. But this is good for now.

review: Approve

[Robert Collins (lifeless)]

There is a count estimator function in Launchpad already - was it
unsuitable for use by this?

[Abel Deuring (adeuring)]

On 14.09.2011 21:50, Robert Collins wrote:
> There is a count estimator function in Launchpad already - was it
> unsuitable for use by this?

Well, I _could_ have tweaked it: It wants a traditional psycopg cursor,
while we have Storm result set.

Aaron already suggested to make a general function from the method -- I
was simply too lazy to do that: proper test setup etc etc.

I am already working a bit too long on the batching stuff, considering
that I naively started with the intent to fix "just" bug 739052 ;) OK,
there were a few interruptions...

And before the Orange squad starts again feature work, I'd like to go
back to the main interruption: the HWDB problems we had recently.

I think we need urgently a much closer "feedback loop" for checkbox (the
HWDB client that submits the reports): We should process the reports
immediately when checkbox submits the data so that the client can notice
any problems immediately.

Sure, not having an general "length estimation function" for Storm
result sets is wart -- but a much smaller one than the HWDB issues...

=== modified file 'lib/canonical/launchpad/webapp/batching.py'
--- lib/canonical/launchpad/webapp/batching.py	2011-08-25 15:13:59 +0000
+++ lib/canonical/launchpad/webapp/batching.py	2011-09-14 15:54:12 +0000
@@ -12,6 +12,7 @@
 import lazr.batchnavigator
 from lazr.batchnavigator.interfaces import IRangeFactory
 from operator import isSequenceType
+import re
 import simplejson
 from storm import Undef
 from storm.expr import (
@@ -23,7 +24,10 @@
     )
 from storm.properties import PropertyColumn
 from storm.zope.interfaces import IResultSet
-from zope.component import adapts
+from zope.component import (
+    adapts,
+    getUtility,
+    )
 from zope.interface import implements
 from zope.interface.common.sequence import IFiniteSequence
 from zope.security.proxy import (
@@ -33,15 +37,23 @@
     )
 
 from canonical.config import config
-from canonical.database.sqlbase import sqlvalues
+from canonical.database.sqlbase import (
+    convert_storm_clause_to_string,
+    sqlvalues,
+    )
+
 from canonical.launchpad.components.decoratedresultset import (
     DecoratedResultSet,
     )
 from canonical.launchpad.webapp.interfaces import (
+    IStoreSelector,
+    MAIN_STORE,
+    SLAVE_FLAVOR,
+    StormRangeFactoryError,
     ITableBatchNavigator,
-    StormRangeFactoryError,
     )
 from canonical.launchpad.webapp.publisher import LaunchpadView
+from lp.services.propertycache import cachedproperty
 
 
 class FiniteSequenceAdapter:
@@ -559,3 +571,25 @@
         """See `IRangeFactory."""
         sliced = self.resultset[start:end]
         return ShadowedList(list(sliced), list(sliced.get_plain_result_set()))
+
+    @cachedproperty
+    def rough_length(self):
+        """See `IRangeFactory."""
+        # get_select_expr() requires at least one column as a parameter.
+        # getorderBy() already knows about columns that can appear
+        # in the result set, let's take just the first one.
+        column = self.getOrderBy()[0]
+        if zope_isinstance(column, Desc):
+            column = column.expr
+        select = removeSecurityProxy(self.plain_resultset).get_select_expr(
+            column)
+        explain = 'EXPLAIN ' + convert_storm_clause_to_string(select)
+        store = getUtility(IStoreSelector).get(MAIN_STORE, SLAVE_FLAVOR)
+        result = store.execute(explain)
+        _rows_re = re.compile("rows=(\d+)\swidth=")
+        first_line = result.get_one()[0]
+        match = _rows_re.search(first_line)
+        if match is None:
+            raise RuntimeError(
+                "Unexpected EXPLAIN output %s" % repr(first_line))
+        return int(match.group(1))

=== modified file 'lib/canonical/launchpad/webapp/tests/test_batching.py'
--- lib/canonical/launchpad/webapp/tests/test_batching.py	2011-08-25 15:13:59 +0000
+++ lib/canonical/launchpad/webapp/tests/test_batching.py	2011-09-14 15:54:12 +0000
@@ -12,6 +12,10 @@
     compile,
     Desc,
     )
+from testtools.matchers import (
+    LessThan,
+    Not,
+    )
 from zope.security.proxy import isinstance as zope_isinstance
 
 from canonical.launchpad.components.decoratedresultset import (
@@ -715,3 +719,31 @@
         self.assertEqual(2, shadow_list[0])
         self.assertEqual(0, shadow_list.shadow_values[-1])
         self.assertEqual(2, shadow_list.shadow_values[0])
+
+    def test_rough_length(self):
+        # StormRangeFactory.rough_length returns an estimate of the
+        # length of the result set.
+        resultset = self.makeStormResultSet()
+        resultset.order_by(Person.id)
+        range_factory = StormRangeFactory(resultset)
+        estimated_length = range_factory.rough_length
+        self.assertThat(estimated_length, LessThan(10))
+        self.assertThat(estimated_length, Not(LessThan(1)))
+
+    def test_rough_length_first_sort_column_desc(self):
+        # StormRangeFactory.rough_length can handle result sets where
+        # the first sort column has descendig order.
+        resultset = self.makeStormResultSet()
+        resultset.order_by(Desc(Person.id))
+        range_factory = StormRangeFactory(resultset)
+        estimated_length = range_factory.rough_length
+        self.assertThat(estimated_length, LessThan(10))
+        self.assertThat(estimated_length, Not(LessThan(1)))
+
+    def test_rough_length_decorated_result_set(self):
+        # StormRangeFactory.rough_length can handle DecoratedResultSets.
+        resultset = self.makeDecoratedStormResultSet()
+        range_factory = StormRangeFactory(resultset)
+        estimated_length = range_factory.rough_length
+        self.assertThat(estimated_length, LessThan(10))
+        self.assertThat(estimated_length, Not(LessThan(1)))

=== modified file 'versions.cfg'
--- versions.cfg	2011-09-14 11:08:33 +0000
+++ versions.cfg	2011-09-14 15:54:12 +0000
@@ -30,7 +30,7 @@
 launchpadlib = 1.9.9
 lazr.amqp = 0.1
 lazr.authentication = 0.1.1
-lazr.batchnavigator = 1.2.9
+lazr.batchnavigator = 1.2.10
 lazr.config = 1.1.3
 lazr.delegates = 1.2.0
 lazr.enum = 1.1.3