Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mango: generate a warning instead of an error when an user-specified index cannot be used #962

Merged
merged 4 commits into from
Nov 30, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 59 additions & 17 deletions src/mango/src/mango_cursor.erl
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
explain/1,
execute/3,
maybe_filter_indexes_by_ddoc/2,
remove_indexes_with_partial_filter_selector/1,
maybe_add_warning/3
]).

Expand Down Expand Up @@ -47,16 +48,18 @@
create(Db, Selector0, Opts) ->
Selector = mango_selector:normalize(Selector0),
UsableIndexes = mango_idx:get_usable_indexes(Db, Selector, Opts),

{use_index, IndexSpecified} = proplists:lookup(use_index, Opts),
case {length(UsableIndexes), length(IndexSpecified)} of
{0, 0} ->
case length(UsableIndexes) of
0 ->
AllDocs = mango_idx:special(Db),
create_cursor(Db, AllDocs, Selector, Opts);
{0, _} ->
?MANGO_ERROR({no_usable_index, selector_unsupported});
_ ->
create_cursor(Db, UsableIndexes, Selector, Opts)
case mango_cursor:maybe_filter_indexes_by_ddoc(UsableIndexes, Opts) of
[] ->
% use_index doesn't match a valid index - fall back to a valid one
create_cursor(Db, UsableIndexes, Selector, Opts);
UserSpecifiedIndex ->
create_cursor(Db, UserSpecifiedIndex, Selector, Opts)
end
end.


Expand Down Expand Up @@ -90,9 +93,7 @@ execute(#cursor{index=Idx}=Cursor, UserFun, UserAcc) ->
maybe_filter_indexes_by_ddoc(Indexes, Opts) ->
case lists:keyfind(use_index, 1, Opts) of
{use_index, []} ->
% We remove any indexes that have a selector
% since they are only used when specified via use_index
remove_indexes_with_partial_filter_selector(Indexes);
[];
{use_index, [DesignId]} ->
filter_indexes(Indexes, DesignId);
{use_index, [DesignId, ViewName]} ->
Expand Down Expand Up @@ -150,12 +151,53 @@ group_indexes_by_type(Indexes) ->
end, ?CURSOR_MODULES).


maybe_add_warning(UserFun, #idx{type = IndexType}, UserAcc) ->
case IndexType of
maybe_add_warning(UserFun, #cursor{index = Index, opts = Opts}, UserAcc) ->
NoIndexWarning = case Index#idx.type of
<<"special">> ->
Arg = {add_key, warning, <<"no matching index found, create an index to optimize query time">>},
{_Go, UserAcc0} = UserFun(Arg, UserAcc),
UserAcc0;
<<"no matching index found, create an index to optimize query time">>;
_ ->
UserAcc
end.
ok
end,

UseIndexInvalidWarning = case lists:keyfind(use_index, 1, Opts) of
{use_index, []} ->
NoIndexWarning;
{use_index, [DesignId]} ->
case filter_indexes([Index], DesignId) of
[] ->
fmt("_design/~s was not used because it does not contain a valid index for this query.",
[ddoc_name(DesignId)]);
_ ->
NoIndexWarning
end;
{use_index, [DesignId, ViewName]} ->
case filter_indexes([Index], DesignId, ViewName) of
[] ->
fmt("_design/~s, ~s was not used because it is not a valid index for this query.",
[ddoc_name(DesignId), ViewName]);
_ ->
NoIndexWarning
end
end,

maybe_add_warning_int(UseIndexInvalidWarning, UserFun, UserAcc).


maybe_add_warning_int(ok, _, UserAcc) ->
UserAcc;

maybe_add_warning_int(Warning, UserFun, UserAcc) ->
Arg = {add_key, warning, Warning},
{_Go, UserAcc0} = UserFun(Arg, UserAcc),
UserAcc0.


fmt(Format, Args) ->
iolist_to_binary(io_lib:format(Format, Args)).


ddoc_name(<<"_design/", Name/binary>>) ->
Name;

ddoc_name(Name) ->
Name.
2 changes: 1 addition & 1 deletion src/mango/src/mango_cursor_text.erl
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ execute(Cursor, UserFun, UserAcc) ->
Arg = {add_key, bookmark, JsonBM},
{_Go, FinalUserAcc} = UserFun(Arg, LastUserAcc),
FinalUserAcc0 = mango_execution_stats:maybe_add_stats(Opts, UserFun, Stats0, FinalUserAcc),
FinalUserAcc1 = mango_cursor:maybe_add_warning(UserFun, Idx, FinalUserAcc0),
FinalUserAcc1 = mango_cursor:maybe_add_warning(UserFun, Cursor, FinalUserAcc0),
{ok, FinalUserAcc1}
end.

Expand Down
2 changes: 1 addition & 1 deletion src/mango/src/mango_cursor_view.erl
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ execute(#cursor{db = Db, index = Idx, execution_stats = Stats} = Cursor0, UserFu
{_Go, FinalUserAcc} = UserFun(Arg, LastCursor#cursor.user_acc),
Stats0 = LastCursor#cursor.execution_stats,
FinalUserAcc0 = mango_execution_stats:maybe_add_stats(Opts, UserFun, Stats0, FinalUserAcc),
FinalUserAcc1 = mango_cursor:maybe_add_warning(UserFun, Idx, FinalUserAcc0),
FinalUserAcc1 = mango_cursor:maybe_add_warning(UserFun, Cursor, FinalUserAcc0),
{ok, FinalUserAcc1};
{error, Reason} ->
{error, Reason}
Expand Down
19 changes: 0 additions & 19 deletions src/mango/src/mango_error.erl
Original file line number Diff line number Diff line change
Expand Up @@ -21,31 +21,12 @@
]).


info(mango_idx, {no_usable_index, no_indexes_defined}) ->
{
400,
<<"no_usable_index">>,
<<"There are no indexes defined in this database.">>
};
info(mango_idx, {no_usable_index, no_index_matching_name}) ->
{
400,
<<"no_usable_index">>,
<<"No index matches the index specified with \"use_index\"">>
};
info(mango_idx, {no_usable_index, missing_sort_index}) ->
{
400,
<<"no_usable_index">>,
<<"No index exists for this sort, try indexing by the sort fields.">>
};
info(mango_cursor, {no_usable_index, selector_unsupported}) ->
{
400,
<<"no_usable_index">>,
<<"The index specified with \"use_index\" is not usable for the query.">>
};

info(mango_json_bookmark, {invalid_bookmark, BadBookmark}) ->
{
400,
Expand Down
14 changes: 5 additions & 9 deletions src/mango/src/mango_idx.erl
Original file line number Diff line number Diff line change
Expand Up @@ -59,20 +59,16 @@ list(Db) ->

get_usable_indexes(Db, Selector, Opts) ->
ExistingIndexes = mango_idx:list(Db),
if ExistingIndexes /= [] -> ok; true ->
?MANGO_ERROR({no_usable_index, no_indexes_defined})
end,

FilteredIndexes = mango_cursor:maybe_filter_indexes_by_ddoc(ExistingIndexes, Opts),
if FilteredIndexes /= [] -> ok; true ->
?MANGO_ERROR({no_usable_index, no_index_matching_name})
end,
GlobalIndexes = mango_cursor:remove_indexes_with_partial_filter_selector(ExistingIndexes),
UserSpecifiedIndex = mango_cursor:maybe_filter_indexes_by_ddoc(ExistingIndexes, Opts),
UsableIndexes0 = lists:usort(GlobalIndexes ++ UserSpecifiedIndex),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we sorting the indexes here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is to remove duplicates - an index might belong to GlobalIndexes and be specified by the user explicitly.


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

case maybe_filter_by_sort_fields(UsableIndexes0, SortFields) of
case maybe_filter_by_sort_fields(UsableIndexes1, SortFields) of
{ok, SortIndexes} ->
SortIndexes;
{error, no_usable_index} ->
Expand Down
80 changes: 46 additions & 34 deletions src/mango/test/05-index-selection-test.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,8 @@ def test_no_valid_sort_index(self):
def test_invalid_use_index(self):
# ddoc id for the age index
ddocid = "_design/ad3d537c03cd7c6a43cf8dff66ef70ea54c2b40f"
try:
self.db.find({}, use_index=ddocid)
except Exception as e:
self.assertEqual(e.response.status_code, 400)
else:
raise AssertionError("bad find")
r = self.db.find({}, use_index=ddocid, return_raw=True)
self.assertEqual(r["warning"], '{0} was not used because it does not contain a valid index for this query.'.format(ddocid))

def test_uses_index_when_no_range_or_equals(self):
# index on ["manager"] should be valid because
Expand All @@ -87,19 +83,18 @@ def test_uses_index_when_no_range_or_equals(self):
resp_explain = self.db.find(selector, explain=True)
self.assertEqual(resp_explain["index"]["type"], "json")


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace

def test_reject_use_index_invalid_fields(self):
# index on ["company","manager"] which should not be valid
ddocid = "_design/a0c425a60cf3c3c09e3c537c9ef20059dcef9198"
selector = {
"company": "Pharmex"
}
try:
self.db.find(selector, use_index=ddocid)
except Exception as e:
self.assertEqual(e.response.status_code, 400)
else:
raise AssertionError("did not reject bad use_index")
r = self.db.find(selector, use_index=ddocid, return_raw=True)
self.assertEqual(r["warning"], '{0} was not used because it does not contain a valid index for this query.'.format(ddocid))

# should still return a correct result
for d in r["docs"]:
self.assertEqual(d["company"], "Pharmex")

def test_reject_use_index_ddoc_and_name_invalid_fields(self):
# index on ["company","manager"] which should not be valid
Expand All @@ -108,41 +103,58 @@ def test_reject_use_index_ddoc_and_name_invalid_fields(self):
selector = {
"company": "Pharmex"
}
try:
self.db.find(selector, use_index=[ddocid,name])
except Exception as e:
self.assertEqual(e.response.status_code, 400)
else:
raise AssertionError("did not reject bad use_index")

resp = self.db.find(selector, use_index=[ddocid,name], return_raw=True)
self.assertEqual(resp["warning"], "{0}, {1} was not used because it is not a valid index for this query.".format(ddocid, name))
Copy link
Contributor

@tonysun83 tonysun83 Nov 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the test case above, you're checking that a value is returned

# should still return a correct result
for d in r["docs"]:
    self.assertEqual(d["company"], "Pharmex")

We should be consistent?


# should still return a correct result
for d in resp["docs"]:
self.assertEqual(d["company"], "Pharmex")

def test_reject_use_index_sort_order(self):
# index on ["company","manager"] which should not be valid
# and there is no valid fallback (i.e. an index on ["company"])
ddocid = "_design/a0c425a60cf3c3c09e3c537c9ef20059dcef9198"
selector = {
"company": {"$gt": None},
"manager": {"$gt": None}
"company": {"$gt": None}
}
try:
self.db.find(selector, use_index=ddocid, sort=[{"manager":"desc"}])
self.db.find(selector, use_index=ddocid, sort=[{"company":"desc"}])
except Exception as e:
self.assertEqual(e.response.status_code, 400)
else:
raise AssertionError("did not reject bad use_index")

def test_reject_use_index_ddoc_and_name_sort_order(self):
# index on ["company","manager"] which should not be valid
ddocid = "_design/a0c425a60cf3c3c09e3c537c9ef20059dcef9198"
name = "a0c425a60cf3c3c09e3c537c9ef20059dcef9198"
def test_use_index_fallback_if_valid_sort(self):
ddocid_valid = "_design/fallbackfoo"
ddocid_invalid = "_design/fallbackfoobar"
self.db.create_index(fields=["foo"], ddoc=ddocid_invalid)
self.db.create_index(fields=["foo", "bar"], ddoc=ddocid_valid)
selector = {
"company": {"$gt": None},
"manager": {"$gt": None}
"foo": {"$gt": None}
}
try:
self.db.find(selector, use_index=[ddocid,name], sort=[{"manager":"desc"}])
except Exception as e:
self.assertEqual(e.response.status_code, 400)
else:
raise AssertionError("did not reject bad use_index")

resp_explain = self.db.find(selector, sort=["foo", "bar"], use_index=ddocid_invalid, explain=True)
self.assertEqual(resp_explain["index"]["ddoc"], ddocid_valid)

resp = self.db.find(selector, sort=["foo", "bar"], use_index=ddocid_invalid, return_raw=True)
self.assertEqual(resp["warning"], '{0} was not used because it does not contain a valid index for this query.'.format(ddocid_invalid))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above, we should get a value back here right? It's going to fall back on the ddoc_valid and return a result, so just confirm a result?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree that we should check the result in the cases above. In this test, however, I don't expect any matches (and can assert that).

self.assertEqual(len(resp["docs"]), 0)

def test_prefer_use_index_over_optimal_index(self):
# index on ["company"] even though index on ["company", "manager"] is better
ddocid_preferred = "_design/testsuboptimal"
self.db.create_index(fields=["baz"], ddoc=ddocid_preferred)
self.db.create_index(fields=["baz", "bar"])
selector = {
"baz": {"$gt": None},
"bar": {"$gt": None}
}
resp = self.db.find(selector, use_index=ddocid_preferred, return_raw=True)
self.assertTrue("warning" not in resp)

resp_explain = self.db.find(selector, use_index=ddocid_preferred, explain=True)
self.assertEqual(resp_explain["index"]["ddoc"], ddocid_preferred)

# This doc will not be saved given the new ddoc validation code
# in couch_mrview
Expand Down