Improvements

* Move sort implementation to each specific file
* Improve comments
* Clean up sorting code for json indexes

6 files changed, 100 insertions, 58 deletions

diff --git a/src/mango/src/mango_idx.erl b/src/mango/src/mango_idx.erl
index b5590a4ba..7c37585c7 100644
--- a/src/mango/src/mango_idx.erl
+++ b/src/mango/src/mango_idx.erl
@@ -105,18 +105,8 @@ maybe_filter_by_sort_fields(Indexes, _Selector, []) ->
 
 maybe_filter_by_sort_fields(Indexes, Selector, SortFields) ->
     FilterFun = fun(Idx) ->
-        Cols = mango_idx:columns(Idx),
-        case {mango_idx:type(Idx), Cols} of
-            {_, all_fields} ->
-                true;
-            {<<"text">>, _} ->
-                sets:is_subset(sets:from_list(SortFields), sets:from_list(Cols));
-            {<<"json">>, _} ->
-                NormalizedSortFields = normalize_sort_fields(Cols, SortFields, Selector),
-                lists:prefix(NormalizedSortFields, Cols);
-            {<<"special">>, _} ->
-                lists:prefix(SortFields, Cols)
-        end
+        Mod = idx_mod(Idx),
+        Mod:maybe_filter_by_sort_fields(Idx, SortFields, Selector)
     end,
     case lists:filter(FilterFun, Indexes) of
         [] ->
@@ -126,32 +116,6 @@ maybe_filter_by_sort_fields(Indexes, Selector, SortFields) ->
     end.
 
 
-% This is a user experience improvement. If a selector has a sort field set,
-% an index is only valid if all the fields in the index are also specified
-% in the sort.
-
-% If a field is in the selector is constant, eg {age: {$eq: 21}} and it's
-% part of the index then we can automatically add it to the sort list because 
-% it won't affect sorting.
-
-% This will then increase the likely hood of the index being choosen 
-% and make it a little easier for the user.
-normalize_sort_fields(Cols, SortFields, Selector) ->
-    % Keep any fields in the sort that might not be defined in the index
-    Start = lists:subtract(SortFields, Cols), 
-    lists:foldl(fun (Col, Sort) ->
-        case lists:member(Col, SortFields) of
-            true -> 
-                [Col | Sort];
-            _ -> 
-                case mango_selector:is_constant_field(Selector, Col) of
-                    true -> [Col | Sort];
-                    _ -> Sort
-                end
-        end
-    end, Start, lists:reverse(Cols)).
-
-
 new(Db, Opts) ->
     Def = get_idx_def(Opts),
     Type = get_idx_type(Opts),
diff --git a/src/mango/src/mango_idx_special.erl b/src/mango/src/mango_idx_special.erl
index 12da1cbe5..a2869ad8e 100644
--- a/src/mango/src/mango_idx_special.erl
+++ b/src/mango/src/mango_idx_special.erl
@@ -22,7 +22,9 @@
     columns/1,
     is_usable/3,
     start_key/1,
-    end_key/1
+    end_key/1,
+
+    maybe_filter_by_sort_fields/3
 ]).
 
 
@@ -96,3 +98,8 @@ end_key([{_, _, '$lte', Key}]) ->
 end_key([{'$eq', Key, '$eq', Key}]) ->
     false = mango_json:special(Key),
     Key.
+
+
+maybe_filter_by_sort_fields(Idx, SortFields, _Selector) ->
+    Cols = mango_idx:columns(Idx),
+    lists:prefix(SortFields, Cols).
diff --git a/src/mango/src/mango_idx_text.erl b/src/mango/src/mango_idx_text.erl
index 29b4441a1..6e4f0350b 100644
--- a/src/mango/src/mango_idx_text.erl
+++ b/src/mango/src/mango_idx_text.erl
@@ -23,7 +23,8 @@
     to_json/1,
     columns/1,
     is_usable/3,
-    get_default_field_options/1
+    get_default_field_options/1,
+    maybe_filter_by_sort_fields/3
 ]).
 
 
