Faster User ID reports processing: use ranking query #14551
Conversation
| ); | ||
|
|
||
| if ($query === false) { | ||
| return; | ||
| } | ||
|
|
||
| $rowsCount = 0; | ||
| while ($row = $query->fetch()) { | ||
| foreach ($query as $row) { |
|
haven't tested it but looks good. This test seems unrelated https://travis-ci.org/matomo-org/matomo/jobs/548604100#L925 |
|
Be great if we can merge this soon and release rc2 👍 |
mattab
added
the
c: Performance
plugins/UserId/Archiver.php
Outdated
| $configLimit = $configGeneral['archiving_ranking_query_row_limit']; | ||
| $limit = $configLimit == 0 ? 0 : max( | ||
| $configLimit, | ||
| $configGeneral['datatable_archiving_maximum_rows_standard'] |
There was a problem hiding this comment.
- let's introduce a different setting eg
datatable_archiving_maximum_rows_userid - set this to a higher value like 5000 rows (i expect users will want to archive more rows in this report.)
There was a problem hiding this comment.
@mattab changed & build fixed. Had to add another order by to make the results deterministic.
| <nb_visits_converted>0</nb_visits_converted> | ||
|
|
||
| </row> | ||
| <row> |
There was a problem hiding this comment.
Would be useful if the tests could set a lower limit like 10, and show the behavior works (show the row Others)
There was a problem hiding this comment.
It does, that's the whole point of this test. See https://github.com/matomo-org/matomo/pull/14551/files#diff-5ca2964519f1aaf3fb5ca033dcb48da0
… summary row which is not treated as datatable summary row.
| // deal w/ ranking query summary row | ||
| $rankingQuerySummaryRow = $dataTable->getRowFromLabel(DataTable::LABEL_SUMMARY_ROW); | ||
| if ($rankingQuerySummaryRow) { | ||
| $rankingQuerySummaryRowId = $dataTable->getRowIdFromLabel(DataTable::LABEL_SUMMARY_ROW); |
There was a problem hiding this comment.
can you explain why this code is needed, and do we do this kind of logic in other archivers too (which ones)? if not, why is it needed in this archiver?
There was a problem hiding this comment.
Ranking query creates a row w/ label -1, this is added to the DataArray which is just an array, not a datatable so it has no concept of a summary row. When DataArray::asDataTable() is called, it adds the row as a regular row and not as a summary row.
There was a problem hiding this comment.
Ok, wondering why is this logic not needed for other plugins?
There was a problem hiding this comment.
It is required, Actions does it here: https://github.com/matomo-org/matomo/blob/3.x-dev/plugins/Actions/ArchivingHelper.php#L383
It's not used in Events/Contents, but that's likely a bug that wasn't dealt with since it's not tested in BlobReportLimitingTest. I'll add it to my list to fix that.
There was a problem hiding this comment.
@diosmosis do you mind creating an issue for this bug in case it later slips off the list? unless you will open the PR this week or so. Thanks!
mattab
changed the title
No description provided.