diff --git a/core/RankingQuery.php b/core/RankingQuery.php index dafad92757e..8c859499a75 100644 --- a/core/RankingQuery.php +++ b/core/RankingQuery.php @@ -293,33 +293,49 @@ public function generateRankingQuery($innerQuery, bool $withRollup = false) // generate select clauses for label columns $labelColumnsString = '`' . implode('`, `', array_keys($this->labelColumns)) . '`'; - $labelColumnsOthersSwitch = array(); - $withRollupColumns = array(); + + $labelColumnsOthersSwitch = []; + $withRollupColumns = []; + $withRollupOthersGroupBy = []; foreach (array_keys($this->labelColumns) as $column) { - $rollupWhen = ''; if ($withRollup) { - $rollupLimitValue = empty($withRollupColumns) ? - "'" . $this->othersLabelValue . "'" - : - 'NULL'; - - $rollupWhen = " - WHEN counterRollup = $limit THEN $rollupLimitValue - WHEN counterRollup > 0 THEN `$column` + if ([] === $withRollupColumns) { + // support "Others" row for first label column + $rollupWhen = " + WHEN counterRollup = $limit THEN '" . $this->othersLabelValue . "' + WHEN counterRollup > 0 THEN `$column` + WHEN counter = $limit AND counterRollup = 0 THEN `$column` + WHEN counter = $limit THEN '" . $this->othersLabelValue . "' + "; + } else { + // support "Others" row for secondary label columns + $rollupWhen = " + WHEN `$column` IS NULL THEN NULL + WHEN counter = $limit AND counterRollup = 0 THEN '" . $this->othersLabelValue . "' + "; + } + + $switch = " + CASE + $rollupWhen + ELSE `$column` + END "; - $withRollupColumns[] = $column; + $labelColumnsOthersSwitch[] = "$switch AS `$column`"; + $withRollupColumns[] = $column; + $withRollupOthersGroupBy[] = $switch; + } else { + $labelColumnsOthersSwitch[] = " + CASE + WHEN counter = $limit THEN '" . $this->othersLabelValue . "' + ELSE `$column` + END AS `$column` + "; } - - $labelColumnsOthersSwitch[] = " - CASE - $rollupWhen - WHEN counter = $limit THEN '" . $this->othersLabelValue . "' - ELSE `$column` - END AS `$column` - "; } + $labelColumnsOthersSwitch = implode(', ', $labelColumnsOthersSwitch); // generate select clauses for additional columns @@ -346,21 +362,21 @@ public function generateRankingQuery($innerQuery, bool $withRollup = false) $counterRollupExpression = ''; - if ($withRollup && !empty($withRollupColumns)) { + if ($withRollup) { $initCounter .= ' ( SELECT @counterRollup:=0 ) initCounterRollup,'; $counterRollupWhen = ''; if (count($withRollupColumns) >= 2) { $counterRollupWhen = " WHEN `" . implode('` IS NULL AND `', $withRollupColumns) . "` IS NULL THEN -1 - "; + "; } foreach ($withRollupColumns as $withRollupColumn) { $counterRollupWhen .= " WHEN `$withRollupColumn` IS NULL AND @counterRollup = $limit THEN $limit WHEN `$withRollupColumn` IS NULL THEN @counterRollup := @counterRollup + 1 - "; + "; } $counterRollupExpression = " @@ -368,7 +384,7 @@ public function generateRankingQuery($innerQuery, bool $withRollup = false) $counterRollupWhen ELSE 0 END AS counterRollup - "; + "; } if (false === strpos($innerQuery, ' LIMIT ') && !Schema::getInstance()->supportsSortingInSubquery()) { @@ -389,7 +405,7 @@ public function generateRankingQuery($innerQuery, bool $withRollup = false) ( $innerQuery ) actualQuery "; - if ($withRollup && !empty($withRollupColumns) && !Schema::getInstance()->supportsRankingRollupWithoutExtraSorting()) { + if ($withRollup && !Schema::getInstance()->supportsRankingRollupWithoutExtraSorting()) { // MariaDB requires an additional sorting layer to return // the counter/counterRollup values we expect $rollupColumnSorts = []; @@ -410,13 +426,16 @@ public function generateRankingQuery($innerQuery, bool $withRollup = false) // group by the counter - this groups "Others" because the counter stops at $limit $groupBy = 'counter'; - if ($withRollup && !empty($counterRollupExpression)) { - $groupBy .= ', counterRollup'; + if ($withRollup) { + // group rollups additionally by the rollup counter and the + // full "Others" switch to ensure correct secondary level "Others" calculation + $groupBy .= ', counterRollup, ' . implode(', ', $withRollupOthersGroupBy); } if ($this->partitionColumn !== false) { $groupBy .= ', `' . $this->partitionColumn . '`'; } + $groupOthers = " SELECT $labelColumnsOthersSwitch @@ -425,13 +444,13 @@ public function generateRankingQuery($innerQuery, bool $withRollup = false) GROUP BY $groupBy "; - if (!Schema::getInstance()->supportsSortingInSubquery()) { + if ($withRollup) { + // Sort the final result if a rollup was used + // to ensure rollup values are returned first, and "Others" last + $groupOthers .= " ORDER BY counter, counterRollup"; + } elseif (!Schema::getInstance()->supportsSortingInSubquery()) { // When subqueries aren't sorted, we need to sort the result manually again $groupOthers .= " ORDER BY counter"; - - if (!empty($counterRollupExpression)) { - $groupOthers .= ', counterRollup'; - } } return $groupOthers; diff --git a/plugins/CustomDimensions/RecordBuilders/CustomDimension.php b/plugins/CustomDimensions/RecordBuilders/CustomDimension.php index 09fb81fcfce..180f2063ef4 100644 --- a/plugins/CustomDimensions/RecordBuilders/CustomDimension.php +++ b/plugins/CustomDimensions/RecordBuilders/CustomDimension.php @@ -186,8 +186,6 @@ protected function aggregateFromActions(DataTable $report, LogAggregator $logAgg $metricIds[] = Metrics::INDEX_BOUNCE_COUNT; $metricIds[] = Metrics::INDEX_PAGE_EXIT_NB_VISITS; - $actionRows = []; - while ($row = $resultSet->fetch()) { if (!isset($row[Metrics::INDEX_NB_VISITS])) { return; @@ -195,80 +193,58 @@ protected function aggregateFromActions(DataTable $report, LogAggregator $logAgg $label = $row[$valueField]; - if ($withRollup) { - $url = $row['url']; - - if (is_null($label)) { - continue; - } - - if (!is_null($url)) { - $actionRows[] = $row; - continue; - } + if ($withRollup && $label === null) { + // top-level rollup result + continue; } $columns = []; + foreach ($metricIds as $id) { $columns[$id] = (float) ($row[$id] ?? 0); } + $label = $this->cleanCustomDimensionValue($label); - $tableRow = $report->sumRowWithLabel($label, $columns); + $url = $row['url']; if (!$withRollup) { - $url = $row['url']; - if (empty($url)) { - continue; - } - - // make sure we always work with normalized URL no matter how the individual action stores it - $normalized = Tracker\PageUrl::normalizeUrl($url); - $url = $normalized['url']; - - if (empty($url)) { - continue; - } - - $tableRow->sumRowWithLabelToSubtable($url, $columns); - } - } - - if ($withRollup) { - foreach ($actionRows as $row) { - if (!isset($row[Metrics::INDEX_NB_VISITS])) { - return; - } - - $label = $row[$valueField]; - $url = $row['url']; - - if (is_null($label) || is_null($url)) { + $tableRow = $report->sumRowWithLabel($label, $columns); + } else { + if ($url === null) { + // second-level rollup result + $report->sumRowWithLabel($label, $columns); continue; } - $label = $this->cleanCustomDimensionValue($label); $tableRow = $report->getRowFromLabel($label); - if (empty($tableRow)) { - continue; - } - - // make sure we always work with normalized URL no matter how the individual action stores it - $normalized = Tracker\PageUrl::normalizeUrl($url); - $url = $normalized['url']; - - if (empty($url)) { + if (false === $tableRow) { + // non-rollup row but rollup row is missing + // should not happen, but don't break continue; } + } - $columns = []; + // make sure we always work with normalized URL no matter how the individual action stores it + $normalized = Tracker\PageUrl::normalizeUrl($url); + $url = $normalized['url']; - foreach ($metricIds as $id) { - $columns[$id] = (float) ($row[$id] ?? 0); - } + if (empty($url)) { + continue; + } - $tableRow->sumRowWithLabelToSubtable($url, $columns); + if ( + $withRollup + && $url === RankingQuery::LABEL_SUMMARY_ROW + && !$tableRow->isSubtableLoaded() + ) { + // skip creating the subtable if: + // - we are using rollups + // - the only row would be "Others" + continue; } + + $tableRow->sumRowWithLabelToSubtable($url, $columns); } } diff --git a/plugins/CustomDimensions/tests/System/expected/test_ranking_limit_with_rollup_by_archiving_query__CustomDimensions.getCustomDimension_day.xml b/plugins/CustomDimensions/tests/System/expected/test_ranking_limit_with_rollup_by_archiving_query__CustomDimensions.getCustomDimension_day.xml index 4a2511f4ba3..b7486e49481 100644 --- a/plugins/CustomDimensions/tests/System/expected/test_ranking_limit_with_rollup_by_archiving_query__CustomDimensions.getCustomDimension_day.xml +++ b/plugins/CustomDimensions/tests/System/expected/test_ranking_limit_with_rollup_by_archiving_query__CustomDimensions.getCustomDimension_day.xml @@ -168,6 +168,48 @@ dimension1==site3+group5;actionUrl=$example.com%2Fpage%2F5%2F5 example.com%2Fpage%2F5%2F5 + + + 2 + 2 + 53 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 2 + 0 + 0% + 100% + dimension1==site3+group5;actionUrl=$__mtm_ranking_query_others__ + __mtm_ranking_query_others__ + diff --git a/plugins/CustomDimensions/tests/System/expected/test_ranking_limit_with_rollup_by_archiving_query_expanded__CustomDimensions.getCustomDimension_day.xml b/plugins/CustomDimensions/tests/System/expected/test_ranking_limit_with_rollup_by_archiving_query_expanded__CustomDimensions.getCustomDimension_day.xml index 4a2511f4ba3..b7486e49481 100644 --- a/plugins/CustomDimensions/tests/System/expected/test_ranking_limit_with_rollup_by_archiving_query_expanded__CustomDimensions.getCustomDimension_day.xml +++ b/plugins/CustomDimensions/tests/System/expected/test_ranking_limit_with_rollup_by_archiving_query_expanded__CustomDimensions.getCustomDimension_day.xml @@ -168,6 +168,48 @@ dimension1==site3+group5;actionUrl=$example.com%2Fpage%2F5%2F5 example.com%2Fpage%2F5%2F5 + + + 2 + 2 + 53 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 2 + 0 + 0% + 100% + dimension1==site3+group5;actionUrl=$__mtm_ranking_query_others__ + __mtm_ranking_query_others__ + diff --git a/plugins/Referrers/RecordBuilders/AIReferrers.php b/plugins/Referrers/RecordBuilders/AIReferrers.php index c4eb82c0fc2..d8da35d883f 100644 --- a/plugins/Referrers/RecordBuilders/AIReferrers.php +++ b/plugins/Referrers/RecordBuilders/AIReferrers.php @@ -88,40 +88,30 @@ protected function aggregateFromVisits(LogAggregator $logAggregator, DataTable $ { $resultSet = $this->queryAIReferrerEntryPages($logAggregator, $field); - $actionRows = []; - while ($row = $resultSet->fetch()) { if (!isset($row[Metrics::INDEX_NB_VISITS])) { return; } $label = $row['referer_name']; - $pageUrlOrTitle = $row['action_name']; - if (is_null($label)) { + if ($label === null) { + // top-level rollup result continue; } - if (!is_null($pageUrlOrTitle)) { - $actionRows[] = $row; + if ($pageUrlOrTitle === null) { + // second-level rollup result + $report->sumRowWithLabel($label, $this->makeVisitRow($row)); continue; } - $report->sumRowWithLabel($label, $this->makeVisitRow($row)); - } - - foreach ($actionRows as $row) { - if (!isset($row[Metrics::INDEX_NB_VISITS])) { - return; - } - - $label = $row['referer_name']; - $pageUrlOrTitle = $row['action_name']; - $tableRow = $report->getRowFromLabel($label); - if (empty($tableRow)) { + if (false === $tableRow) { + // non-rollup row but rollup row is missing + // should not happen, but don't break continue; } @@ -131,6 +121,16 @@ protected function aggregateFromVisits(LogAggregator $logAggregator, DataTable $ $pageUrlOrTitle = $normalized['url']; } + if ( + $pageUrlOrTitle === RankingQuery::LABEL_SUMMARY_ROW + && !$tableRow->isSubtableLoaded() + ) { + // skip creating the subtable if: + // - we are using rollups + // - the only row would be "Others" + continue; + } + $tableRow->sumRowWithLabelToSubtable($pageUrlOrTitle, $this->makeVisitRow($row)); } } diff --git a/tests/PHPUnit/System/RankingQueryTest.php b/tests/PHPUnit/System/RankingQueryTest.php new file mode 100644 index 00000000000..0c5adfb8c5b --- /dev/null +++ b/tests/PHPUnit/System/RankingQueryTest.php @@ -0,0 +1,235 @@ +idSite); + $period = Period\Factory::build('month', self::$fixture->dateTime); + $segment = new Segment('', [$site->getId()]); + $params = new Parameters($site, $period, $segment); + + $this->logAggregator = new LogAggregator($params); + } + + /** + * Make sure the results are consistently structured across database vendors. + * + * We always expect the same order, with second-level results ordered by the given base query: + * + * 1. top-level rollup (both labels NULL) + * 2. first-level rollups (secondary label NULL) + * 3. first-level "Others" + * 4. second-level results + * 5. second-level "Others" + * + * @dataProvider getRollupResultStructureTestData + */ + public function testRollupResultStructure(string $sortOrder, array $expectedResultSet): void + { + $select = ' + log_visit.config_browser_engine AS config_browser_engine, + log_visit.custom_var_v1 AS custom_var_v1, + COUNT(log_visit.idvisit) AS nb_visits + '; + + $where = ' + log_visit.visit_last_action_time >= ? + AND log_visit.visit_last_action_time <= ? + AND log_visit.idsite IN (?) + AND custom_var_v1 IS NOT NULL + '; + + $from = 'log_visit'; + $groupBy = 'config_browser_engine, custom_var_v1'; + $orderBy = "nb_visits $sortOrder, config_browser_engine, custom_var_v1"; + + $query = $this->logAggregator->generateQuery( + $select, + $from, + $where, + $groupBy, + $orderBy, + $limit = 0, + $offset = 0, + true + ); + + $rankingQuery = new RankingQuery(3); + $rankingQuery->addLabelColumn(['config_browser_engine', 'custom_var_v1']); + $rankingQuery->addColumn(['nb_visits'], 'sum'); + + $query['sql'] = $rankingQuery->generateRankingQuery($query['sql'], true); + $resultSet = $this->logAggregator->getDb()->query($query['sql'], $query['bind'])->fetchAll(); + + self::assertEquals($expectedResultSet, $resultSet); + } + + public function getRollupResultStructureTestData(): iterable + { + yield 'nb_visits DESC' => [ + 'DESC', + [ + [ + 'config_browser_engine' => null, + 'custom_var_v1' => null, + 'nb_visits' => 30, + ], + [ + 'config_browser_engine' => 'Gecko', + 'custom_var_v1' => null, + 'nb_visits' => 20, + ], + [ + 'config_browser_engine' => 'Blink', + 'custom_var_v1' => null, + 'nb_visits' => 4, + ], + [ + 'config_browser_engine' => 'WebKit', + 'custom_var_v1' => null, + 'nb_visits' => 4, + ], + [ + 'config_browser_engine' => RankingQuery::LABEL_SUMMARY_ROW, + 'custom_var_v1' => null, + 'nb_visits' => 2, + ], + [ + 'config_browser_engine' => 'Gecko', + 'custom_var_v1' => 'Cvar1 value is 1', + 'nb_visits' => 4, + ], + [ + 'config_browser_engine' => 'Blink', + 'custom_var_v1' => 'Cvar1 value is 0', + 'nb_visits' => 3, + ], + [ + 'config_browser_engine' => 'Gecko', + 'custom_var_v1' => 'Cvar1 value is 0', + 'nb_visits' => 2, + ], + [ + 'config_browser_engine' => 'Blink', + 'custom_var_v1' => RankingQuery::LABEL_SUMMARY_ROW, + 'nb_visits' => 1, + ], + [ + 'config_browser_engine' => 'Gecko', + 'custom_var_v1' => RankingQuery::LABEL_SUMMARY_ROW, + 'nb_visits' => 14, + ], + [ + 'config_browser_engine' => 'Trident', + 'custom_var_v1' => RankingQuery::LABEL_SUMMARY_ROW, + 'nb_visits' => 2, + ], + [ + 'config_browser_engine' => 'WebKit', + 'custom_var_v1' => RankingQuery::LABEL_SUMMARY_ROW, + 'nb_visits' => 4, + ], + ], + ]; + + yield 'nb_visits ASC' => [ + 'ASC', + [ + [ + 'config_browser_engine' => null, + 'custom_var_v1' => null, + 'nb_visits' => 30, + ], + [ + 'config_browser_engine' => 'Trident', + 'custom_var_v1' => null, + 'nb_visits' => 2, + ], + [ + 'config_browser_engine' => 'Blink', + 'custom_var_v1' => null, + 'nb_visits' => 4, + ], + [ + 'config_browser_engine' => 'WebKit', + 'custom_var_v1' => null, + 'nb_visits' => 4, + ], + [ + 'config_browser_engine' => RankingQuery::LABEL_SUMMARY_ROW, + 'custom_var_v1' => null, + 'nb_visits' => 20, + ], + [ + 'config_browser_engine' => 'Blink', + 'custom_var_v1' => 'Cvar1 value is 3', + 'nb_visits' => 1, + ], + [ + 'config_browser_engine' => 'Trident', + 'custom_var_v1' => 'Cvar1 value is 1', + 'nb_visits' => 1, + ], + [ + 'config_browser_engine' => 'Trident', + 'custom_var_v1' => 'Cvar1 value is 2', + 'nb_visits' => 1, + ], + [ + 'config_browser_engine' => 'Blink', + 'custom_var_v1' => RankingQuery::LABEL_SUMMARY_ROW, + 'nb_visits' => 3, + ], + [ + 'config_browser_engine' => 'Gecko', + 'custom_var_v1' => RankingQuery::LABEL_SUMMARY_ROW, + 'nb_visits' => 20, + ], + [ + 'config_browser_engine' => 'WebKit', + 'custom_var_v1' => RankingQuery::LABEL_SUMMARY_ROW, + 'nb_visits' => 4, + ], + ], + ]; + } +} + +RankingQueryTest::$fixture = new ManyVisitsWithGeoIP(); diff --git a/tests/PHPUnit/Unit/RankingQueryTest.php b/tests/PHPUnit/Unit/RankingQueryTest.php index d54f4e3dc23..04305f04376 100644 --- a/tests/PHPUnit/Unit/RankingQueryTest.php +++ b/tests/PHPUnit/Unit/RankingQueryTest.php @@ -12,12 +12,13 @@ use Piwik\Db\Schema; use Piwik\RankingQuery; +/** + * @group Core + * @group RankingQuery + */ class RankingQueryTest extends \PHPUnit\Framework\TestCase { - /** - * @group Core - */ - public function testBasic() + public function testBasic(): void { $query = new RankingQuery(); $query->setOthersLabel('Others'); @@ -82,10 +83,7 @@ public function testBasic() $this->checkQuery($query, $innerQuery, $expected); } - /** - * @group Core - */ - public function testBasicWithRollup() + public function testBasicWithRollup(): void { $query = new RankingQuery(); $query->setOthersLabel('Others'); @@ -109,13 +107,13 @@ public function testBasicWithRollup() CASE WHEN counterRollup = 11 THEN 'Others' WHEN counterRollup > 0 THEN `label` + WHEN counter = 11 AND counterRollup = 0 THEN `label` WHEN counter = 11 THEN 'Others' ELSE `label` END AS `label`, CASE - WHEN counterRollup = 11 THEN NULL - WHEN counterRollup > 0 THEN `url` - WHEN counter = 11 THEN 'Others' + WHEN `url` IS NULL THEN NULL + WHEN counter = 11 AND counterRollup = 0 THEN 'Others' ELSE `url` END AS `url`, `column`, @@ -151,7 +149,20 @@ public function testBasicWithRollup() ORDER BY `column` ) actualQuery ) AS withCounter - GROUP BY counter, counterRollup + GROUP BY counter, counterRollup, + CASE + WHEN counterRollup = 11 THEN 'Others' + WHEN counterRollup > 0 THEN `label` + WHEN counter = 11 AND counterRollup = 0 THEN `label` + WHEN counter = 11 THEN 'Others' + ELSE `label` + END, + CASE + WHEN `url` IS NULL THEN NULL + WHEN counter = 11 AND counterRollup = 0 THEN 'Others' + ELSE `url` + END + ORDER BY counter, counterRollup "; if (!Schema::getInstance()->supportsSortingInSubquery()) { @@ -160,13 +171,13 @@ public function testBasicWithRollup() CASE WHEN counterRollup = 11 THEN 'Others' WHEN counterRollup > 0 THEN `label` + WHEN counter = 11 AND counterRollup = 0 THEN `label` WHEN counter = 11 THEN 'Others' ELSE `label` END AS `label`, CASE - WHEN counterRollup = 11 THEN NULL - WHEN counterRollup > 0 THEN `url` - WHEN counter = 11 THEN 'Others' + WHEN `url` IS NULL THEN NULL + WHEN counter = 11 AND counterRollup = 0 THEN 'Others' ELSE `url` END AS `url`, `column`, @@ -203,7 +214,19 @@ public function testBasicWithRollup() LIMIT 18446744073709551615 ) actualQuery ) AS withCounter - GROUP BY counter, counterRollup + GROUP BY counter, counterRollup, + CASE + WHEN counterRollup = 11 THEN 'Others' + WHEN counterRollup > 0 THEN `label` + WHEN counter = 11 AND counterRollup = 0 THEN `label` + WHEN counter = 11 THEN 'Others' + ELSE `label` + END, + CASE + WHEN `url` IS NULL THEN NULL + WHEN counter = 11 AND counterRollup = 0 THEN 'Others' + ELSE `url` + END ORDER BY counter, counterRollup "; } @@ -214,13 +237,13 @@ public function testBasicWithRollup() CASE WHEN counterRollup = 11 THEN 'Others' WHEN counterRollup > 0 THEN `label` + WHEN counter = 11 AND counterRollup = 0 THEN `label` WHEN counter = 11 THEN 'Others' ELSE `label` END AS `label`, CASE - WHEN counterRollup = 11 THEN NULL - WHEN counterRollup > 0 THEN `url` - WHEN counter = 11 THEN 'Others' + WHEN `url` IS NULL THEN NULL + WHEN counter = 11 AND counterRollup = 0 THEN 'Others' ELSE `url` END AS `url`, `column`, @@ -257,17 +280,27 @@ public function testBasicWithRollup() ) actualQuery ORDER BY `label` IS NULL, `url` IS NULL, `column` ) AS withCounter - GROUP BY counter, counterRollup + GROUP BY counter, counterRollup, + CASE + WHEN counterRollup = 11 THEN 'Others' + WHEN counterRollup > 0 THEN `label` + WHEN counter = 11 AND counterRollup = 0 THEN `label` + WHEN counter = 11 THEN 'Others' + ELSE `label` + END, + CASE + WHEN `url` IS NULL THEN NULL + WHEN counter = 11 AND counterRollup = 0 THEN 'Others' + ELSE `url` + END + ORDER BY counter, counterRollup "; } $this->checkQuery($query, $innerQuery, $expected, true); } - /** - * @group Core - */ - public function testExcludeRows() + public function testExcludeRows(): void { $query = new RankingQuery(20); @@ -335,10 +368,7 @@ public function testExcludeRows() $this->checkQuery($query, $innerQuery, $expected); } - /** - * @group Core - */ - public function testPartitionResult() + public function testPartitionResult(): void { $query = new RankingQuery(1000); $query->setOthersLabel('Others');