@@ -376,6 +377,16 @@ forbid_index_all() ->
     config:get("mango", "index_all_disabled", "false").
 
 
+maybe_filter_by_sort_fields(Idx, SortFields, _Selector) ->
+    Cols = mango_idx:columns(Idx),
+    case Cols of
+        all_fields -> 
+            true;
+        _ ->
+            sets:is_subset(sets:from_list(SortFields), sets:from_list(Cols))
+    end.
+
+
 -ifdef(TEST).
 -include_lib("eunit/include/eunit.hrl").
 
diff --git a/src/mango/src/mango_idx_view.erl b/src/mango/src/mango_idx_view.erl
index 8956b27b0..fe4549e34 100644
--- a/src/mango/src/mango_idx_view.erl
+++ b/src/mango/src/mango_idx_view.erl
@@ -24,6 +24,7 @@
     columns/1,
     start_key/1,
     end_key/1,
+    maybe_filter_by_sort_fields/3,
 
     indexable_fields/1,
     field_ranges/1,
@@ -511,3 +512,44 @@ range_pos(Low, Arg, High) ->
                     max
             end
     end.
+
+
+maybe_filter_by_sort_fields(Idx, SortFields, Selector) ->
+    Cols = mango_idx:columns(Idx),
+    case lists:subtract(SortFields, Cols) of
+        [] -> 
+            NormalizedSortFields = normalize_sort_fields(Cols, SortFields, Selector),
+            lists:prefix(NormalizedSortFields, Cols);
+        _ ->
+            false
+    end.
+
+
+% This is an user experience improvement. If a selector has a sort field set
+% then an index is only valid if the prefix of the sort fields match the 
+% prefix of the index fields. 
+
+% e.g Index = [A, B, C] with Sort = [A, B] is a valid sort
+% but if Sort = [B, C] then it is not valid for this index.
+
+% If an indexed field in the selector is constant, eg {A: {$eq: 21}} then
+% we can add it to the sort list because it won't affect sorting and the
+% original sort will still be valid.
+
+% e.g Index = [A, B] with Sort = [B] and selector has {A: 1}. 
+% Then we can make the Sort = [A, B].
+% The sort will work as expected and this will increase the possibility
+% of the index being choosen. It also helps a user where they might not have
+% put correct initial fields in the sort.
+normalize_sort_fields(Cols, SortFields, Selector) ->
+    lists:foldr(fun (Col, Sort) ->
+        case lists:member(Col, SortFields) of
+            true -> 
+                [Col | Sort];
+            _ -> 
+                case mango_selector:is_constant_field(Selector, Col) of
+                    true -> [Col | Sort];
+                    _ -> Sort
+                end
+        end
+    end, [], Cols).
diff --git a/src/mango/src/mango_selector.erl b/src/mango/src/mango_selector.erl
index b87747389..fffadcd20 100644
--- a/src/mango/src/mango_selector.erl
+++ b/src/mango/src/mango_selector.erl
@@ -640,30 +640,26 @@ has_required_fields_int([{[{Field, Cond}]} | Rest], RequiredFields) ->
 
 
 % Returns true if a field in the selector is a constant value e.g. {a: {$eq: 1}}
-is_constant_field(Selector, Field) ->
-    is_constant_field_int(Selector, Field).
-
-
-is_constant_field_int({[]}, _Field) ->
+is_constant_field({[]}, _Field) ->
     false;
 
-is_constant_field_int(Selector, Field) when not is_list(Selector) ->
-    is_constant_field_int([Selector], Field);
+is_constant_field(Selector, Field) when not is_list(Selector) ->
+    is_constant_field([Selector], Field);
 
-is_constant_field_int([], _Field) ->
+is_constant_field([], _Field) ->
     false;
 
-is_constant_field_int([{[{<<"$and">>, Args}]}], Field) when is_list(Args) ->
-    lists:any(fun(Arg) -> is_constant_field_int(Arg, Field) end, Args);
+is_constant_field([{[{<<"$and">>, Args}]}], Field) when is_list(Args) ->
+    lists:any(fun(Arg) -> is_constant_field(Arg, Field) end, Args);
 
