[6.0 & 6.1] [account] def compute needs optimization

[Moisés López - http://www.vauxoo.com]

The def compute of account module no use parent_right & parent_left fields for calculate sum(debit) & sum(credit)
and not in use recursive function's

These is very slow with a big account chart and very much account_move_line

This can optimizate using parent_left & parent_right

Im working for a propose merge with the solution.

Meanwhile i put a script sql with my idea

 SELECT MIN(aa_tree_1.code) AS code,
  SUM(account_move_line.debit) AS debit, SUM(account_move_line.credit) AS credit
 FROM account_account aa_tree_1
 INNER JOIN account_account aa_tree_2
    ON aa_tree_2.parent_left
       BETWEEN aa_tree_1.parent_left AND aa_tree_1.parent_right
 INNER JOIN account_move_line
   ON account_move_line.account_id = aa_tree_2.id
 INNER JOIN account_move
   ON account_move.id = account_move_line.move_id
   AND account_move.state = 'posted'
 GROUP BY aa_tree_1.id

These script is functionally without consolidate account, but I'm working

[Moisés López - http://www.vauxoo.com (moylop260)]

The time difference is huge, with these sql script vs current function.

3 minutes VS 0.3 seconds in my case

[Amit Parik (amit-parik)]

Hello,

you are right, Time makes considerable difference but at same time we avoid SQL injections so this can be Improvement.

Currently I am setting this as a "Wishlist", If you have perfect solution then would you please create a new stake branch and put a merge proposal for official addons.

Thanks!

[Moisés López - http://www.vauxoo.com (moylop260)]

Ok, thank you.
We are working for it

I comment what, my patch example solution no have SQL injections
My idea is next patch:

=== modified file 'account/account.py'
--- account/account.py 2011-10-10 08:50:07 +0000
+++ account/account.py 2011-11-08 00:54:14 +0000
@@ -271,6 +271,13 @@
             request = ("SELECT l.account_id as id, " +\
                        ', '.join(map(mapping.__getitem__, field_names)) +
                        " FROM account_move_line l" \
+ """
+ INNER JOIN account_account aa_tree_1
+ ON aa_tree_1.id = l.account_id
+ INNER JOIN account_account aa_tree_2
+ ON aa_tree_2.parent_left
+ BETWEEN aa_tree_1.parent_left AND aa_tree_1.parent_right
+ """ \
                        " WHERE l.account_id IN %s " \
                             + filters +
                        " GROUP BY l.account_id")

NOTE: This is a example of my idea, not have the full solution

[Moisés López - http://www.vauxoo.com (moylop260)]

sorry, I meant that logic does not change much, than as it is doing now.
So the sql injection that could have right now would be another problem to solve. Even, just as it is now

[Moisés López - http://www.vauxoo.com (moylop260)]

Here I leave the beta phase of pure sql script. Now I need to download the standard of OpenERP, but for now you can try to measure performance.

This script displays a report "general ledger" in PostgreSQL

[Cristian Salamea (ovnicraft)]

@Amit IMHO set as wishlist this issue i a bad idea there is a good propose from moylop260, i think you must assign someone in R&D teams to help him and do it better.

Can you explain is how and where sql injection is possible?

Regards,

[Moisés López - http://www.vauxoo.com (moylop260)]

I'm very excited.
The first test has successful.
I'm working on the proposed merge.

I let you a video with the difference of perfomace

[Moisés López - http://www.vauxoo.com (moylop260)]

In fact, there are 2 improvements that I have mentioned.

1) Every time I open the account and subaccount on tree view, recalculates the subaccount amount.
But, the calculate of the account parent depends of account childs, then was calculate before.
This don't need re-calculate in account child.

I mean?

2) The credit is calculated sum(credit) and debit with sum(debit), this is correct.
But the balance is calculated sum (debit) - sum (credit), should not get here to recalculate the sum (debit) and then sum (credit) when you got them before.
This need to use the accumulated calculated in a parent view
credit-debit
from ( sum(credit) as credit, sum(debit) as debit .... )

The 2) now is fixed it
The 1) I can't fixed it

[Amit Parik (amit-parik)]

Hello,

Thanks for your quick reponse and fix!

we will take this in to account as an when this wishlist targeted and we will consider this as a feature-roadmaps.

You can also post this feature in feedback.openerp.com, at there experts can review it.

Thanks again!

[Ferdinand (office-chricar)]

IMHO for the balance there is an easy "fix" / workaround

    def _balance_move_line(self, cr, uid, ids, name, arg, context={}):
        res = {}
        for line in self.browse(cr, uid, ids,context):
            balance = line.debit - line.credit
            res[line.id] = balance
        return res

'balance_move_line': fields.function(_balance_move_line, method=True, string='Balance Line'),

with store=true

this stored value can be used by the SQL statement

for reconciliation we found it usefull to be able to sort by balance without signe to get matching values next to each other

    def _balance_move_line_abs(self, cr, uid, ids, name, arg, context={}):
        res = {}
        for line in self.browse(cr, uid, ids,context):
            balance = abs(line.debit - line.credit)
            res[line.id] = balance
        return res

        'balance_move_line_abs': fields.function(_balance_move_line_abs, method=True, string='Balance Line abs'),

for server side sorting of function fields please see
http://www.camptocamp.com/fr/component/wordpress/?author=87
https://code.launchpad.net/~c2c/openobject-server/6.0-c2c-official

[Nhomar - Vauxoo (nhomar)]

Hello Ferdinand.

You proposal is not bad, even almost all systems works with your concept, because what @moylop260 did already is a really complex concept in computational logic, called b_tree or something like that, so, no so much programers understand it or know how apply it, this was the case for the original programmer that write _compute method in openerp..... But all the scenary was well designed to use it... ;-)

BUT, logic that have OpenERP right now is so so inefficient, even this until now has been a really big problem, but, we in vauxoo have 3 customers with chart of account of 8000 and 16.000 accounts in charts, in v5 v6 and preparing for v6.1, for this reason we know the behaviour in three versions, and calculate a simple Balance has been a headache, with 1 year of data and more than 100.000 moves we REALLY need optimization to REALLY have our tree view of the chart of account efficient and usable in an production enviroment, and financial reports calculated quickly...., @moylop260 (our CTO in MExico) work this week in Venezuela already with our CTO in Venezuela @humbertoarocha and myself in find an Extraordinary solution and I think this was really the solution.

I mean.....

Your Option is good.

THis patch and future merge is AMAZING.

We are really excited with the improve in speed and even tryton that talk A LOT about good practices and this stuff don't have this feature.

-- Calculate 16000 balance in 1 second and show inmediatly on tree view..... WAOOOOO i think it is amazing,

We invite to all community in try our method please, and try multicurrecny stuff and give us feedback to improve it.

WE want this for V6.1 ..........

Regards.....

[Moisés López - http://www.vauxoo.com (moylop260)]

Merge proposal done:

https://code.launchpad.net/~vauxoo/openobject-addons/trunk-bug_887376-account_move_compute_with_btree-moylop260/+merge/81731

[Moisés López - http://www.vauxoo.com (moylop260)]

Feedback's created too

http://feedback.openerp.com/forums/77459-general-improvements/suggestions/2378287-accounts-tree-view-should-not-be-re-calculated-whe

http://feedback.openerp.com/forums/77459-general-improvements/suggestions/2378275-account-should-be-optimized-compute-sql-b-tree

[Ferdinand (office-chricar)]

 + 'balance': "COALESCE(SUM(l.debit),0) " \
 + "- COALESCE(SUM(l.credit), 0) as balance",

IMHO must be
'balance' : "SUM( COALESCE(l.debit,0) - COALESCE(l.credit),0) as balance",

I just want to ask if it is not "cheaper" (more performant) to make debit/credit as required and store 0 as default to avoid execution of COALESCE for every read...??

[Moisés López - http://www.vauxoo.com (moylop260)]

@Ferdinand Im total agree with you
Add a
_defaults = {
  'debit': 0,
  'credit': 0,
}

Reviewed in-depth I see there are many things to optimize account.py

Getting compute
Getting children and consolidated
Getting _get_level account level
Getting _check_recursion

We have parent_left & parent_rigth without use.