Merge lp:~openerp-dev/openobject-server/6.1-res_partner_address_name_search-rco into lp:openobject-server/6.1

Proposed by Raphael Collet (OpenERP)
Status: Merged
Merged at revision: 4120
Proposed branch: lp:~openerp-dev/openobject-server/6.1-res_partner_address_name_search-rco
Merge into: lp:openobject-server/6.1
Diff against target: 47 lines (+25/-10)
1 file modified
openerp/addons/base/res/res_partner.py (+25/-10)
To merge this branch: bzr merge lp:~openerp-dev/openobject-server/6.1-res_partner_address_name_search-rco
Reviewer Review Type Date Requested Status
Olivier Dony (Odoo) Approve
Raphael Collet (OpenERP) (community) Needs Resubmitting
Review via email: mp+98196@code.launchpad.net

Description of the change

Make the method name_search of res.partner.address consistent with method name_get.
It searches on all the fields that are used in name_get in the same context.

To post a comment you must log in.
Revision history for this message
Olivier Dony (Odoo) (odo-openerp) wrote :

Functionally it looks good to me, with one exception: despite name_get() not using the `zip` column, it seems a useful thing to allow searching on zipcodes, as it could be a useful way to get a shortlist of possible candidates. Seeing that it used to have the priority over other columns in the previous implementation, dropping it completely sounds dangerous for a stable version. We could simply add it to the default list of fields, this still means that name_search will behave as expected wrt name_get (especially since zip codes can hardly be confused with other column values).

On a technical level I have one performance question: have you tested this name_search() implementation on a large database containing 10k+ randomly distributed addresses? Past experience told us that performance can become terrible when searching on an OR'ed domain involving translatable records (such as countries here!), due to the complexity of the query being executed on the database. For instance for product.name_search() we had to specifically execute the various sub-queries separately and merge the results in-memory, otherwise the system became very slow with 10k products [1]. I think it makes sense to double-check this here too - creating 10k random addresses is easy with a few `INSERT SELECT` queries.

Finally, some minor technical remarks:
- l.10,l.23: while you're changing the code, it would be nice to use proper spacing for == operators ;-)
- l.25: using a list comprehension is usually much more readable than map():
   [(f, operator, name) for f in fields] vs map(lambda f: (f, operator, name), fields)
- l.25: using expression.OR(domains...) might be more readable than hand-crafting the OR'ed domain

Thanks!

[1] See the product.name_search() implementation note: http://bazaar.launchpad.net/~openerp/openobject-addons/6.1/view/head:/product/product.py#L619

review: Needs Information
Revision history for this message
Raphael Collet (OpenERP) (rco-openerp) wrote :

Thanks, I was not aware of the poor performance of these queries. I'll rewrite the code to follow the former style "incremental search on fields".

4115. By Raphael Collet (OpenERP)

[IMP] res.partner.address: improve name_search() to avoid performance issues

4116. By Raphael Collet (OpenERP)

[IMP] res.partner.address: in name_search, add handle case where name is empty

Revision history for this message
Raphael Collet (OpenERP) (rco-openerp) wrote :

Reworked name_search() to make a series of incremental search statements instead of a big disjunction. This should handle the potential performance issue. It also stops searching when the search limit has been reached.
Added some comments to justify the code as well.

review: Needs Resubmitting
4117. By Raphael Collet (OpenERP)

[IMP] res.partner.address: in name_search, search on partner_id before other fields

Revision history for this message
Olivier Dony (Odoo) (odo-openerp) wrote :

Looks just fine now, thanks!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'openerp/addons/base/res/res_partner.py'
--- openerp/addons/base/res/res_partner.py 2012-01-12 10:50:43 +0000
+++ openerp/addons/base/res/res_partner.py 2012-03-21 13:24:20 +0000
@@ -349,18 +349,33 @@
349 args=[]349 args=[]
350 if not context:350 if not context:
351 context={}351 context={}
352 if context.get('contact_display', 'contact')=='partner ' or context.get('contact_display', 'contact')=='partner_address ' :352
353 ids = self.search(cr, user, [('partner_id',operator,name)], limit=limit, context=context)353 if not name:
354 ids = self.search(cr, user, args, limit=limit, context=context)
355 elif context.get('contact_display', 'contact')=='partner':
356 ids = self.search(cr, user, [('partner_id', operator, name)] + args, limit=limit, context=context)
354 else:357 else:
355 if not name:358 # first lookup zip code, as it is a common and efficient way to search on these data
356 ids = self.search(cr, user, args, limit=limit, context=context)359 ids = self.search(cr, user, [('zip', '=', name)] + args, limit=limit, context=context)
360 # then search on other fields:
361 if context.get('contact_display', 'contact')=='partner_address':
362 fields = ['partner_id', 'name', 'country_id', 'city', 'street']
357 else:363 else:
358 ids = self.search(cr, user, [('zip','=',name)] + args, limit=limit, context=context)364 fields = ['name', 'country_id', 'city', 'street']
359 if not ids:365 # Here we have to search the records that satisfy the domain:
360 ids = self.search(cr, user, [('city',operator,name)] + args, limit=limit, context=context)366 # OR([[(f, operator, name)] for f in fields])) + args
361 if name:367 # Searching on such a domain can be dramatically inefficient, due to the expansion made
362 ids += self.search(cr, user, [('name',operator,name)] + args, limit=limit, context=context)368 # for field translations, and the handling of the disjunction by the DB engine itself.
363 ids += self.search(cr, user, [('partner_id',operator,name)] + args, limit=limit, context=context)369 # So instead, we search field by field until the search limit is reached.
370 while len(ids) < limit and fields:
371 f = fields.pop(0)
372 new_ids = self.search(cr, user, [(f, operator, name)] + args, limit=limit, context=context)
373 # extend ids with the ones in new_ids that are not in ids yet (and keep order)
374 old_ids = set(ids)
375 ids.extend([id for id in new_ids if id not in old_ids])
376
377 if len(ids) > limit:
378 ids = ids[:limit]
364 return self.name_get(cr, user, ids, context=context)379 return self.name_get(cr, user, ids, context=context)
365380
366 def get_city(self, cr, uid, id):381 def get_city(self, cr, uid, id):