-is_constant_field_int([{[{<<"$and">>, Args}]}], Field) ->
-    is_constant_field_int(Args, Field);
+is_constant_field([{[{<<"$and">>, Args}]}], Field) ->
+    is_constant_field(Args, Field);
 
-is_constant_field_int([{[{SelectorField, {[{Cond, _Val}]}}]} | _Rest], Field) when SelectorField =:= Field ->
+is_constant_field([{[{Field, {[{Cond, _Val}]}}]} | _Rest], Field) ->
     Cond =:= <<"$eq">>;
 
-is_constant_field_int([{[{SelectorField, _}]} | Rest], Field) when SelectorField =/= Field  ->
-    is_constant_field_int(Rest, Field).
+is_constant_field([{[{_UnMatched, _}]} | Rest], Field) ->
+    is_constant_field(Rest, Field).
 
 
 %%%%%%%% module tests below %%%%%%%%
@@ -677,17 +673,32 @@ is_constant_field_basic_test() ->
     ?assertEqual(true, is_constant_field(Selector, Field)).
 
 is_constant_field_basic_two_test() ->
-    Selector = normalize({[{<<"$and">>, [{[{<<"cars">>,{[{<<"$eq">>,<<"2">>}]}}]}, {[{<<"age">>,{[{<<"$gt">>,10}]}}]}]}]}),
+    Selector = normalize({[{<<"$and">>,
+        [
+            {[{<<"cars">>,{[{<<"$eq">>,<<"2">>}]}}]},
+            {[{<<"age">>,{[{<<"$gt">>,10}]}}]}
+        ]
+    }]}),
     Field = <<"cars">>,
     ?assertEqual(true, is_constant_field(Selector, Field)).
 
 is_constant_field_not_eq_test() ->
-    Selector = normalize({[{<<"$and">>, [{[{<<"cars">>,{[{<<"$eq">>,<<"2">>}]}}]}, {[{<<"age">>,{[{<<"$gt">>,10}]}}]}]}]}),
+    Selector = normalize({[{<<"$and">>,
+        [
+            {[{<<"cars">>,{[{<<"$eq">>,<<"2">>}]}}]},
+            {[{<<"age">>,{[{<<"$gt">>,10}]}}]}
+        ]
+    }]}),
     Field = <<"age">>,
     ?assertEqual(false, is_constant_field(Selector, Field)).
 
 is_constant_field_missing_field_test() ->
-    Selector = normalize({[{<<"$and">>, [{[{<<"cars">>,{[{<<"$eq">>,<<"2">>}]}}]}, {[{<<"age">>,{[{<<"$gt">>,10}]}}]}]}]}),
+    Selector = normalize({[{<<"$and">>,
+        [
+            {[{<<"cars">>,{[{<<"$eq">>,<<"2">>}]}}]},
+            {[{<<"age">>,{[{<<"$gt">>,10}]}}]}
+        ]
+    }]}),
     Field = <<"wrong">>,
     ?assertEqual(false, is_constant_field(Selector, Field)).
 
diff --git a/src/mango/test/02-basic-find-test.py b/src/mango/test/02-basic-find-test.py
index f7e151ad8..6a31d33ee 100644
--- a/src/mango/test/02-basic-find-test.py
+++ b/src/mango/test/02-basic-find-test.py
@@ -333,3 +333,10 @@ class BasicFindTests(mango.UserDocsTests):
         assert explain["mrargs"]["start_key"] == [0]
         assert explain["mrargs"]["end_key"] == ["<MAX>"]
         assert explain["mrargs"]["include_docs"] == True
+
+    def test_sort_with_all_docs(self):
+        explain = self.db.find({
+            "_id": {"$gt": 0},
+            "age": {"$gt": 0}
+        }, sort=["_id"], explain=True)
+        self.assertEquals(explain["index"]["type"], "special")