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
feel free to merge if the test failures are unrelated
Seems the test failures are related, as they are not failing on 3.x-dev. Will investigate that now
@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...
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?
@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
tests should be fixed now