Bug 21560 - Optimize ODS exports
Summary: Optimize ODS exports
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Fridolin SOMERS
QA Contact: Martin Renvoize
URL:
Keywords:
: 13847 (view as bug list)
Depends on: 11603 11679
Blocks:
  Show dependency treegraph
 
Reported: 2018-10-12 12:07 UTC by Fridolin SOMERS
Modified: 2019-03-26 06:31 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21560: report ODS export optimization (3.49 KB, patch)
2018-10-12 12:27 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 21560: report ODS export optimization (3.53 KB, patch)
2018-10-13 09:13 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 21560: report ODS export optimization (3.59 KB, patch)
2018-10-17 00:55 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 21560: Optimize ODS export of gather_print_notices.pl (3.49 KB, patch)
2018-12-07 09:33 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 21560: report ODS export optimization (3.64 KB, patch)
2019-01-21 09:12 UTC, David Nind
Details | Diff | Splinter Review
Bug 21560: Optimize ODS export of gather_print_notices.pl (3.54 KB, patch)
2019-01-21 09:13 UTC, David Nind
Details | Diff | Splinter Review
Bug 21560: report ODS export optimization (3.70 KB, patch)
2019-02-04 11:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21560: Optimize ODS export of gather_print_notices.pl (3.60 KB, patch)
2019-02-04 11:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21560: create Koha::Util::OpenDocument with subroutine for ODS generation (9.63 KB, patch)
2019-02-19 11:21 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 21560: Unit test for Koha::Util::OpenDocument (1.97 KB, patch)
2019-02-19 11:21 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 21560: (follow-up) move use at the top (1.41 KB, patch)
2019-03-05 14:52 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 21560: report ODS export optimization (3.70 KB, patch)
2019-03-12 12:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21560: Optimize ODS export of gather_print_notices.pl (3.60 KB, patch)
2019-03-12 12:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21560: create Koha::Util::OpenDocument with subroutine for ODS generation (9.68 KB, patch)
2019-03-12 12:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21560: Unit test for Koha::Util::OpenDocument (2.03 KB, patch)
2019-03-12 12:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21560: (follow-up) move use at the top (1.48 KB, patch)
2019-03-12 12:18 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin SOMERS 2018-10-12 12:07:57 UTC
SQL reports can be exported with CSV, TSV of ODS format.
When report has thousands of rows, using ODS format is around 10 times longer than other formats.
It also loads CPU and RAM a lot.

The longest call is expandTable().
I found that the call on OpenOffice-OODoc lib can be optimized.
Based on https://grep.metacpan.org/search?qci=&q=expandTable&qft=&qd=OpenOffice-OODoc&f=examples%2Ftext2table
Comment 1 Fridolin SOMERS 2018-10-12 12:27:12 UTC Comment hidden (obsolete)
Comment 2 David Nind 2018-10-13 09:10:26 UTC
Tested on BibLibre sandbox six with two different reports - these were selected randomly.

