Skip to content
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

Fix Sort filters are sometimes applied multiple times #7452

Merged
merged 3 commits into from Mar 17, 2015
Merged

Fix Sort filters are sometimes applied multiple times #7452

merged 3 commits into from Mar 17, 2015

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Mar 17, 2015

see #7388

We should only apply a sort filter once, meaning it will be much faster since a sort takes a long time on many rows (many seconds under circumstances). The only way to apply the filter only once is by applying it in GenericFilters. This means we will fallback to a default sort column, specified in a report class, if no column was specifically set.

This does not yet actually fix the issue #7388 . We should probably also apply the default sort column to more reports such as Actions. We might also expose more settings to a report like "defaultSortOrder", "preferNaturalSort", ... This is to be discussed. I do not want to expose this Report::$defaultSortColumn as an API yet since I am not sure whether this is the best solution. We probably need a 'DefaultSortConfig` for reports that allows to specify "defaultSortOrder", "preferNaturalSort", "defaultSortColumn", "defaultSecondarySortColumn", ... once we merge #7420 and work on #7401. Further steps are to be discussed.

Please note that this also fixes some issues in the Sort filter. I could have created a separate pull request for it, but it was kinda needed to fix it here since otherwise it would sometimes return a wrong result. Some fixed bugs include:

  • make sure to select correct column (the column value might be false which is valid, meaning column actually exists whereas we assumed before it does not exist)
  • use correct sort algorithm (if value of first column was false we picked under circumstances a string comparison instead of number)
  • If we sort by label, use always a string or natural comparison even if the label is numeric

I had to update the expected system tests but it seems to be a valid change. By default we do now sort by nb_visits and as a secondary column by label. This was not the case before.

* make sure to select correct column (the column value might be false which is valid, meaning column actually exists whereas we assumed before it does not exist)
* use correct sort algorithm (if value of first column was false we picked under circumstances a string comparison instead of number)
* If we sort by label, use always a string or natural comparison even if the label is numeric
@tsteur tsteur added c: Performance For when we could improve the performance / speed of Matomo. Bug For errors / faults / flaws / inconsistencies etc. labels Mar 17, 2015
@tsteur tsteur added this to the Piwik 2.12.0 milestone Mar 17, 2015
@tsteur tsteur added the Needs Review PRs that need a code review label Mar 17, 2015
mattab pushed a commit that referenced this pull request Mar 17, 2015
Fix Sort filters are sometimes applied multiple times
@mattab mattab merged commit f1894a1 into master Mar 17, 2015
@mnapoli mnapoli deleted the 7388 branch March 23, 2015 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc. c: Performance For when we could improve the performance / speed of Matomo. Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants