Skip to content

Commit

Permalink
Revert "Fix incorrect index selection when sort specified (apache#866)"
Browse files Browse the repository at this point in the history
This reverts commit d0445f5.
  • Loading branch information
iilyak committed Nov 13, 2017
1 parent 01658a0 commit dc89343
Show file tree
Hide file tree
Showing 8 changed files with 35 additions and 93 deletions.
2 changes: 1 addition & 1 deletion src/mango/src/mango_cursor.erl
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@

create(Db, Selector0, Opts) ->
Selector = mango_selector:normalize(Selector0),
UsableIndexes = mango_idx:get_usable_indexes(Db, Selector, Opts),
UsableIndexes = mango_idx:get_usable_indexes(Db, Selector0, Opts),

{use_index, IndexSpecified} = proplists:lookup(use_index, Opts),
case {length(UsableIndexes), length(IndexSpecified)} of
Expand Down
2 changes: 1 addition & 1 deletion src/mango/src/mango_error.erl
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ info(mango_cursor, {no_usable_index, selector_unsupported}) ->
{
400,
<<"no_usable_index">>,
<<"The index specified with \"use_index\" is not usable for the query.">>
<<"There is no index available for this selector.">>
};

info(mango_json_bookmark, {invalid_bookmark, BadBookmark}) ->
Expand Down
55 changes: 24 additions & 31 deletions src/mango/src/mango_idx.erl
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
-export([
list/1,
recover/1,
for_sort/2,

new/2,
validate_new/2,
Expand All @@ -35,7 +36,7 @@
def/1,
opts/1,
columns/1,
is_usable/3,
is_usable/2,
start_key/2,
end_key/2,
cursor_mod/1,
Expand All @@ -56,8 +57,9 @@ list(Db) ->
{ok, Indexes} = ddoc_cache:open(db_to_name(Db), ?MODULE),
Indexes.

get_usable_indexes(Db, Selector0, Opts) ->
Selector = mango_selector:normalize(Selector0),

get_usable_indexes(Db, Selector, Opts) ->
ExistingIndexes = mango_idx:list(Db),
if ExistingIndexes /= [] -> ok; true ->
?MANGO_ERROR({no_usable_index, no_indexes_defined})
Expand All @@ -68,17 +70,13 @@ get_usable_indexes(Db, Selector, Opts) ->
?MANGO_ERROR({no_usable_index, no_index_matching_name})
end,

SortFields = get_sort_fields(Opts),
UsableFilter = fun(I) -> is_usable(I, Selector, SortFields) end,
UsableIndexes0 = lists:filter(UsableFilter, FilteredIndexes),

case maybe_filter_by_sort_fields(UsableIndexes0, SortFields) of
{ok, SortIndexes} ->
SortIndexes;
{error, no_usable_index} ->
?MANGO_ERROR({no_usable_index, missing_sort_index})
end.
SortIndexes = mango_idx:for_sort(FilteredIndexes, Opts),
if SortIndexes /= [] -> ok; true ->
?MANGO_ERROR({no_usable_index, missing_sort_index})
end,

UsableFilter = fun(I) -> mango_idx:is_usable(I, Selector) end,
lists:filter(UsableFilter, SortIndexes).

recover(Db) ->
{ok, DDocs0} = mango_util:open_ddocs(Db),
Expand All @@ -95,38 +93,33 @@ recover(Db) ->
end, DDocs)}.


get_sort_fields(Opts) ->
for_sort(Indexes, Opts) ->
% If a sort was specified we have to find an index that
% can satisfy the request.
case lists:keyfind(sort, 1, Opts) of
{sort, Sort} ->
mango_sort:fields(Sort);
{sort, {SProps}} when is_list(SProps) ->
for_sort_int(Indexes, {SProps});
_ ->
[]
Indexes
end.


maybe_filter_by_sort_fields(Indexes, []) ->
{ok, Indexes};

maybe_filter_by_sort_fields(Indexes, SortFields) ->
for_sort_int(Indexes, Sort) ->
Fields = mango_sort:fields(Sort),
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));
sets:is_subset(sets:from_list(Fields), sets:from_list(Cols));
{<<"json">>, _} ->
lists:prefix(SortFields, Cols);
lists:prefix(Fields, Cols);
{<<"special">>, _} ->
lists:prefix(SortFields, Cols)
lists:prefix(Fields, Cols)
end
end,
case lists:filter(FilterFun, Indexes) of
[] ->
{error, no_usable_index};
FilteredIndexes ->
{ok, FilteredIndexes}
end.
lists:filter(FilterFun, Indexes).


