@sgiehl opened this Pull Request on March 4th 2020 Member

fixes #15588

@sgiehl commented on March 5th 2020 Member

@tsteur applied the feedback

@tsteur commented on March 5th 2020 Member

tested it locally and worked. I suppose these failing tests are not related to this? https://travis-ci.org/matomo-org/matomo/jobs/658576281#L1228

@tsteur commented on March 5th 2020 Member

feel free to merge if the test failures are unrelated

@sgiehl commented on March 6th 2020 Member

Seems the test failures are related, as they are not failing on 3.x-dev. Will investigate that now

@sgiehl commented on March 6th 2020 Member

@tsteur I looked into the test failures now for a while. It might be a case that only appears in that test scenario, as all visits have the same visitorId and some even have the same time. Fetching the visitor then returns different results as forcing the index causes the result being sorted by the index order (for same visitorId and datetime). Not using the index sorts those entries by the order they were stored in the table. So basically that "change" only occurs if there are multiple entries for the same visitorid and datetime, which likely shouldn't occur ever I guess 🤔

I've adjusted the failing test now, so it uses different visitor ids and time. That way it works the same with or without the index...

@tsteur commented on March 7th 2020 Member

Not sure I understand @sgiehl . In https://github.com/matomo-org/matomo/pull/15664/files#diff-c27b9aad61da66057fe4abcbdc1c9f67R3 there is quite a big difference in the number of unique visitors for example. This wouldn't be quite expected if it was just a sorting issue?

Seems very tricky. I can imagine as you say that it picks a different visit now when they have same visitorId and datetime, but the number of uniq visitors to be so different is maybe bit unexpected? Not quite sure how to explain this. The other ones can be indeed because if this and this should usually not happen or at least not cause any issue. Thought could sort by visit_last_action_time, idvisit but this would be overkill for something that usually doesn't happen and shouldn't do it.

Do you have a thought how the nb_uniq_visitors change can be explained?

@sgiehl commented on March 7th 2020 Member

@tsteur that's because the fixture is changed to force new visitorIds and datetimes to avoid having duplicate entries for those at all. The tests would now work with the forced index and without

@tsteur commented on March 8th 2020 Member

There are still some tests failing @sgiehl LGTM otherwise

@sgiehl commented on March 14th 2020 Member

tests should be fixed now

This Pull Request was closed on March 15th 2020
Powered by GitHub Issue Mirror