MDL-83718 reportbuilder: use window count method to get table data.

Use the new "counted recordset" DML API from 42664ee 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.
mackensen · Nov 19, 2024 · 02186de · 02186de
1 parent 269a8a8
commit 02186de
Show file tree

Hide file tree

Showing 7 changed files with 54 additions and 34 deletions.
diff --git a/.upgradenotes/MDL-83718-2024111916571879.yml 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
@@ -77,7 +77,6 @@ protected static function define_other_properties(): array {
      * @return array
      */
     protected function get_other_values(renderer_base $output): array {
-        global $DB;
 
         /** @var datasource $report */
         $report = $this->related['report'];
@@ -101,7 +100,7 @@ protected function get_other_values(renderer_base $output): array {
         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
@@ -77,7 +77,6 @@ protected static function define_other_properties(): array {
      * @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 @@ protected function get_other_values(renderer_base $output): array {
         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
@@ -143,12 +143,11 @@ public static function get_schedule_report_users(model $schedule): array {
      * @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 @@ public static function get_schedule_report_file(model $schedule): stored_file {
         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
@@ -578,14 +578,11 @@ public function get_sort_fields(): array {
             }
 
             // 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
@@ -116,15 +116,6 @@ protected function init_sql(string $fields, string $from, array $joins, string $
         $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 @@ private function get_filter_sql(filter $filter, array $filtervalues, string $par
     /**
      * 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 @@ public function query_db($pagesize, $useinitialsbar = true): void {
         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
@@ -475,7 +475,7 @@ public function test_is_sortable(): void {
     /**
      * 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 @@ public function test_get_sortfields(): void {
     /**
      * 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 @@ public function test_get_sortfields_with_field_alias(): void {
         $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')