Hi Jeroen, This is a nice improvement. Comments below. -Edwin >=== modified file 'lib/lp/code/interfaces/branchrevision.py' >--- lib/lp/code/interfaces/branchrevision.py 2009-06-25 04:06:00 +0000 >+++ lib/lp/code/interfaces/branchrevision.py 2010-07-13 13:32:27 +0000 >@@ -21,7 +21,7 @@ > ancestry of a branch. History revisions have an integer sequence, merged > revisions have sequence set to None. > """ >- >+ # XXX JeroenVermeulen 2010-07-12: We're dropping this column. I assume you are planning on adding a bug for this XXX. > id = Int(title=_('The database revision ID')) > > sequence = Int( > >=== modified file 'lib/lp/code/model/branch.py' >--- lib/lp/code/model/branch.py 2010-07-09 10:22:32 +0000 >+++ lib/lp/code/model/branch.py 2010-07-13 13:32:27 +0000 >@@ -226,11 +226,12 @@ > > @property > def revision_history(self): >- return BranchRevision.select(''' >- BranchRevision.branch = %s AND >- BranchRevision.sequence IS NOT NULL >- ''' % sqlvalues(self), >- prejoins=['revision'], orderBy='-sequence') >+ # XXX JeroenVermeulen 2010-07-12: Prejoin revision. >+ result = Store.of(self).find( >+ BranchRevision, >+ BranchRevision.branch_id == self.id, >+ BranchRevision.sequence != None) >+ return result.order_by(Desc(BranchRevision.sequence)) You can prejoin the revision like this. The Revision will be added to the cache, and since BranchRevision has the foreign key, it will retrieve the Revision from the cache by id, even though the list comprehension appears to throw it away. > result = Store.of(self).find( > (BranchRevision, Revision), > BranchRevision.revisionID == Revision.id, > BranchRevision.branch_id == self.id, > BranchRevision.sequence != None) > result = result.order_by(Desc(BranchRevision.sequence)) > return [branch_revision for branch_revision, branch in result] > > > subscriptions = SQLMultipleJoin( > 'BranchSubscription', joinColumn='branch', orderBy='id') >@@ -509,7 +510,7 @@ > > def latest_revisions(self, quantity=10): > """See `IBranch`.""" >- return self.revision_history.limit(quantity) >+ return self.revision_history.config(limit=quantity) Of course, if you use the prejoin above, you will need access to the storm query before stripping out the Revision. > def getMainlineBranchRevisions(self, start_date, end_date=None, > oldest_first=False): >@@ -532,14 +533,13 @@ > > def getRevisionsSince(self, timestamp): > """See `IBranch`.""" >- return BranchRevision.select( >- 'Revision.id=BranchRevision.revision AND ' >- 'BranchRevision.branch = %d AND ' >- 'BranchRevision.sequence IS NOT NULL AND ' >- 'Revision.revision_date > %s' % >- (self.id, quote(timestamp)), >- orderBy='-sequence', >- clauseTables=['Revision']) >+ result = Store.of(self).using(Revision).find( >+ BranchRevision, >+ Revision == BranchRevision.revision, >+ BranchRevision.branch == self, >+ BranchRevision.sequence != None, >+ Revision.revision_date > timestamp) >+ return result.order_by(Desc(BranchRevision.sequence)) A prejoin would be good here, too. > def canBeDeleted(self): > """See `IBranch`.""" >@@ -860,8 +860,8 @@ > > def getScannerData(self): > """See `IBranch`.""" >- cur = cursor() >- cur.execute(""" >+# XXX JeroenVermeulen 2010-07-12: We're dropping BranchRevision.id. This XXX needs that bug number and some indentation. >+ rows = Store.of(self).execute(""" Storm queries can retrieve individual columns also: Store.of(self).find( (BranchRevision.id, BranchRevision.sequence, Revision.revision_id) > SELECT BranchRevision.id, BranchRevision.sequence, > Revision.revision_id > FROM Revision, BranchRevision >@@ -872,10 +872,12 @@ > ancestry = set() > history = [] > branch_revision_map = {} >- for branch_revision_id, sequence, revision_id in cur.fetchall(): >+ for branch_revision_id, sequence, revision_id in rows: > ancestry.add(revision_id) > branch_revision_map[revision_id] = branch_revision_id > if sequence is not None: >+ assert sequence == len(history) + 1, ( >+ "Broken revision sequence number.") I hope a `code` developer is comfortable that this won't cause too many oopses. > history.append(revision_id) > return ancestry, history, branch_revision_map > >@@ -1001,7 +1003,7 @@ > name = "date_trunc" > results = Store.of(self).find( > (DateTrunc('day', Revision.revision_date), Count(Revision.id)), >- Revision.id == BranchRevision.revisionID, >+ Revision.id == BranchRevision.revision_id, > Revision.revision_date > since, > BranchRevision.branch == self) > results = results.group_by( > >=== modified file 'lib/lp/codehosting/scanner/tests/test_bzrsync.py' >--- lib/lp/codehosting/scanner/tests/test_bzrsync.py 2010-06-16 19:47:27 +0000 >+++ lib/lp/codehosting/scanner/tests/test_bzrsync.py 2010-07-13 13:32:27 +0000 >@@ -228,10 +231,11 @@ > :return: A set of tuples (sequence, revision-id) for all the > BranchRevisions rows belonging to self.db_branch. > """ >+ branchrevisions = IStore(BranchRevision).find( >+ BranchRevision, BranchRevision.branch == db_branch) Adding an underscore in `branchrevisions` would be good. Instead of pulling out the sequence object and revision_id below, the storm query can do it. > return set( > (branch_revision.sequence, branch_revision.revision.revision_id) >- for branch_revision >- in BranchRevision.selectBy(branch=db_branch)) >+ for branch_revision in branchrevisions) > > def writeToFile(self, filename="file", contents=None): > """Set the contents of the specified file. >@@ -459,8 +463,9 @@ > # retrieveDatabaseAncestry. > branch = getUtility(IBranchLookup).getByUniqueName( > '~name12/+junk/junk.contrib') >- sampledata = list( >- BranchRevision.selectBy(branch=branch).orderBy('sequence')) >+ branchrevisions = IStore(BranchRevision).find( >+ BranchRevision, BranchRevision.branch == branch) >+ sampledata = list(branchrevisions.order_by(BranchRevision.sequence)) > expected_ancestry = set(branch_revision.revision.revision_id > for branch_revision in sampledata) storm can pull out the revision_id instead of using a generator. > expected_history = [branch_revision.revision.revision_id > >=== modified file 'lib/lp/code/model/branch.py' >--- lib/lp/code/model/branch.py 2010-07-12 17:15:50 +0000 >+++ lib/lp/code/model/branch.py 2010-07-13 14:55:59 +0000 >@@ -533,9 +533,9 @@ > > def getRevisionsSince(self, timestamp): > """See `IBranch`.""" >- result = Store.of(self).using(Revision).find( >+ result = Store.of(self).using(BranchRevision, Revision).find( > BranchRevision, This is another candidate for a prejoin, and then you won't need the using(). >- Revision == BranchRevision.revision, >+ Revision.id == BranchRevision.revision_id, > BranchRevision.branch == self, > BranchRevision.sequence != None, > Revision.revision_date > timestamp) > >=== modified file 'lib/lp/code/model/branch.py' >--- lib/lp/code/model/branch.py 2010-07-12 17:15:50 +0000 >+++ lib/lp/code/model/branch.py 2010-07-13 14:55:59 +0000 >@@ -533,9 +533,9 @@ > > def getRevisionsSince(self, timestamp): > """See `IBranch`.""" >- result = Store.of(self).using(Revision).find( >+ result = Store.of(self).using(BranchRevision, Revision).find( > BranchRevision, This is another candidate for a prejoin, and then you won't need the using(). >- Revision == BranchRevision.revision, >+ Revision.id == BranchRevision.revision_id, > BranchRevision.branch == self, > BranchRevision.sequence != None, > Revision.revision_date > timestamp) > >=== modified file 'lib/lp/code/model/branchmergeproposal.py' >--- lib/lp/code/model/branchmergeproposal.py 2010-07-12 17:15:50 +0000 >+++ lib/lp/code/model/branchmergeproposal.py 2010-07-13 14:55:59 +0000 >@@ -88,13 +96,13 @@ > return False > # Non-reviewers can toggle within the reviewed ok states > # (approved/queued/failed): they can dequeue something they spot an >- # environmental issue with (queued or failed to approved). Retry things >- # that had an environmental issue (failed or approved to queued) and note >- # things as failing (approved and queued to failed). >+ # environmental issue with (queued or failed to approved). Retry >+ # things that had an environmental issue (failed or approved to >+ # queued) and note things as failing (approved and queued to failed). > # This is perhaps more generous than needed, but its not clearly wrong >- # - a key concern is to prevent non reviewers putting things in the >- # queue that haven't been oked (and thus moved to approved or one of the >- # workflow states that approved leads to). >+ # - a key concern is to prevent non reviewers putting things in the >+ # queue that haven't been oked (and thus moved to approved or one of s/oked/okayed/ or maybe just say "approved". >+ # the workflow states that approved leads to). > elif (next_state in reviewed_ok_states and > from_state not in reviewed_ok_states): > return False >@@ -581,14 +589,19 @@ > > def getUnlandedSourceBranchRevisions(self): > """See `IBranchMergeProposal`.""" >- return BranchRevision.select(''' >- BranchRevision.branch = %s AND >- BranchRevision.sequence IS NOT NULL AND >- BranchRevision.revision NOT IN ( >- SELECT revision FROM BranchRevision >- WHERE branch = %s) >- ''' % sqlvalues(self.source_branch, self.target_branch), >- prejoins=['revision'], orderBy='-sequence', limit=10) >+ store = Store.of(self) >+ SourceRevision = ClassAlias(BranchRevision) >+ TargetRevision = ClassAlias(BranchRevision) >+ target_join = LeftJoin( >+ TargetRevision, And( >+ TargetRevision.revision_id == SourceRevision.revision_id, >+ TargetRevision.branch_id == self.target_branch.id)) >+ result = store.using(SourceRevision, target_join).find( EDG: This was a little confusing since it's formatted differently than we do normally: origin = [ SourceRevision, LeftJoin(...) ] store.using(*origin).find(...) You could also prejoin Revision here. >+ SourceRevision, >+ SourceRevision.branch_id == self.source_branch.id, >+ SourceRevision.sequence != None, >+ TargetRevision.id == None) >+ return result.order_by(Desc(SourceRevision.sequence)).config(limit=10) > > def createComment(self, owner, subject, content=None, vote=None, > review_type=None, parent=None, _date_created=DEFAULT,