New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Force using index index_idsite_idvisitor if available #15664
Conversation
@tsteur applied the feedback |
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 |
*/ | ||
public static function tableHasIndex($table, $indexName) | ||
{ | ||
$result = Db::get()->fetchOne('SHOW INDEX FROM '.$table.' WHERE Key_name = ?', [$indexName]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was thinking could maybe catch an exception here but this would just hide errors I suppose. So be good to keep it for now like this I reckon
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 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 |
There are still some tests failing @sgiehl LGTM otherwise |
tests should be fixed now |
fixes #15588