launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bulk-insert-2 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bulk-insert-2/+merge/94084

A couple of improvements to bulk.create(), suggested by abentley:

 - Don't crash when asked to insert 0 rows. Instead just do nothing.
 - Add a return_created kwarg to disable load()ing of the created objects, as an optimisation for code that doesn't care.
-- 
https://code.launchpad.net/~wgrant/launchpad/bulk-insert-2/+merge/94084
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bulk-insert-2 into lp:launchpad.

=== modified file 'lib/lp/services/database/bulk.py'
--- lib/lp/services/database/bulk.py	2012-02-21 22:14:07 +0000
+++ lib/lp/services/database/bulk.py	2012-02-22 05:11:20 +0000
@@ -192,13 +192,14 @@
         return (col,)
 
 
-def create(columns, values):
+def create(columns, values, return_created=True):
     """Create a large number of objects efficiently.
 
     :param cols: The Storm columns to insert values into. Must be from a
         single class.
     :param values: A list of lists of values for the columns.
-    :return: A list of the created objects.
+    :param return_created: Retrieve the created objects.
+    :return: A list of the created objects if return_created, otherwise None.
     """
     # Flatten Reference faux-columns into their primary keys.
     db_cols = list(chain.from_iterable(map(_dbify_column, columns)))
@@ -207,6 +208,10 @@
         raise ValueError(
             "The Storm columns to insert values into must be from a single "
             "class.")
+
+    if len(values) == 0:
+        return [] if return_created else None
+
     [cls] = clses
     primary_key = get_cls_info(cls).primary_key
 
@@ -218,10 +223,12 @@
             _dbify_value(col, val) for col, val in zip(columns, value)))
         for value in values]
 
-    result = IStore(cls).execute(
-        Returning(Insert(
-            db_cols, expr=db_values, primary_columns=primary_key)))
-    if len(primary_key) == 1:
-        return load(cls, map(itemgetter(0), result))
+    if not return_created:
+        IStore(cls).execute(Insert(db_cols, expr=db_values))
+        return None
     else:
-        return load(cls, result)
+        result = IStore(cls).execute(
+            Returning(Insert(
+                db_cols, expr=db_values, primary_columns=primary_key)))
+        keys = map(itemgetter(0), result) if len(primary_key) == 1 else result
+        return load(cls, keys)

=== modified file 'lib/lp/services/database/tests/test_bulk.py'
--- lib/lp/services/database/tests/test_bulk.py	2012-02-21 22:14:07 +0000
+++ lib/lp/services/database/tests/test_bulk.py	2012-02-22 05:11:20 +0000
@@ -24,6 +24,7 @@
 from lp.code.model.branchjob import (
     BranchJob,
     BranchJobType,
+    ReclaimBranchSpaceJob,
     )
 from lp.code.model.branchsubscription import BranchSubscription
 from lp.registry.model.person import Person
@@ -269,8 +270,7 @@
 
     def test_null_reference(self):
         # create() handles None as a Reference value.
-        job = Job()
-        IStore(Job).add(job)
+        job = IStore(Job).add(Job())
         wanted = [(None, job, BranchJobType.RECLAIM_BRANCH_SPACE)]
         [branchjob] = bulk.create(
             (BranchJob.branch, BranchJob.job, BranchJob.job_type),
@@ -288,4 +288,28 @@
         # create() handles Reference columns in a typesafe manner.
         self.assertRaisesWithContent(
             RuntimeError, "Property used in an unknown class",
-            bulk.create, (BugSubscription.bug,), [[self.factory.makeBranch()]])
+            bulk.create, (BugSubscription.bug,),
+            [[self.factory.makeBranch()]])
+
+    def test_zero_values_is_noop(self):
+        # create()ing 0 rows is a no-op.
+        with StormStatementRecorder() as recorder:
+            self.assertEqual([], bulk.create((BugSubscription.bug,), []))
+        self.assertThat(recorder, HasQueryCount(Equals(0)))
+
+    def test_load_can_be_skipped(self):
+        # create() can be told not to load the created rows.
+        job = IStore(Job).add(Job())
+        IStore(Job).flush()
+        wanted = [(None, job, BranchJobType.RECLAIM_BRANCH_SPACE)]
+        with StormStatementRecorder() as recorder:
+            self.assertIs(
+                None,
+                bulk.create(
+                    (BranchJob.branch, BranchJob.job, BranchJob.job_type),
+                    wanted, return_created=False))
+        self.assertThat(recorder, HasQueryCount(Equals(1)))
+        [reclaimjob] = ReclaimBranchSpaceJob.iterReady()
+        branchjob = reclaimjob.context
+        self.assertEqual(
+            wanted, [(branchjob.branch, branchjob.job, branchjob.job_type)])