Skip to content

Commit

Permalink
perf: Faster report exporting logic (frappe#21415)
Browse files Browse the repository at this point in the history
* refactor: Correctly consider extra row in visible_idx

This logic didn't consider reason why +1 is needed.

* perf: Faster report exports to Excel

Try exporting report with 100K rows, most likely it fails or takes a really long time.

Root causes:
- visible_idx was a list, lookups are SLOW AF when lists grow to 100k+
- visible_idx check is not required when I am exporting entire report.

Test with 85,000 rows.

|         | Before | After | Improvement |
| ---     | ---    | ---   | ---         |
| Export  | 25.99  | 0.77  | ~33x faster |
  • Loading branch information
ankush authored Jun 17, 2023
1 parent 53b074c commit e6d8912
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 1 deletion.
7 changes: 7 additions & 0 deletions frappe/desk/query_report.py
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,13 @@ def build_xlsx_data(data, visible_idx, include_indentation, ignore_visible_idx=F
datetime.timedelta,
)

if len(visible_idx) == len(data.result):
# It's not possible to have same length and different content.
ignore_visible_idx = True
else:
# Note: converted for faster lookups
visible_idx = set(visible_idx)

result = [[]]
column_widths = []

Expand Down
3 changes: 2 additions & 1 deletion frappe/public/js/frappe/views/reports/query_report.js
Original file line number Diff line number Diff line change
Expand Up @@ -1466,7 +1466,8 @@ frappe.views.QueryReport = class QueryReport extends frappe.views.BaseList {
}

const visible_idx = this.datatable.bodyRenderer.visibleRowIndices;
if (visible_idx.length + 1 === this.data.length) {
// data.length will be +1 if add total row is enabled.
if (this.report_doc.add_total_row) {
visible_idx.push(visible_idx.length);
}

Expand Down

0 comments on commit e6d8912

Please sign in to comment.