make sure summary rows are serialized with their in-db subtable ID and not in memory one #17831
Conversation
…d not in memory one
diosmosis
added
Bug
There was a problem hiding this comment.
@diosmosis does this mean maybe it also has an impact on some other usages of getRowsWithoutSummaryRow() and getSummaryRow()?
We separated this at some point as getRows() was causing memory issues.
Do you know how this later detects the summary row in methods like addRowsFromSerializedArray where it relied on having the summary row ID?
I'm not sure I fully understand the issue yet.
|
@tsteur from the blame it looks like we were never serializing the summary row. The specific line was last changed seven years ago: https://github.com/matomo-org/matomo/blame/b77e5ff594cdae59862dd4d863220baffc200bec/core/DataTable.php#L1323 And the summaryRow property has been there for nine years. So I think this has always been an issue.
We serialize the rows array which associates id w/ row data, so we simply unserialize it. The subtable ID after directly unserializing is used to get the subtable in the database. So it detects it because of the ID we serialize it with. I'm not sure if this answers your questions... |
There was a problem hiding this comment.
I understand now 👍 and the ID is afterwards still the same because the summary row has that ID anyway. This way we export also subtables of summary rows.
I do remember in https://github.com/matomo-org/matomo/pull/7577/files#diff-6542f24c55ecdc44d36de949a6c0be12f5a4e9ebcb064ae5a78e3a8cdb6e1b28R1269 years back making changes there for improving performance.
Just to be safe we're not making things slower would it be too bad to duplicate the code there (as it's executed a lot with a lot of data) like
if (isset($this->summaryRow)) {
if (isset($consecutiveSubtableIds[self::ID_SUMMARY_ROW])) {
$backup = $row->subtableId;
$row->subtableId = $consecutiveSubtableIds[self::ID_SUMMARY_ROW];
$rows[self::ID_SUMMARY_ROW] = $this->summaryRow->export();
$row->subtableId = $backup;
} else {
$rows[self::ID_SUMMARY_ROW] = $this->summaryRow->export();
}
}
|
@tsteur 👍 that makes sense, to avoid the array operation |
|
applied review feedback |
Description:
As title, for summary rows we are currently serializing the row with the incorrect subtable ID, causing it not to load w/ the subtable.
Review