From a23468c845425f3187c9c76dd2200fb9a1c9f3ab Mon Sep 17 00:00:00 2001 From: Denis Ledoux Date: Thu, 5 Nov 2015 16:00:47 +0100 Subject: [PATCH 1/3] [FIX] hr_timesheet_sheet: total attendances & timesheets performances For these function fields, bypassing the ORM, using a SQL query, improves the execution time by 100 for a set of 80 timesheets in a database with - 250K `hr.analytic.timesheet` & - 250K `hr.attendance`. These function fields depends on a one2many field which use the SQL view `hr_timesheet_sheet_sheet_day`. When performing `sheet.period_ids`, two SQL requests are performed, - the first just to know the ids in the sql view matching this sheet - the second to read the fields `total_attendance` & `total_timesheet` and the request is performed on the entire set of lines of this view (~250K lines in the observed use case) while, when replaced by this SQL request, only one request is performed, on a restricted set of lines, speeding up significantly the computation of these computed fields for smaller sets of sheets. opw-653447 --- .../hr_timesheet_sheet/hr_timesheet_sheet.py | 28 +++++++++++-------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/addons/hr_timesheet_sheet/hr_timesheet_sheet.py b/addons/hr_timesheet_sheet/hr_timesheet_sheet.py index ceb661f2bb6..d89552f087e 100644 --- a/addons/hr_timesheet_sheet/hr_timesheet_sheet.py +++ b/addons/hr_timesheet_sheet/hr_timesheet_sheet.py @@ -41,18 +41,24 @@ class hr_timesheet_sheet(osv.osv): """ Compute the attendances, analytic lines timesheets and differences between them for all the days of a timesheet and the current day """ + res = dict.fromkeys(ids, { + 'total_attendance': 0.0, + 'total_timesheet': 0.0, + 'total_difference': 0.0, + }) + + cr.execute(""" + SELECT sheet_id as id, + sum(total_attendance) as total_attendance, + sum(total_timesheet) as total_timesheet, + sum(total_difference) as total_difference + FROM hr_timesheet_sheet_sheet_day + WHERE sheet_id IN %s + GROUP BY sheet_id + """, (tuple(ids),)) + + res.update(dict((x.pop('id'), x) for x in cr.dictfetchall())) - res = {} - for sheet in self.browse(cr, uid, ids, context=context or {}): - res.setdefault(sheet.id, { - 'total_attendance': 0.0, - 'total_timesheet': 0.0, - 'total_difference': 0.0, - }) - for period in sheet.period_ids: - res[sheet.id]['total_attendance'] += period.total_attendance - res[sheet.id]['total_timesheet'] += period.total_timesheet - res[sheet.id]['total_difference'] += period.total_attendance - period.total_timesheet return res def check_employee_attendance_state(self, cr, uid, sheet_id, context=None): From c28a28e69eb9a39a5acfcd1247d87f5bccaca7f3 Mon Sep 17 00:00:00 2001 From: Raphael Collet Date: Thu, 5 Nov 2015 09:18:48 +0100 Subject: [PATCH 2/3] [IMP] osv: use iteration for expression negating The current code when applying negative operator on an expression used recursion which in extreme case is not best friend with python. e.g: on instance with a lot of wharehouse, some simple action could lead to a domain with lot of elements which could easiliy go over the python maximum recursion limit. This commit fixes this by replacing recursion with iteration. We have a stack of negation flags and loop on each token of the domain as follow : - when we iterate on a leaf, it consumes the top negation flag, - after a '!' operator, the top token negation is inversed, - after an '&' or '|' operator, the top negation flag is duplicated on the top of the stack. closes #9433 opw-653802 --- openerp/osv/expression.py | 93 +++++++++++++++++++++------------------ 1 file changed, 49 insertions(+), 44 deletions(-) diff --git a/openerp/osv/expression.py b/openerp/osv/expression.py index 1e3ac1d0034..a7539dd6108 100644 --- a/openerp/osv/expression.py +++ b/openerp/osv/expression.py @@ -165,6 +165,26 @@ TERM_OPERATORS = ('=', '!=', '<=', '<', '>', '>=', '=?', '=like', '=ilike', # legal in the processed term. NEGATIVE_TERM_OPERATORS = ('!=', 'not like', 'not ilike', 'not in') +# Negation of domain expressions +DOMAIN_OPERATORS_NEGATION = { + AND_OPERATOR: OR_OPERATOR, + OR_OPERATOR: AND_OPERATOR, +} +TERM_OPERATORS_NEGATION = { + '<': '>=', + '>': '<=', + '<=': '>', + '>=': '<', + '=': '!=', + '!=': '=', + 'in': 'not in', + 'like': 'not like', + 'ilike': 'not ilike', + 'not in': 'in', + 'not like': 'like', + 'not ilike': 'ilike', +} + TRUE_LEAF = (1, '=', 1) FALSE_LEAF = (0, '=', 1) @@ -261,51 +281,36 @@ def distribute_not(domain): ['|',('user_id','!=',4),('partner_id','not in',[1,2])] """ - def negate(leaf): - """Negates and returns a single domain leaf term, - using the opposite operator if possible""" - left, operator, right = leaf - mapping = { - '<': '>=', - '>': '<=', - '<=': '>', - '>=': '<', - '=': '!=', - '!=': '=', - } - if operator in ('in', 'like', 'ilike'): - operator = 'not ' + operator - return [(left, operator, right)] - if operator in ('not in', 'not like', 'not ilike'): - operator = operator[4:] - return [(left, operator, right)] - if operator in mapping: - operator = mapping[operator] - return [(left, operator, right)] - return [NOT_OPERATOR, (left, operator, right)] - def distribute_negate(domain): - """Negate the domain ``subtree`` rooted at domain[0], - leaving the rest of the domain intact, and return - (negated_subtree, untouched_domain_rest) - """ - if is_leaf(domain[0]): - return negate(domain[0]), domain[1:] - if domain[0] == AND_OPERATOR: - done1, todo1 = distribute_negate(domain[1:]) - done2, todo2 = distribute_negate(todo1) - return [OR_OPERATOR] + done1 + done2, todo2 - if domain[0] == OR_OPERATOR: - done1, todo1 = distribute_negate(domain[1:]) - done2, todo2 = distribute_negate(todo1) - return [AND_OPERATOR] + done1 + done2, todo2 - if not domain: - return [] - if domain[0] != NOT_OPERATOR: - return [domain[0]] + distribute_not(domain[1:]) - if domain[0] == NOT_OPERATOR: - done, todo = distribute_negate(domain[1:]) - return done + distribute_not(todo) + # This is an iterative version of a recursive function that split domain + # into subdomains, processes them and combine the results. The "stack" below + # represents the recursive calls to be done. + result = [] + stack = [False] + + for token in domain: + negate = stack.pop() + # negate tells whether the subdomain starting with token must be negated + if is_leaf(token): + if negate: + left, operator, right = token + if operator in TERM_OPERATORS_NEGATION: + result.append((left, TERM_OPERATORS_NEGATION[operator], right)) + else: + result.append(NOT_OPERATOR) + result.append(token) + else: + result.append(token) + elif token == NOT_OPERATOR: + stack.append(not negate) + elif token in DOMAIN_OPERATORS_NEGATION: + result.append(DOMAIN_OPERATORS_NEGATION[token] if negate else token) + stack.append(negate) + stack.append(negate) + else: + result.append(token) + + return result # -------------------------------------------------- From d24fcd1d2e489d2c4877b0b55d0325a5a8e43b27 Mon Sep 17 00:00:00 2001 From: Nicolas Lempereur Date: Thu, 5 Nov 2015 15:08:17 +0100 Subject: [PATCH 3/3] [IMP] base: test for expression distribute_not Testing the use case of c28a28e, in which distribute_not was refactored with iteration instead of recursion. --- openerp/addons/base/tests/test_expression.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/openerp/addons/base/tests/test_expression.py b/openerp/addons/base/tests/test_expression.py index bbdaa697bcd..146439f0c7d 100644 --- a/openerp/addons/base/tests/test_expression.py +++ b/openerp/addons/base/tests/test_expression.py @@ -1,5 +1,6 @@ import unittest2 +import openerp.osv.expression as expression from openerp.osv.expression import get_unaccent_wrapper from openerp.osv.orm import BaseModel import openerp.tests.common as common @@ -459,6 +460,25 @@ class test_expression(common.TransactionCase): partner_parent_id_col._auto_join = False state_country_id_col._auto_join = False + def test_40_negating_long_expression(self): + source = ['!','&',('user_id','=',4),('partner_id','in',[1,2])] + expect = ['|',('user_id','!=',4),('partner_id','not in',[1,2])] + self.assertEqual(expression.distribute_not(source), expect, + "distribute_not on expression applied wrongly") + + pos_leaves = [[('a', 'in', [])], [('d', '!=', 3)]] + neg_leaves = [[('a', 'not in', [])], [('d', '=', 3)]] + + source = expression.OR([expression.AND(pos_leaves)] * 1000) + expect = source + self.assertEqual(expression.distribute_not(source), expect, + "distribute_not on long expression without negation operator should not alter it") + + source = ['!'] + source + expect = expression.AND([expression.OR(neg_leaves)] * 1000) + self.assertEqual(expression.distribute_not(source), expect, + "distribute_not on long expression applied wrongly") + def test_translate_search(self): Country = self.registry('res.country') be = self.ref('base.be')