Rewrite aggregation processing to be more efficient

app/models/module_result.rb

@@ -31,13 +31,33 @@ def to_json(options={})
   end
 
   def pre_order
-    root = self
-    root = root.parent until root.parent.nil?
-    @pre_order ||= pre_order_traverse(root).to_a
+    return @pre_order unless @pre_order.nil?
+    return processing.root_module_result.pre_order unless parent.nil?
+
+    @pre_order = pre_order_traverse(self).to_a
+  end
+
+  def level_order
+    return @level_order unless @level_order.nil?
+    return processing.root_module_result.level_order unless parent.nil?
+
+    @level_order = descendants_by_level.flatten
   end
 
   def descendants
-    @descendants ||= pre_order_traverse(self).to_a
+    @descendants ||= descendants_by_level.flatten
+  end
+
+  def descendants_by_level
+    level = [self]
+    descendants = []
+
+    until level.empty? do
+      descendants << level
+      level = fetch_all_children(level)
+    end
+
+    descendants
   end
 
   def descendant_hotspot_metric_results
@@ -66,4 +86,13 @@ def pre_order_traverse(module_result, &block)
       end
     end
   end
+
+  def fetch_all_children(parents)
+    self.class.
+      where(parent_id: parents.map(&:id)).
+      eager_load(:kalibro_module, :tree_metric_results).
+      to_a
+  end
+
+
 end

app/models/tree_metric_result.rb

@@ -14,10 +14,10 @@ def has_grade?
   def grade; self.range.grade; end
 
   def descendant_values
-    module_result.children.map { |child|
-      metric_result = child.tree_metric_result_for(self.metric)
-      metric_result.value if metric_result
-    }.compact
+    self.class.
+      joins(:module_result).
+      where('module_results.parent_id' => module_result_id).
+      pluck(:value)
   end
 
   def as_json(options={})

lib/processor/aggregator.rb

@@ -1,54 +1,74 @@
-require 'metric_result_aggregator'
-
 module Processor
   class Aggregator < ProcessingStep
-    protected
+    def self.task(context)
+      # Only bother processing Tree metric configurations
+      # Do nothing if there are none
+      metric_configurations = extract_tree_metric_configurations(context)
+      return if metric_configurations.empty?
 
-    def self.task(runner)
-      @native_metrics = runner.native_metrics
-      set_all_metrics
-      aggregate(runner.processing.root_module_result.pre_order)
-    end
+      # Find all the descendants of the root at once.
+      root = context.processing.root_module_result
+      descendants_by_level = root.descendants_by_level
+      # Keep a hash to allow looking up parents by id without going to the DB again
+      module_results_by_id = module_results_by_id(descendants_by_level)
 
-    def self.state
-      "AGGREGATING"
-    end
+      new_tree_metric_results = []
 
-    private
+      # Process levels bottom-up to ensure every descendants has been aggregated already when a module result is
+      # processed
+      descendants_by_level.reverse_each do |level|
+        # Group module results in each level by their parents and aggregate the results for the group, to then be
+        # written to the parent
+        level.group_by(&:parent_id).each do |parent_id, children|
+          next if parent_id.nil?
+          parent = module_results_by_id[parent_id]
 
-    def self.aggregate(pre_order_module_results)
-      # The upper nodes of the tree need the children to be calculated first, so we reverse the pre_order
-      pre_order_module_results.reverse_each do | module_result_child |
+          metric_configurations.each do |mc|
+            # Ensure we won't try to aggregate into something of an unexpected granularity
+            next if parent.kalibro_module.granularity < mc.metric.scope
 
-        already_calculated_metrics = module_result_child.tree_metric_results.map { |tree_metric_result| tree_metric_result.metric}
+            # Don't create a result if the parent already has a result for this metric
+            next if parent.tree_metric_results.any? { |tmr| tmr.metric_configuration_id == mc.id }
 
-        remaining_metrics = @all_metrics.reject { |metric| already_calculated_metrics.include?(metric) }
+            # Get all the metric results for this metric configuration from all the module results in the group
+            tree_metric_results = children.map { |module_result|
+              module_result.tree_metric_results.find { |tmr| tmr.metric_configuration_id == mc.id }
+            }.compact
 
-        remaining_metrics.each do |metric|
-          if module_result_child.kalibro_module.granularity >= KalibroClient::Entities::Miscellaneous::Granularity.new(metric.scope.to_s.to_sym)
-            tree_metric_result = TreeMetricResult.new(metric: metric, module_result: module_result_child, metric_configuration_id: metric_configuration(metric).id)
-            tree_metric_result.value = MetricResultAggregator.aggregated_value(tree_metric_result)
-            tree_metric_result.save
+            # Aggregate the results and save
+            new_value = aggregate_values(tree_metric_results, mc)
+            new_tree_metric_results << parent.tree_metric_results.build(metric_configuration_id: mc.id, value: new_value)
           end
         end
       end
+
+      TreeMetricResult.import!(new_tree_metric_results)
     end
 
-    def self.set_all_metrics
-      @all_metrics = []
-      @native_metrics.each_value do |metric_configurations|
-        metric_configurations.each do |metric_configuration|
-          @all_metrics << metric_configuration.metric
-        end
-      end
+    def self.state
+      "AGGREGATING"
+    end
+
+    def self.extract_tree_metric_configurations(context)
+      context.native_metrics.values.flatten.reject { |mc| mc.metric.type != 'NativeMetricSnapshot' }
     end
 
-    def self.metric_configuration(metric)
-      @native_metrics.each_value do |metric_configurations|
-        metric_configurations.each do |metric_configuration|
-          return metric_configuration if metric_configuration.metric == metric
+    def self.module_results_by_id(descendants_by_level)
+      results = {}
+      descendants_by_level.each do |level|
+        level.each do |module_result|
+          results[module_result.id] = module_result
         end
       end
+
+      results
+    end
+
+    def self.aggregate_values(tree_metric_results, metric_configuration)
+      aggregation_form = metric_configuration.aggregation_form.to_s.downcase
+      DescriptiveStatistics::Stats.new(tree_metric_results.map(&:value)).send(aggregation_form)
     end
+
+    private_class_method :extract_tree_metric_configurations, :module_results_by_id, :aggregate_values
   end
 end

spec/controllers/tree_metric_results_controller_spec.rb

@@ -9,10 +9,8 @@
       let!(:module_result) { FactoryGirl.build(:module_result) }
 
       before :each do
-        module_result.expects(:tree_metric_result_for).with(metric_result.metric).returns(metric_result)
-        module_result.expects(:children).returns([module_result])
-        metric_result.expects(:module_result).returns(module_result)
-        TreeMetricResult.expects(:find).with(metric_result.id).returns(metric_result)
+        MetricResult.expects(:find).with(metric_result.id).returns(metric_result)
+        metric_result.expects(:descendant_values).returns([metric_result.value])
       end
 
       context 'json format' do