MergeDataTables does not correctly merge if table2 has more data than table1 #15294
Conversation
…make sure to correctly copy child datatables.
diosmosis
added
Bug
| @@ -52,4 +52,23 @@ public function mergeDataTables($table1, $table2) | |||
| } | |||
| } | |||
|
|
|||
| private function makeNewDataTable(DataTable\DataTableInterface $subTable2) | |||
| { | |||
| if ($subTable2 instanceof DataTable\Map) { | |||
There was a problem hiding this comment.
@diosmosis wondering if we would need to create a clone and keep any filters? Not sure though
There was a problem hiding this comment.
I guess we could. Might be an issue if filters hold references to the old table or to some other object that isn't relevant anymore (ie, like ones that calculate percent over a total or specific column value in another table or something like that). Actually, it's probably better to require this class be used before filters are run, since if metrics are formatted they won't be able to be added properly...
There was a problem hiding this comment.
It shouldn't hold a reference to the table but possibly to some other parameters.
There was a problem hiding this comment.
Some filters do hold references to other tables, eg, CalculateEvolutionFilter. It is conceivable that they could hold onto a lot of data.
Ideally it would be great to deprecate queued filters and instead move that logic to a method in the Report class, like, formatApiResponse(), similar to configureView() or whatever it's called.
| $subTables1 = $table1->getDataTables(); | ||
| foreach ($table2->getDataTables() as $index => $subTable2) { | ||
| if (!array_key_exists($index, $subTables1)) { | ||
| $subTable1 = $this->makeNewDataTable($subTable2); |
There was a problem hiding this comment.
@diosmosis reading the previous comment in #5168 (comment) I wonder if this could cause any issues with potentially wrong data when archiving overlaps another day basically? Not sure I fully understand #5168 . I suppose if it was a problem, it would need to be handled somewhere else maybe and not in this class as when merging you expect both tables to be merged as it is implemented in this PR
There was a problem hiding this comment.
So:
- archive starts on one day sending request for 'today'
- new day begins, archive finishes, 'today' value changes
then some bug happens? I'm not sure I understand either.
There was a problem hiding this comment.
as you said, "if it was a problem, it would need to be handled somewhere else maybe and not in this class as when merging you expect both tables to be merged as it is implemented in this PR" -> so sounds good to proceed with this proper fix and we can see if the problem in #5168 starts appearing again
|
Would it be possible adding some unit tests or so? |
|
👍 can add unit tests |
|
@diosmosis looks like you forgot to add the unit tests in 1c83ed2 ? |
… table1 (matomo-org#15294) * update submodules to fix build * Handle DataTable Maps in VisitFrequency API and in DataTable merger, make sure to correctly copy child datatables. * fix some tests * Copy metadata over. * Updated expected test results. * Adding unit tests.
… table1 (matomo-org#15294) * update submodules to fix build * Handle DataTable Maps in VisitFrequency API and in DataTable merger, make sure to correctly copy child datatables. * fix some tests * Copy metadata over. * Updated expected test results. * Adding unit tests.
... & fix build.
When fixing tests I noticed a bug exposed by the new VisitFrequency metrics. MergeDataTables will only iterate over the first table when merging tables, which means if there are tables that exist in table2 only, they won't get added.