From 02186de904d2982def225f71b9ce36b5e7f8661f Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Tue, 19 Nov 2024 16:03:30 +0000 Subject: [PATCH] MDL-83718 reportbuilder: use window count method to get table data. Use the new "counted recordset" DML API from 42664ee49a to obtain the raw table data for reports. For those databases with defined support for count window functions, this should give a performance benefit by combining the count and main query into one request. --- .upgradenotes/MDL-83718-2024111916571879.yml | 9 +++++ .../external/custom_report_data_exporter.php | 3 +- .../external/system_report_data_exporter.php | 3 +- .../classes/local/helpers/schedule.php | 6 ++-- reportbuilder/classes/local/report/column.php | 13 +++---- .../classes/table/base_report_table.php | 36 +++++++++++-------- .../tests/local/report/column_test.php | 18 ++++++++-- 7 files changed, 54 insertions(+), 34 deletions(-) create mode 100644 .upgradenotes/MDL-83718-2024111916571879.yml diff --git a/.upgradenotes/MDL-83718-2024111916571879.yml b/.upgradenotes/MDL-83718-2024111916571879.yml new file mode 100644 index 00000000000..ee1b8fffc85 --- /dev/null +++ b/.upgradenotes/MDL-83718-2024111916571879.yml @@ -0,0 +1,9 @@ +issueNumber: MDL-83718 +notes: + core_reportbuilder: + - message: >- + Report table instances no longer populate the `countsql` and + `countparams` class properties. Instead calling code can access + `totalrows` to obtain the same value, rather than manually counting via + the prior properties + type: changed diff --git a/reportbuilder/classes/external/custom_report_data_exporter.php b/reportbuilder/classes/external/custom_report_data_exporter.php index 3faf5777e2c..c39765ba106 100644 --- a/reportbuilder/classes/external/custom_report_data_exporter.php +++ b/reportbuilder/classes/external/custom_report_data_exporter.php @@ -77,7 +77,6 @@ class custom_report_data_exporter extends exporter { * @return array */ protected function get_other_values(renderer_base $output): array { - global $DB; /** @var datasource $report */ $report = $this->related['report']; @@ -101,7 +100,7 @@ class custom_report_data_exporter extends exporter { return [ 'headers' => $table->headers, 'rows' => $tablerows, - 'totalrowcount' => $DB->count_records_sql($table->countsql, $table->countparams), + 'totalrowcount' => $table->totalrows, ]; } } diff --git a/reportbuilder/classes/external/system_report_data_exporter.php b/reportbuilder/classes/external/system_report_data_exporter.php index 58389e28fc7..e8d1e1ab411 100644 --- a/reportbuilder/classes/external/system_report_data_exporter.php +++ b/reportbuilder/classes/external/system_report_data_exporter.php @@ -77,7 +77,6 @@ class system_report_data_exporter extends exporter { * @return array */ protected function get_other_values(renderer_base $output): array { - global $DB; /** @var system_report $report */ $report = $this->related['report']; @@ -109,7 +108,7 @@ class system_report_data_exporter extends exporter { return [ 'headers' => array_values($tableheaders), 'rows' => $tablerows, - 'totalrowcount' => $DB->count_records_sql($table->countsql, $table->countparams), + 'totalrowcount' => $table->totalrows, ]; } } diff --git a/reportbuilder/classes/local/helpers/schedule.php b/reportbuilder/classes/local/helpers/schedule.php index 86ba8d5b6cd..e92498f38a2 100644 --- a/reportbuilder/classes/local/helpers/schedule.php +++ b/reportbuilder/classes/local/helpers/schedule.php @@ -143,12 +143,11 @@ class schedule { * @return int */ public static function get_schedule_report_count(model $schedule): int { - global $DB; - $table = custom_report_table_view::create($schedule->get('reportid')); $table->setup(); + $table->query_db(0, false); - return $DB->count_records_sql($table->countsql, $table->countparams); + return $table->totalrows; } /** @@ -163,7 +162,6 @@ class schedule { require_once("{$CFG->libdir}/filelib.php"); $table = custom_report_table_view::create($schedule->get('reportid')); - $table->setup(); $table->query_db(0, false); diff --git a/reportbuilder/classes/local/report/column.php b/reportbuilder/classes/local/report/column.php index 54501bc91f5..f0d96f85aff 100644 --- a/reportbuilder/classes/local/report/column.php +++ b/reportbuilder/classes/local/report/column.php @@ -578,14 +578,11 @@ final class column { } // Check whether sortfield refers to field SQL. - foreach ($fieldsalias as $field) { - if (strcasecmp($sortfield, $field['sql']) === 0) { - $sortfield = $field['alias']; - break; - } - } - - return $sortfield; + return str_ireplace( + array_column($fieldsalias, 'sql'), + array_column($fieldsalias, 'alias'), + $sortfield, + ); }, $this->sortfields); } diff --git a/reportbuilder/classes/table/base_report_table.php b/reportbuilder/classes/table/base_report_table.php index 3ca83e63b27..7bf5c954fa9 100644 --- a/reportbuilder/classes/table/base_report_table.php +++ b/reportbuilder/classes/table/base_report_table.php @@ -116,15 +116,6 @@ abstract class base_report_table extends table_sql implements dynamic, renderabl $from .= ' ' . implode(' ', array_unique($joins)); $this->set_sql($fields, $from, $wheresql, $params); - - $counttablealias = database::generate_alias(); - $this->set_count_sql(" - SELECT COUNT(1) - FROM (SELECT {$fields} - FROM {$from} - WHERE {$wheresql} - {$this->groupbysql} - ) {$counttablealias}", $params); } /** @@ -165,13 +156,13 @@ abstract class base_report_table extends table_sql implements dynamic, renderabl /** * Generate suitable SQL for the table * + * @param bool $includesort * @return string */ - protected function get_table_sql(): string { + protected function get_table_sql(bool $includesort = true): string { $sql = "SELECT {$this->sql->fields} FROM {$this->sql->from} WHERE {$this->sql->where} {$this->groupbysql}"; - $sort = $this->get_sql_sort(); - if ($sort) { + if ($includesort && ($sort = $this->get_sql_sort())) { $sql .= " ORDER BY {$sort}"; } @@ -188,10 +179,25 @@ abstract class base_report_table extends table_sql implements dynamic, renderabl global $DB; if (!$this->is_downloading()) { - $this->pagesize($pagesize, $DB->count_records_sql($this->countsql, $this->countparams)); - $this->rawdata = $DB->get_recordset_sql($this->get_table_sql(), $this->sql->params, $this->get_page_start(), - $this->get_page_size()); + // Initially set the page size, so the following SQL read has correct values. + $this->pagesize($pagesize, 0); + + $countedcolumn = database::generate_alias(); + $countedrecordset = $DB->get_counted_recordset_sql( + $this->get_table_sql(false), + $countedcolumn, + $this->get_sql_sort(), + $this->sql->params, + (int) $this->get_page_start(), + (int) $this->get_page_size(), + ); + + // Now set the total page size. + $countedsize = (int) ($countedrecordset->current()->{$countedcolumn} ?? 0); + $this->pagesize($pagesize, $countedsize); + + $this->rawdata = $countedrecordset; } else { $this->rawdata = $DB->get_recordset_sql($this->get_table_sql(), $this->sql->params); } diff --git a/reportbuilder/tests/local/report/column_test.php b/reportbuilder/tests/local/report/column_test.php index 8ad01c67e20..10b82f62745 100644 --- a/reportbuilder/tests/local/report/column_test.php +++ b/reportbuilder/tests/local/report/column_test.php @@ -475,7 +475,7 @@ final class column_test extends advanced_testcase { /** * Test retrieving sort fields */ - public function test_get_sortfields(): void { + public function test_get_sort_fields(): void { $column = $this->create_column('test') ->set_index(1) ->add_fields('t.foo, t.bar, t.baz') @@ -487,7 +487,7 @@ final class column_test extends advanced_testcase { /** * Test retrieving sort fields when an aliased field is set as sortable */ - public function test_get_sortfields_with_field_alias(): void { + public function test_get_sort_fields_with_field_alias(): void { $column = $this->create_column('test') ->set_index(1) ->add_field('t.foo') @@ -497,10 +497,22 @@ final class column_test extends advanced_testcase { $this->assertEquals(['c1_lionel'], $column->get_sort_fields()); } + /** + * Test retrieving sort fields that contain references to fields within complex snippet + */ + public function test_get_sort_fields_complex(): void { + $column = $this->create_column('test') + ->set_index(1) + ->add_fields('t.foo, t.bar') + ->set_is_sortable(true, ['CASE WHEN 1=1 THEN t.foo ELSE t.bar END']); + + $this->assertEquals(['CASE WHEN 1=1 THEN c1_foo ELSE c1_bar END'], $column->get_sort_fields()); + } + /** * Test retrieving sort fields when an unknown field is set as sortable */ - public function test_get_sortfields_unknown_field(): void { + public function test_get_sort_fields_unknown_field(): void { $column = $this->create_column('test') ->set_index(1) ->add_fields('t.foo')