Records with item count (https://wiki.koha-community.org/wiki/SQL_Reports_Library#Records_with_item_count): 4843 results; csv - about 3 seconds; ods - about 20 seconds before patch and 9 seconds after patch applied.

Accession Register Sorted by Barcode Number Report (https://wiki.koha-community.org/wiki/SQL_Reports_Library#Accession_Register_Sorted_by_Barcode_Number_Report): 6015 results; csv - about 2.5 seconds; ods - about 30 seconds before patch and 13 seconds after patch applied.
Comment 3 sandboxes@biblibre.com 2018-10-13 09:13:13 UTC
Patch tested with a sandbox, by David Nind <david@davidnind.com>
Comment 4 sandboxes@biblibre.com 2018-10-13 09:13:39 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2018-10-16 17:27:41 UTC
Fridolin, I think it would make sense to fix misc/cronjobs/gather_print_notices.pl as well on the same bug report.
Comment 6 Chris Cormack 2018-10-17 00:55:45 UTC Comment hidden (obsolete)
Comment 7 Fridolin SOMERS 2018-10-17 13:08:14 UTC
(In reply to Jonathan Druart from comment #5)
> Fridolin, I think it would make sense to fix
> misc/cronjobs/gather_print_notices.pl as well on the same bug report.

Indeed, I will create a second patch
Comment 8 Fridolin SOMERS 2018-12-07 09:33:33 UTC Comment hidden (obsolete)
Comment 9 David Nind 2019-01-21 09:12:58 UTC Comment hidden (obsolete)
Comment 10 David Nind 2019-01-21 09:13:01 UTC Comment hidden (obsolete)
Comment 11 David Nind 2019-01-21 09:17:41 UTC
An updated test plan for the second patch (Optimize ODS export of gather_print_notices.pl)

Test plan
---------

Pre-requisites: To test the gather_print_notices.pl patch you need to setup some overdue items to generate overdue notices. The basic steps to do this are included in the test plan.

1) Define your ODUE notice for the print template (Tools > Patrons and circulation > Notices & slips > edit the ODUE notice > expand the Print section):
   cardnumber:patron:email:item
   <<borrowers.cardnumber>>:<<borrowers.firstname>> <<borrowers.surname>>:<<borrowers.email>>:<item><<items.barcode>></item>
2) Define the overdue rules for a patron category, for example:
   . Check that 'Overdue notice required' is set to 'Yes' for the patron category (Administration > Patrons and circulation > Patron categories)
   . Check that the 'Loan period' for the 'Standard rules all libraries' is set to 5 days (Administration > Patrons and circulation > Circulation and fine rules)
   . Check that a trigger is set for the patron category, for example: Delay = 1, Letter = Overdue Notice, Print = ticked (Tools > Patrons and circulation > Overview notice/status triggers)
3) Check out 2 items for a patron in the same patron category defined in step 2 - set the due date so that it will generate the overdue notices. (Circulation > Check out > enter patron details (card number or partial name) > enter a barcode for an item > specifiy a due date by expanding 'Checkout settings' and entering a date at least 5 days before the current date > Check out > confirm check out.)
4) Run the overdue notices cronjob from the command line: perl misc/cronjobs/overdue_notices.pl
5) Check that the 'message_queue' table in the database contains 2 new entries (from the command line 'koha-mysql kohadev' and then enter the query 'select * from message_queue;')
6) Run the gather_print_notices cronjob from the command line and record how long it takes to run (use  the time command* if available to record this): time perl misc/cronjobs/gather_print_notices.pl /tmp --ods --letter_code=ODUE -d=:
7) An ods file should be generated in the /tmp directory
8) Apply the patch
9) Run step 6 again to get the time after the patch is applied

* If using the bash shell and the time package is installed you can record how long a script takes to run.

Result before the patch was applied:
real    0m1.702s
user    0m1.336s
sys     0m0.172s

Result after the patch was applied:
real    0m1.541s
user    0m1.404s
sys     0m0.132s

Note: got several different times, but all were quicker.
Comment 12 Martin Renvoize 2019-02-04 11:20:12 UTC
Created attachment 84693 [details] [review]
Bug 21560: report ODS export optimization

SQL reports can be exported with CSV, TSV of ODS format.
When report has thousands of rows, using ODS format is around 10 times longer than other formats.
It also loads CPU and RAM a lot.

The longest call is expandTable().
I found that the call on OpenOffice-OODoc lib can be optimized.
Based on https://grep.metacpan.org/search?qci=&q=expandTable&qft=&qd=OpenOffice-OODoc&f=examples%2Ftext2table

Test plan :
1) Don't apply patch yet
2) Create a new SQL report : Home > Reports > Create from SQL
3) Enter a SQL that will return thousands of results
4) Run report
5) Click Download > Semicolon separated text (.csv), look execution time
6) Click Download > Open Document Spreadsheet, look execution time
7) Apply patch
8) Redo 5) and 6) and compare times, CSV export should be the same but ODS export should be better

My tests shows 1,5 seconds for CSV export.
And for ODS export : 18 seconds without patch and 8 seconds with patch.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2019-02-04 11:20:16 UTC
Created attachment 84694 [details] [review]
Bug 21560: Optimize ODS export of gather_print_notices.pl

Like previous patch, the call on OpenOffice-OODoc lib can be optimized.
Based on https://grep.metacpan.org/search?qci=&q=expandTable&qft=&qd=OpenOffice-OODoc&f=examples%2Ftext2table

