Merge pull request #415 from kbrock/partial_tree_sort

Make arrange_nodes more memory efficient

Author: kbrock

Diff for: lib/ancestry/class_methods.rb

@@ -46,8 +46,10 @@ def arrange options = {}
       end
     end
 
-    # Arrange array of nodes into a nested hash of the form
-    # {node => children}, where children = {} if the node has no children
+    # arranges array of nodes to a hierarchical hash
+    #
+    # @param nodes [Array[Node]] nodes to be arranged
+    # @returns Hash{Node => {Node => {}, Node => {}}}
     # If a node's parent is not included, the node will be included as if it is a top level node
     def arrange_nodes(nodes)
       node_ids = Set.new(nodes.map(&:id))
@@ -60,6 +62,18 @@ def arrange_nodes(nodes)
       end
     end
 
+    # convert a hash of the form {node => children} to an array of nodes, child first
+    #
+    # @param arranged [Hash{Node => {Node => {}, Node => {}}}] arranged nodes
+    # @returns [Array[Node]] array of nodes with the parent before the children
+    def flatten_arranged_nodes(arranged, nodes = [])
+      arranged.each do |node, children|
+        nodes << node
+        flatten_arranged_nodes(children, nodes) unless children.empty?
+      end
+      nodes
+    end
+
      # Arrangement to nested array for serialization
      # You can also supply your own serialization logic using blocks
      # also allows you to pass the order just as you can pass it to the arrange method
@@ -89,29 +103,21 @@ def tree_view(column, data = nil)
     end
 
     # Pseudo-preordered array of nodes.  Children will always follow parents,
+    # This is deterministic unless the parents are missing *and* a sort block is specified
     def sort_by_ancestry(nodes, &block)
       arranged = nodes if nodes.is_a?(Hash)
 
       unless arranged
         presorted_nodes = nodes.sort do |a, b|
-          a_cestry, b_cestry = a.ancestry || '0', b.ancestry || '0'
-
-          if block_given? && a_cestry == b_cestry
-            yield a, b
-          else
-            a_cestry <=> b_cestry
-          end
+          rank = (a.ancestry || ' ') <=> (b.ancestry || ' ')
+          rank = yield(a, b) if rank == 0 && block_given?
+          rank
         end
 
         arranged = arrange_nodes(presorted_nodes)
       end
 
-      arranged.inject([]) do |sorted_nodes, pair|
-        node, children = pair
-        sorted_nodes << node
-        sorted_nodes += sort_by_ancestry(children, &block) unless children.blank?
-        sorted_nodes
-      end
+      flatten_arranged_nodes(arranged)
     end
 
     # Integrity checking

Diff for: test/concerns/sort_by_ancestry_test.rb

@@ -37,7 +37,7 @@ def test_sort_by_ancestry_full_tree
     AncestryTestDatabase.with_model do |model|
       n1, n2, n3, n4, n5, n6 = build_tree(model)
 
-      records = model.sort_by_ancestry(model.all.order(:id).reverse)
+      records = model.sort_by_ancestry(model.all.ordered_by_ancestry_and(:id => :desc))
       assert_equal [n1, n5, n6, n2, n4, n3].map(&:id), records.map(&:id)
     end
   end
@@ -51,7 +51,8 @@ def test_sort_by_ancestry_no_parents_siblings
     AncestryTestDatabase.with_model do |model|
       _, _, n3, n4, _, _ = build_tree(model)
 
-      assert_equal [n4, n3].map(&:id), model.sort_by_ancestry([n4, n3]).map(&:id)
+      records = model.sort_by_ancestry(model.all.ordered_by_ancestry_and(:id => :desc).offset(3).take(2))
+      assert_equal [n4, n3].map(&:id), records.map(&:id)
     end
   end
 
@@ -61,15 +62,18 @@ def test_sort_by_ancestry_no_parents_same_level
     AncestryTestDatabase.with_model do |model|
       _, _, n3, n4, n5, _ = build_tree(model)
 
-      assert_equal [n5, n4, n3].map(&:id), model.sort_by_ancestry([n5, n4, n3]).map(&:id)
+      records = [n5, n4, n3]
+      # records = model.sort_by_ancestry(model.all.ordered_by_ancestry_and(:id => :desc).offset(3).take(3))
+      assert_equal [n5, n4, n3].map(&:id), records.map(&:id)
     end
   end
 
   def test_sort_by_ancestry_partial_tree
     AncestryTestDatabase.with_model do |model|
       n1, n2, _, _, n5, _ = build_tree(model)
 