new(Db, Opts) ->
Expand Down Expand Up @@ -257,9 +250,9 @@ columns(#idx{}=Idx) ->
Mod:columns(Idx).


is_usable(#idx{}=Idx, Selector, SortFields) ->
is_usable(#idx{}=Idx, Selector) ->
Mod = idx_mod(Idx),
Mod:is_usable(Idx, Selector, SortFields).
Mod:is_usable(Idx, Selector).


start_key(#idx{}=Idx, Ranges) ->
Expand Down
4 changes: 2 additions & 2 deletions src/mango/src/mango_idx_special.erl
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
from_ddoc/1,
to_json/1,
columns/1,
is_usable/3,
is_usable/2,
start_key/1,
end_key/1
]).
Expand Down Expand Up @@ -63,7 +63,7 @@ columns(#idx{def=all_docs}) ->
[<<"_id">>].


is_usable(#idx{def=all_docs}, Selector, _) ->
is_usable(#idx{def=all_docs}, Selector) ->
Fields = mango_idx_view:indexable_fields(Selector),
lists:member(<<"_id">>, Fields).

Expand Down
4 changes: 2 additions & 2 deletions src/mango/src/mango_idx_text.erl
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
from_ddoc/1,
to_json/1,
columns/1,
is_usable/3,
is_usable/2,
get_default_field_options/1
]).

Expand Down Expand Up @@ -125,7 +125,7 @@ columns(Idx) ->
end.


is_usable(Idx, Selector, _) ->
is_usable(Idx, Selector) ->
case columns(Idx) of
all_fields ->
true;
Expand Down
13 changes: 4 additions & 9 deletions src/mango/src/mango_idx_view.erl
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
remove/2,
from_ddoc/1,
to_json/1,
is_usable/3,
is_usable/2,
columns/1,
start_key/1,
end_key/1,
Expand Down Expand Up @@ -114,17 +114,12 @@ columns(Idx) ->
[Key || {Key, _} <- Fields].


is_usable(Idx, Selector, SortFields) ->
% This index is usable if all of the columns are
is_usable(Idx, Selector) ->
% This index is usable if all of the columns are
% restricted by the selector such that they are required to exist
% and the selector is not a text search (so requires a text index)
RequiredFields = columns(Idx),

% sort fields are required to exist in the results so
% we don't need to check the selector for these
RequiredFields1 = ordsets:subtract(lists:usort(RequiredFields), lists:usort(SortFields)),

mango_selector:has_required_fields(Selector, RequiredFields1)
mango_selector:has_required_fields(Selector, RequiredFields)
andalso not is_text_search(Selector).


Expand Down
2 changes: 1 addition & 1 deletion src/mango/src/mango_selector.erl
Original file line number Diff line number Diff line change
Expand Up @@ -609,7 +609,7 @@ has_required_fields([{[{Field, Cond}]} | Rest], RequiredFields) ->
_ ->
has_required_fields(Rest, lists:delete(Field, RequiredFields))
end.


%%%%%%%% module tests below %%%%%%%%

Expand Down
46 changes: 0 additions & 46 deletions src/mango/test/02-basic-find-test.py
Original file line number Diff line number Diff line change
Expand Up @@ -222,52 +222,6 @@ def test_sort(self):
docs2 = list(reversed(sorted(docs1, key=lambda d: d["age"])))
assert docs1 is not docs2 and docs1 == docs2

def test_sort_desc_complex(self):
docs = self.db.find({
"company": {"$lt": "M"},
"$or": [
{"company": "Dreamia"},
{"manager": True}
]
}, sort=[{"company":"desc"}, {"manager":"desc"}])

companies_returned = list(d["company"] for d in docs)
desc_companies = sorted(companies_returned, reverse=True)
self.assertEqual(desc_companies, companies_returned)

def test_sort_with_primary_sort_not_in_selector(self):
try:
docs = self.db.find({
"name.last": {"$lt": "M"}
}, sort=[{"name.first":"desc"}])
except Exception as e:
self.assertEqual(e.response.status_code, 400)
resp = e.response.json()
self.assertEqual(resp["error"], "no_usable_index")
else:
raise AssertionError("expected find error")

def test_sort_exists_true(self):
docs1 = self.db.find({"age": {"$gt": 0, "$exists": True}}, sort=[{"age":"asc"}])
docs2 = list(sorted(docs1, key=lambda d: d["age"]))
assert docs1 is not docs2 and docs1 == docs2

def test_sort_desc_complex_error(self):
try:
self.db.find({
"company": {"$lt": "M"},
"$or": [
{"company": "Dreamia"},
{"manager": True}
]
}, sort=[{"company":"desc"}])
except Exception as e:
self.assertEqual(e.response.status_code, 400)
resp = e.response.json()
self.assertEqual(resp["error"], "no_usable_index")
else:
raise AssertionError("expected find error")

def test_fields(self):
selector = {"age": {"$gt": 0}}
docs = self.db.find(selector, fields=["user_id", "location.address"])
Expand Down

0 comments on commit dc89343

Please sign in to comment.