From ae649aadf03e1160a1b335a6570bdebd6353dc50 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 12 Mar 2024 09:16:35 +0530 Subject: [PATCH 1/6] fix: dont add useless distinct clause confuses query planner in some cases --- frappe/desk/reportview.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frappe/desk/reportview.py b/frappe/desk/reportview.py index 598648cc4c..df403c76a9 100644 --- a/frappe/desk/reportview.py +++ b/frappe/desk/reportview.py @@ -14,6 +14,7 @@ from frappe.model.base_document import get_controller from frappe.model.db_query import DatabaseQuery from frappe.model.utils import is_virtual_doctype from frappe.utils import add_user_info, cint, format_duration +from frappe.utils.data import sbool @frappe.whitelist() @@ -53,7 +54,8 @@ def get_count() -> int: controller = get_controller(args.doctype) data = frappe.call(controller.get_count, args=args, **args) else: - distinct = "distinct " if args.distinct == "true" else "" + args.distinct = sbool(args.distinct) + distinct = "distinct " if args.distinct else "" args.fields = [f"count({distinct}`tab{args.doctype}`.name) as total_count"] data = execute(**args)[0].get("total_count") From a49fafbf8e7fb01e589c8b918fef49bb1096351b Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 12 Mar 2024 09:25:21 +0530 Subject: [PATCH 2/6] feat: support countig till a limit In InnoDB counting is essentially O(n) operation, it can be pretty fast on indexes but when filters don't use any index it usually means doing a full table scan. Adding a limit will stop the scan as soon as that many records are matched. --- frappe/desk/reportview.py | 18 ++++++++++++++---- frappe/public/js/frappe/db.js | 2 ++ frappe/public/js/frappe/list/list_view.js | 1 + frappe/tests/test_db_query.py | 21 ++++++++++++++++++--- 4 files changed, 35 insertions(+), 7 deletions(-) diff --git a/frappe/desk/reportview.py b/frappe/desk/reportview.py index df403c76a9..5194f84008 100644 --- a/frappe/desk/reportview.py +++ b/frappe/desk/reportview.py @@ -52,14 +52,24 @@ def get_count() -> int: if is_virtual_doctype(args.doctype): controller = get_controller(args.doctype) - data = frappe.call(controller.get_count, args=args, **args) + count = frappe.call(controller.get_count, args=args, **args) else: args.distinct = sbool(args.distinct) distinct = "distinct " if args.distinct else "" - args.fields = [f"count({distinct}`tab{args.doctype}`.name) as total_count"] - data = execute(**args)[0].get("total_count") + args.limit = cint(args.limit) + if args.limit: + # Only "count until this limit" + args.fields = ["*"] + partial_query = execute(**args, run=0) + count = frappe.db.sql( + f"""with records as ( {partial_query} ) + select count(*) from records""", + )[0][0] + else: + args.fields = [f"count({distinct}`tab{args.doctype}`.name) as total_count"] + count = execute(**args)[0].get("total_count") - return data + return count def execute(doctype, *args, **kwargs): diff --git a/frappe/public/js/frappe/db.js b/frappe/public/js/frappe/db.js index 58329f16d7..b569205aa9 100644 --- a/frappe/public/js/frappe/db.js +++ b/frappe/public/js/frappe/db.js @@ -96,6 +96,7 @@ frappe.db = { }, count: function (doctype, args = {}) { let filters = args.filters || {}; + let limit = args.limit; // has a filter with childtable? const distinct = @@ -111,6 +112,7 @@ frappe.db = { filters, fields, distinct, + limit, }); }, get_link_options(doctype, txt = "", filters = {}) { diff --git a/frappe/public/js/frappe/list/list_view.js b/frappe/public/js/frappe/list/list_view.js index 6adf057391..47c5f1bbeb 100644 --- a/frappe/public/js/frappe/list/list_view.js +++ b/frappe/public/js/frappe/list/list_view.js @@ -946,6 +946,7 @@ frappe.views.ListView = class ListView extends frappe.views.BaseList { return frappe.db .count(this.doctype, { filters: this.get_filters_for_args(), + limit: 1001, }) .then((total_count) => { this.total_count = total_count || current_count; diff --git a/frappe/tests/test_db_query.py b/frappe/tests/test_db_query.py index 078bb55c42..f80f73c410 100644 --- a/frappe/tests/test_db_query.py +++ b/frappe/tests/test_db_query.py @@ -1187,7 +1187,7 @@ class TestReportView(FrappeTestCase): "distinct": "false", } ) - list_filter_response = execute_cmd("frappe.desk.reportview.get_count") + count = execute_cmd("frappe.desk.reportview.get_count") frappe.local.form_dict = frappe._dict( { "doctype": "DocType", @@ -1196,8 +1196,8 @@ class TestReportView(FrappeTestCase): } ) dict_filter_response = execute_cmd("frappe.desk.reportview.get_count") - self.assertIsInstance(list_filter_response, int) - self.assertEqual(list_filter_response, dict_filter_response) + self.assertIsInstance(count, int) + self.assertEqual(count, dict_filter_response) # test with child table filter frappe.local.form_dict = frappe._dict( @@ -1218,6 +1218,21 @@ class TestReportView(FrappeTestCase): )[0][0] self.assertEqual(child_filter_response, current_value) + # test with limit + limit = 2 + frappe.local.form_dict = frappe._dict( + { + "doctype": "DocType", + "filters": [["DocType", "is_virtual", "=", 1]], + "fields": [], + "distinct": "false", + "limit": limit, + } + ) + count = execute_cmd("frappe.desk.reportview.get_count") + self.assertIsInstance(count, int) + self.assertLessEqual(count, limit) + def test_reportview_get(self): user = frappe.get_doc("User", "test@example.com") add_child_table_to_blog_post() From 7e88c53aa485a8ea6799f74af4b184b473058fca Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 12 Mar 2024 09:36:58 +0530 Subject: [PATCH 3/6] perf: show estimated count on list view --- frappe/desk/reportview.py | 2 +- frappe/public/js/frappe/list/list_view.js | 13 +++++++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/frappe/desk/reportview.py b/frappe/desk/reportview.py index 5194f84008..528d06eeb5 100644 --- a/frappe/desk/reportview.py +++ b/frappe/desk/reportview.py @@ -59,7 +59,7 @@ def get_count() -> int: args.limit = cint(args.limit) if args.limit: # Only "count until this limit" - args.fields = ["*"] + args.fields = [f"`tab{args.doctype}`.name"] partial_query = execute(**args, run=0) count = frappe.db.sql( f"""with records as ( {partial_query} ) diff --git a/frappe/public/js/frappe/list/list_view.js b/frappe/public/js/frappe/list/list_view.js index 47c5f1bbeb..536ef602da 100644 --- a/frappe/public/js/frappe/list/list_view.js +++ b/frappe/public/js/frappe/list/list_view.js @@ -28,6 +28,7 @@ frappe.views.ListView = class ListView extends frappe.views.BaseList { this.process_document_refreshes.bind(this), 2000 ); + this.count_upper_bound = 1001; } has_permissions() { @@ -946,13 +947,21 @@ frappe.views.ListView = class ListView extends frappe.views.BaseList { return frappe.db .count(this.doctype, { filters: this.get_filters_for_args(), - limit: 1001, + limit: this.count_upper_bound, }) .then((total_count) => { this.total_count = total_count || current_count; this.count_without_children = count_without_children !== current_count ? count_without_children : undefined; - let str = __("{0} of {1}", [current_count, this.total_count]); + + let estimated_count = this.total_count; + if (this.total_count === this.count_upper_bound) { + estimated_count = `${format_number(estimated_count - 1, null, 0)}+`; + } + let str = __("{0} of {1}", [ + format_number(current_count, null, 0), + estimated_count, + ]); if (this.count_without_children) { str = __("{0} of {1} ({2} rows with children)", [ count_without_children, From fdcff2d9e88a3726f60665218756e950f7581a90 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 12 Mar 2024 09:56:57 +0530 Subject: [PATCH 4/6] fix(UX): let user see actual count on click --- frappe/public/js/frappe/list/list_view.js | 40 ++++++++++++++++------- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/frappe/public/js/frappe/list/list_view.js b/frappe/public/js/frappe/list/list_view.js index 536ef602da..3a3a239545 100644 --- a/frappe/public/js/frappe/list/list_view.js +++ b/frappe/public/js/frappe/list/list_view.js @@ -617,11 +617,29 @@ frappe.views.ListView = class ListView extends frappe.views.BaseList { } render_count() { - if (!this.list_view_settings.disable_count) { - this.get_count_str().then((str) => { - this.$result.find(".list-count").html(`${str}`); - }); - } + if (this.list_view_settings.disable_count) return; + + this.get_count_str().then((str) => { + let me = this; + let count_element = this.$result.find(".list-count"); + count_element.html(`${str}`); + if (this.count_upper_bound) { + count_element.attr( + "title", + __( + "The count shown is an estimated count. Click here to see the accurate count." + ) + ); + count_element.tooltip({ delay: { show: 600, hide: 100 }, trigger: "hover" }); + count_element.on("click", () => { + me.count_upper_bound = 0; + count_element.off("click"); + count_element.tooltip("disable"); + me.freeze(); + me.render_count(); + }); + } + }); } get_header_html() { @@ -954,14 +972,14 @@ frappe.views.ListView = class ListView extends frappe.views.BaseList { this.count_without_children = count_without_children !== current_count ? count_without_children : undefined; - let estimated_count = this.total_count; + let count_str; if (this.total_count === this.count_upper_bound) { - estimated_count = `${format_number(estimated_count - 1, null, 0)}+`; + count_str = `${format_number(count_str - 1, null, 0)}+`; + } else { + count_str = format_number(this.total_count, null, 0); } - let str = __("{0} of {1}", [ - format_number(current_count, null, 0), - estimated_count, - ]); + + let str = __("{0} of {1}", [format_number(current_count, null, 0), count_str]); if (this.count_without_children) { str = __("{0} of {1} ({2} rows with children)", [ count_without_children, From 1fa7cc7816e19b4bf919c1fe0381589fc4c498c0 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 12 Mar 2024 12:06:11 +0530 Subject: [PATCH 5/6] fix: support child tables in count with limit --- frappe/desk/reportview.py | 12 +++++------- frappe/public/js/frappe/list/list_view.js | 2 +- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/frappe/desk/reportview.py b/frappe/desk/reportview.py index 528d06eeb5..3891b78f43 100644 --- a/frappe/desk/reportview.py +++ b/frappe/desk/reportview.py @@ -57,16 +57,14 @@ def get_count() -> int: args.distinct = sbool(args.distinct) distinct = "distinct " if args.distinct else "" args.limit = cint(args.limit) + fieldname = f"{distinct}`tab{args.doctype}`.name" + if args.limit: - # Only "count until this limit" - args.fields = [f"`tab{args.doctype}`.name"] + args.fields = [fieldname] partial_query = execute(**args, run=0) - count = frappe.db.sql( - f"""with records as ( {partial_query} ) - select count(*) from records""", - )[0][0] + count = frappe.db.sql(f"""select count(*) from ( {partial_query} ) p""")[0][0] else: - args.fields = [f"count({distinct}`tab{args.doctype}`.name) as total_count"] + args.fields = [f"count({fieldname}) as total_count"] count = execute(**args)[0].get("total_count") return count diff --git a/frappe/public/js/frappe/list/list_view.js b/frappe/public/js/frappe/list/list_view.js index 3a3a239545..f64c4e8d55 100644 --- a/frappe/public/js/frappe/list/list_view.js +++ b/frappe/public/js/frappe/list/list_view.js @@ -974,7 +974,7 @@ frappe.views.ListView = class ListView extends frappe.views.BaseList { let count_str; if (this.total_count === this.count_upper_bound) { - count_str = `${format_number(count_str - 1, null, 0)}+`; + count_str = `${format_number(this.total_count - 1, null, 0)}+`; } else { count_str = format_number(this.total_count, null, 0); } From 698ef95ca1735b9cbc66f219280fb02882f44dfa Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 12 Mar 2024 13:06:17 +0530 Subject: [PATCH 6/6] refactor: avoid duplicate render_count for report view Only difference is element --- cypress/integration/list_paging.js | 2 +- frappe/public/js/frappe/list/list_view.js | 28 +++++++++++-------- .../public/js/frappe/views/file/file_view.js | 1 + .../js/frappe/views/reports/report_view.js | 15 ++++------ 4 files changed, 23 insertions(+), 23 deletions(-) diff --git a/cypress/integration/list_paging.js b/cypress/integration/list_paging.js index cd4e2bc68d..494ca6ae74 100644 --- a/cypress/integration/list_paging.js +++ b/cypress/integration/list_paging.js @@ -37,6 +37,6 @@ context("List Paging", () => { cy.get(".list-paging-area .list-count").should("contain.text", "500 of"); cy.get(".list-paging-area .btn-more").click(); - cy.get(".list-paging-area .list-count").should("contain.text", "1000 of"); + cy.get(".list-paging-area .list-count").should("contain.text", "1,000 of"); }); }); diff --git a/frappe/public/js/frappe/list/list_view.js b/frappe/public/js/frappe/list/list_view.js index f64c4e8d55..5c2cabfe54 100644 --- a/frappe/public/js/frappe/list/list_view.js +++ b/frappe/public/js/frappe/list/list_view.js @@ -501,9 +501,9 @@ frappe.views.ListView = class ListView extends frappe.views.BaseList { freeze() { if (this.list_view_settings && !this.list_view_settings.disable_count) { - this.$result - .find(".list-count") - .html(`${__("Refreshing", null, "Document count in list view")}...`); + this.get_count_element().html( + `${__("Refreshing", null, "Document count in list view")}...` + ); } } @@ -619,22 +619,22 @@ frappe.views.ListView = class ListView extends frappe.views.BaseList { render_count() { if (this.list_view_settings.disable_count) return; - this.get_count_str().then((str) => { - let me = this; - let count_element = this.$result.find(".list-count"); - count_element.html(`${str}`); + let me = this; + let $count = this.get_count_element(); + this.get_count_str().then((count) => { + $count.html(`${count}`); if (this.count_upper_bound) { - count_element.attr( + $count.attr( "title", __( "The count shown is an estimated count. Click here to see the accurate count." ) ); - count_element.tooltip({ delay: { show: 600, hide: 100 }, trigger: "hover" }); - count_element.on("click", () => { + $count.tooltip({ delay: { show: 600, hide: 100 }, trigger: "hover" }); + $count.on("click", () => { me.count_upper_bound = 0; - count_element.off("click"); - count_element.tooltip("disable"); + $count.off("click"); + $count.tooltip("disable"); me.freeze(); me.render_count(); }); @@ -642,6 +642,10 @@ frappe.views.ListView = class ListView extends frappe.views.BaseList { }); } + get_count_element() { + return this.$result.find(".list-count"); + } + get_header_html() { if (!this.columns) { return; diff --git a/frappe/public/js/frappe/views/file/file_view.js b/frappe/public/js/frappe/views/file/file_view.js index fb2a12a268..de787e10c6 100644 --- a/frappe/public/js/frappe/views/file/file_view.js +++ b/frappe/public/js/frappe/views/file/file_view.js @@ -231,6 +231,7 @@ frappe.views.FileView = class FileView extends frappe.views.ListView { } else { super.render(); this.render_header(); + this.render_count(); } } diff --git a/frappe/public/js/frappe/views/reports/report_view.js b/frappe/public/js/frappe/views/reports/report_view.js index 725ebd06a0..8e54c43c97 100644 --- a/frappe/public/js/frappe/views/reports/report_view.js +++ b/frappe/public/js/frappe/views/reports/report_view.js @@ -220,19 +220,14 @@ frappe.views.ReportView = class ReportView extends frappe.views.ListView { this.setup_datatable(this.data); } - render_count() { - if (this.list_view_settings?.disable_count) { - return; - } - let $list_count = this.$paging_area.find(".list-count"); - if (!$list_count.length) { - $list_count = $("") + get_count_element() { + let $count = this.$paging_area.find(".list-count"); + if (!$count.length) { + $count = $("") .addClass("text-muted list-count") .prependTo(this.$paging_area.find(".level-right")); } - this.get_count_str().then((str) => { - $list_count.text(str); - }); + return $count; } on_update(data) {