-      assert_equal [n1, n5, n2].map(&:id), model.sort_by_ancestry([n5, n2, n1]).map(&:id)
+      records = model.sort_by_ancestry(model.all.ordered_by_ancestry_and(:id => :desc).offset(0).take(3))
+      assert_equal [n1, n5, n2].map(&:id), records.map(&:id)
     end
   end
 
@@ -142,7 +146,7 @@ def test_sort_by_ancestry_with_block_full_tree_sql_sort
     AncestryTestDatabase.with_model :extra_columns => {:rank => :integer} do |model|
       n1, n2, n3, n4, n5, n6 = build_ranked_tree(model)
 
-      records = model.sort_by_ancestry(model.all.order(:rank))
+      records = model.sort_by_ancestry(model.all.ordered_by_ancestry_and(:rank))
       assert_equal [n1, n5, n3, n2, n4, n6].map(&:id), records.map(&:id)
     end
   end
@@ -151,7 +155,8 @@ def test_sort_by_ancestry_with_block_all_parents_some_children
     AncestryTestDatabase.with_model :extra_columns => {:rank => :integer} do |model|
       n1, n2, _, _, n5, _ = build_ranked_tree(model)
 
-      assert_equal [n1, n5, n2].map(&:id), model.sort_by_ancestry([n1, n2, n5], &RANK_SORT).map(&:id)
+      records = model.sort_by_ancestry(model.all.ordered_by_ancestry_and(:rank).take(3), &RANK_SORT)
+      assert_equal [n1, n5, n2].map(&:id), records.map(&:id)
     end
   end
 
@@ -176,7 +181,7 @@ def test_sort_by_ancestry_with_block_no_parents_all_children
     AncestryTestDatabase.with_model :extra_columns => {:rank => :integer} do |model|
       _, _, n3, n4, n5, n6 = build_ranked_tree(model)
 
-      records = model.sort_by_ancestry([n3, n4, n5, n6], &RANK_SORT)
+      records = model.sort_by_ancestry(model.all.ordered_by_ancestry_and(:rank).offset(2), &RANK_SORT)
       if CORRECT || records[0] == n5
         assert_equal [n5, n3, n4, n6].map(&:id), records.map(&:id)
       else
@@ -191,20 +196,20 @@ def test_sort_by_ancestry_with_block_no_parents_all_children
   #   - n2 (1)
   #     - n4 (0)
   #     - x
-  # Issue: n2 will always go before n4.
+  # Issue: n2 will always go before n4, n5.
   #        But n1 is not available to put n3 before the n2 tree.
   #        not sure why it doesn't follow the input order
   #
   # NOTE: even for partial trees, if the input records are ranked, the output works
   def test_sort_by_ancestry_with_sql_sort_paginated_missing_parents_and_children
     AncestryTestDatabase.with_model :extra_columns => {:rank => :integer} do |model|
-      _, n2, n3, n4, _, _ = build_ranked_tree(model)
+      _, n2, n3, n4, n5, _ = build_ranked_tree(model)
 
-      records = model.sort_by_ancestry([n2, n4, n3])
+      records = model.sort_by_ancestry(model.all.ordered_by_ancestry_and(:rank).offset(1).take(4))
       if CORRECT
-        assert_equal [n3, n2, n4].map(&:id), records.map(&:id)
+        assert_equal [n3, n2, n4, n5].map(&:id), records.map(&:id)
       else
-        assert_equal [n2, n4, n3].map(&:id), records.map(&:id)
+        assert_equal [n2, n4, n5, n3].map(&:id), records.map(&:id)
       end
     end
   end
@@ -218,13 +223,13 @@ def test_sort_by_ancestry_with_sql_sort_paginated_missing_parents_and_children
   #     - n5
   def test_sort_by_ancestry_with_block_paginated_missing_parents_and_children
     AncestryTestDatabase.with_model :extra_columns => {:rank => :integer} do |model|
-      _, n2, n3, n4, _, _ = build_ranked_tree(model)
+      _, n2, n3, n4, n5, _ = build_ranked_tree(model)
 
-      records = model.sort_by_ancestry([n2, n4, n3], &RANK_SORT)
+      records = model.sort_by_ancestry(model.all.ordered_by_ancestry_and(:rank).offset(1).take(4), &RANK_SORT)
       if CORRECT
-        assert_equal [n3, n2, n4].map(&:id), records.map(&:id)
+        assert_equal [n3, n2, n4, n5].map(&:id), records.map(&:id)
       else
-        assert_equal [n2, n4, n3].map(&:id), records.map(&:id)
+        assert_equal [n2, n4, n5, n3].map(&:id), records.map(&:id)
       end
     end
   end