Test plan inspired from Bug 11679.

Test plan:
- define your ODUE notice for the print template as:
cardnumber:patron:email:item
<<borrowers.cardnumber>>:<<borrowers.firstname>> <<borrowers.surname>>:<<borrowers.email>>:<item><<items.barcode>></item>
- define overdues rules for a patron category
- check-out 2 items using a due date in order to generate the overdue notices
- check-in these 2 items
- launch the script /misc/cronjobs/overdue_notices.pl
- the 'message_queue' table should now contain 2 new entries
- launch the gather_print_notices cronjob :
  perl misc/cronjobs/gather_print_notices.pl /tmp/test --ods --letter_code=OVERDUE -d=:
- A ods file should be generated in your /tmp/test directory
- Compare times with and without patch

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2019-02-04 11:21:04 UTC
Nice little optimization, passing QA scripts and no regressions found..

Passed QA
Comment 15 Nick Clemens 2019-02-04 14:06:18 UTC
This seems to be the same code in two scripts - any chance we could move this to a subroutine?
Comment 16 Fridolin SOMERS 2019-02-08 16:30:33 UTC
(In reply to Nick Clemens from comment #15)
> This seems to be the same code in two scripts - any chance we could move
> this to a subroutine?

Good idea.
But can this be pushed and I create a new Bug for a subroutine ?
Comment 17 Fridolin SOMERS 2019-02-18 15:39:48 UTC
I'm on a subroutine
Comment 18 Fridolin SOMERS 2019-02-19 11:21:08 UTC
Created attachment 85264 [details] [review]
Bug 21560: create Koha::Util::OpenDocument with subroutine for ODS generation

There is nearly the same code in misc/cronjobs/gather_print_notices.pl and reports/guided_reports.pl.
This patch creates a new module with subroutine called by both scripts.

If file path or content is undefined, subroutine will just return undef.
If content is an empty arrayref, empty ods file is still generated.

Unicode encoding is kept outisde because it may not be necessary.

Note that for print notices the first line of message is always the column names so it is extracted only from first message.

Test plan :
Run previous patches test plans and check ODS files are the same.
Comment 19 Fridolin SOMERS 2019-02-19 11:21:20 UTC
Created attachment 85265 [details] [review]
Bug 21560: Unit test for Koha::Util::OpenDocument

Run prove t/Koha/Util/OpenDocument.t
Comment 20 Owen Leonard 2019-02-22 15:31:05 UTC
Should this be considered a duplicate of Bug 13847?
Comment 21 Fridolin SOMERS 2019-02-22 17:50:40 UTC
(In reply to Owen Leonard from comment #20)
> Should this be considered a duplicate of Bug 13847?

I think yes Bug 13847 can be closed as duplicate of this one
Comment 22 Owen Leonard 2019-02-22 17:53:50 UTC
*** Bug 13847 has been marked as a duplicate of this bug. ***
Comment 23 Martin Renvoize 2019-02-26 10:14:22 UTC
Comment on attachment 85264 [details] [review]
Bug 21560: create Koha::Util::OpenDocument with subroutine for ODS generation

Review of attachment 85264 [details] [review]:
-----------------------------------------------------------------

Generally, this is a nice improvement and refactors thanks.. one tiny remark on two lines and I think we can push this over the finish line.

::: misc/cronjobs/gather_print_notices.pl
@@ +275,5 @@
>          }
>      }
> +
> +    # Process
> +    use Koha::Util::OpenDocument;

I realise the script has inlined the use statements previously, but any chance we could use this opportunity to tidy up a little and put this at the top of the file for consistency.. I nearly failed the patch thinking it was missing.

::: reports/guided_reports.pl
@@ +938,4 @@
>                  }
>  
> +                # Process
> +                use Koha::Util::OpenDocument;

Another case of can we move the use statement to the top.
Comment 24 Fridolin SOMERS 2019-03-05 14:48:05 UTC
> I realise the script has inlined the use statements previously, but any
> chance we could use this opportunity to tidy up a little and put this at the
> top of the file for consistency.. I nearly failed the patch thinking it was
> missing.

