Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance improvement for OCA accounting reports #219

Merged
merged 1 commit into from
Sep 9, 2016

Conversation

jcoux
Copy link

@jcoux jcoux commented Sep 7, 2016

This PR complete the PR #211 with a performance improvement for OCA accounting reports.

  • Deletion of joins between tables with "OR" clauses
  • Deletion of useless data generation of General Ledger report for Trial Balance report
  • Customization of xlsxwriter options to limit usage of memory for xlsx files generation

@@ -3,6 +3,8 @@
# Copyright 2016 Camptocamp SA
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html).

from cStringIO import StringIO
import xlsxwriter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this dependency here? It should be in other module

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @pedrobaeza

I have added the cStringIO and xlsxwriter dependencies because I have override the create_xlslx_report method for "Customization of xlsxwriter options to limit usage of memory for xlsx files generation".

See lines 38 to 51 of this file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then the previous PR was incorrect. We can't mix XLSX generation with PDF generation. That should be optional through a module account_financial_report_qweb_xlsx

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pedrobaeza , you're right on this point.
But it's not the subject of this PR.
I will try to do another PR for that, but I don't know when I could do that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thanks

@fclementic2c
Copy link
Member

2015 Trial balance (PDF) out in 3 min pdf on a 6.2 millions account move lines database... much better

👍

@fclementic2c
Copy link
Member

fclementic2c commented Sep 7, 2016

and...balance calculation validated !
kudos @jcoux for your great work !
Now we have financial reports in version 9 : fast and reliable :)

cc @jgrandguillaume

@sbidoul
Copy link
Member

sbidoul commented Sep 7, 2016

@fclementic2c @jcoux @jgrandguillaume out of curiosity, would you have a chance to time a trial balance with mis_builder on the same database? Or this method:

def get_balances_variation(cls, company, date_from, date_to,

@gurneyalex
Copy link
Member

worked with julien on this. 👍

@fclementic2c
Copy link
Member

@sbidoul I ll do the test on the same DB and PC and I will tell you.

For now we need this merged so let's do it.

@fclementic2c fclementic2c merged commit 13d4310 into OCA:9.0 Sep 9, 2016
@sbidoul
Copy link
Member

sbidoul commented Sep 9, 2016

@fclementic2c sure, I did not intend to slow this PR. Pure curiosity, nothing else.

Thanks for finishing this, BTW!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants