Run queued filters after generic filters making visualizations much faster #7387
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tsteur
added
the
c: Performance
| @@ -147,6 +147,8 @@ private function getLabelVariations($originalLabel) | |||
| } | |||
| $variations[] = $label; | |||
|
|
|||
| $variations = array_unique($variations); | |||
There was a problem hiding this comment.
Just a small performance tweak for labelfilter
| throw new PluginDeactivatedException($module); | ||
| } | ||
| $apiClassName = self::getClassNameAPI($module); | ||
| $apiClassName = $this->getClassNameAPI($module); |
There was a problem hiding this comment.
Note: this reverts https://github.com/piwik/piwik/pull/7344/files but that's not wanted
tsteur
force-pushed
the
visualization_filter_fix
branch
2 times, most recently
from
c4f6c61
to
3c3a11b
Compare
| @@ -60,6 +62,16 @@ protected function getGoalMetrics(Row $row) | |||
| $alternateKey = 'idgoal=' . $this->idGoal; | |||
| if (isset($allGoalMetrics[$alternateKey])) { | |||
| return $allGoalMetrics[$alternateKey]; | |||
| } elseif ($this->idGoal === Piwik::LABEL_ID_GOAL_IS_ECOMMERCE_ORDER) { | |||
There was a problem hiding this comment.
do these if statements fix a bug or add a new feature?
There was a problem hiding this comment.
fixes ui tests as sometimes we have a string here and sometimes an integer / metric id. Wasn't needed before but now
|
Looks good! 👍 let's test it on demo |
mattab
pushed a commit
that referenced
this pull request
Mar 9, 2015
Run queued filters after generic filters making visualizations much faster
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When rendering a visualization, we applied queued filters, computed processed metrics, etc. always on all rows (meaning before generic filters were applied). This means when a datatable has 25k rows (which is not so much), then we ran
computeProcessedMetrics,applyQueuedFilter, ... on 25k rows instead of only 100 displayed rows. By changing this it made rendering a report with 25k rows over 3 seconds faster on my server. The time spent shrank to filter rows shrank from > 3 seconds to < 40ms.Also before this refactoring filters were applied in a completely random order which is entirely different to what we test in system tests resulting in different displayed data compared to what we test and compared to the exported data. Apart from that there was some duplicated code in applying filters in the visualization. We do now make sure to run the filters in the same order as we test it in system tests.
Note: Some UI tests are failing but only for some rows having the same value in the
filter_sort_column. So it is not really a change, rather a fix since we do now apply the filters in the correct order.