OK I'm on it
Comment 25 Fridolin SOMERS 2019-03-05 14:52:18 UTC
Created attachment 86084 [details] [review]
Bug 21560: (follow-up) move use at the top
Comment 26 Martin Renvoize 2019-03-12 12:18:15 UTC
Created attachment 86515 [details] [review]
Bug 21560: report ODS export optimization

SQL reports can be exported with CSV, TSV of ODS format.
When report has thousands of rows, using ODS format is around 10 times longer than other formats.
It also loads CPU and RAM a lot.

The longest call is expandTable().
I found that the call on OpenOffice-OODoc lib can be optimized.
Based on https://grep.metacpan.org/search?qci=&q=expandTable&qft=&qd=OpenOffice-OODoc&f=examples%2Ftext2table

Test plan :
1) Don't apply patch yet
2) Create a new SQL report : Home > Reports > Create from SQL
3) Enter a SQL that will return thousands of results
4) Run report
5) Click Download > Semicolon separated text (.csv), look execution time
6) Click Download > Open Document Spreadsheet, look execution time
7) Apply patch
8) Redo 5) and 6) and compare times, CSV export should be the same but ODS export should be better

My tests shows 1,5 seconds for CSV export.
And for ODS export : 18 seconds without patch and 8 seconds with patch.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Martin Renvoize 2019-03-12 12:18:19 UTC
Created attachment 86516 [details] [review]
Bug 21560: Optimize ODS export of gather_print_notices.pl

Like previous patch, the call on OpenOffice-OODoc lib can be optimized.
Based on https://grep.metacpan.org/search?qci=&q=expandTable&qft=&qd=OpenOffice-OODoc&f=examples%2Ftext2table

Test plan inspired from Bug 11679.

Test plan:
- define your ODUE notice for the print template as:
cardnumber:patron:email:item
<<borrowers.cardnumber>>:<<borrowers.firstname>> <<borrowers.surname>>:<<borrowers.email>>:<item><<items.barcode>></item>
- define overdues rules for a patron category
- check-out 2 items using a due date in order to generate the overdue notices
- check-in these 2 items
- launch the script /misc/cronjobs/overdue_notices.pl
- the 'message_queue' table should now contain 2 new entries
- launch the gather_print_notices cronjob :
  perl misc/cronjobs/gather_print_notices.pl /tmp/test --ods --letter_code=OVERDUE -d=:
- A ods file should be generated in your /tmp/test directory
- Compare times with and without patch

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Martin Renvoize 2019-03-12 12:18:22 UTC
Created attachment 86517 [details] [review]
Bug 21560: create Koha::Util::OpenDocument with subroutine for ODS generation

There is nearly the same code in misc/cronjobs/gather_print_notices.pl and reports/guided_reports.pl.
This patch creates a new module with subroutine called by both scripts.

If file path or content is undefined, subroutine will just return undef.
If content is an empty arrayref, empty ods file is still generated.

Unicode encoding is kept outisde because it may not be necessary.

Note that for print notices the first line of message is always the column names so it is extracted only from first message.

Test plan :
Run previous patches test plans and check ODS files are the same.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 29 Martin Renvoize 2019-03-12 12:18:26 UTC
Created attachment 86518 [details] [review]
Bug 21560: Unit test for Koha::Util::OpenDocument

Run prove t/Koha/Util/OpenDocument.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Martin Renvoize 2019-03-12 12:18:29 UTC
Created attachment 86519 [details] [review]
Bug 21560: (follow-up) move use at the top

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Martin Renvoize 2019-03-12 12:19:38 UTC
Nice lot of tidying and optimising here.. going back to PQA after the cleanups.. Thanks Fridolin!
Comment 32 Nick Clemens 2019-03-15 12:47:15 UTC
Awesome work all!

Pushed to master for 19.05
Comment 33 Martin Renvoize 2019-03-19 09:37:18 UTC
Pushed to 18.11.x for 18.11.04
Comment 34 Lucas Gass 2019-03-19 23:15:26 UTC
backported to 18.05.x for 18.05.11
Comment 35 Fridolin SOMERS 2019-03-26 06:31:17 UTC
Pushed to 17.11.x for 17.11.17