You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by "nickva (via GitHub)" <gi...@apache.org> on 2023/04/14 16:12:44 UTC
[GitHub] [couchdb] nickva commented on a diff in pull request #4512: feat(mango): keys-only covering indexes
nickva commented on code in PR #4512:
URL: https://github.com/apache/couchdb/pull/4512#discussion_r1166983131
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -26,14 +26,20 @@
indexable_fields/1,
field_ranges/1,
- field_ranges/2
+ field_ranges/2,
+
+ covers/2
]).
-include_lib("couch/include/couch_db.hrl").
-include("mango.hrl").
-include("mango_idx.hrl").
-include("mango_idx_view.hrl").
+-ifdef(TEST).
+-import(test_util, [as_selector/1]).
+-endif.
+
Review Comment:
We usually try to avoid imports just as a style preference. Can add a small helper function like `indexable_as_selector` in the `TEST` define section perhaps.
##########
src/mango/src/mango_cursor_view.erl:
##########
@@ -30,25 +30,60 @@
-include_lib("couch_mrview/include/couch_mrview.hrl").
-include_lib("fabric/include/fabric.hrl").
+-include("mango.hrl").
-include("mango_cursor.hrl").
-include("mango_idx.hrl").
-include("mango_idx_view.hrl").
-define(HEARTBEAT_INTERVAL_IN_USEC, 4000000).
+-type cursor_options() :: [{term(), term()}].
+-type message() ::
+ {meta, _}
+ | {row, row_properties()}
+ | {execution_stats, shard_stats()}
+ | {stop, #cursor{}}
+ | {complete, #cursor{}}
+ | {error, any()}.
+
% viewcbargs wraps up the arguments that view_cb uses into a single
% entry in the mrargs.extra list. We use a Map to allow us to later
% add fields without having old messages causing errors/crashes.
-viewcbargs_new(Selector, Fields) ->
+
+-type viewcbargs() ::
+ #{
+ selector => selector(),
+ fields => fields(),
+ covering_index => maybe(#idx{})
+ }.
+
+-spec viewcbargs_new(Selector, Fields, CoveringIndex) -> ViewCBArgs when
+ Selector :: selector(),
+ Fields :: fields(),
+ CoveringIndex :: maybe(#idx{}),
+ ViewCBArgs :: viewcbargs().
+viewcbargs_new(Selector, Fields, CoveringIndex) ->
#{
selector => Selector,
- fields => Fields
+ fields => Fields,
+ covering_index => CoveringIndex
}.
+
+-spec viewcbargs_get(Key, Args) -> maybe(term()) when
Review Comment:
`maybe(term())` looks a bit odd since `undefined` is also a `term()` so it would be just `term()`?
##########
src/mango/src/mango_cursor_view.erl:
##########
@@ -73,13 +108,12 @@ create(Db, Indexes, Selector, Opts) ->
bookmark = Bookmark
}}.
-explain(Cursor) ->
- #cursor{
- opts = Opts
- } = Cursor,
-
+-spec explain(#cursor{}) -> nonempty_list(term()).
+explain(#cursor{opts = Opts} = Cursor) ->
BaseArgs = base_args(Cursor),
- Args = apply_opts(Opts, BaseArgs),
+ Args0 = apply_opts(Opts, BaseArgs),
+ #cursor{index = Index, fields = Fields} = Cursor,
Review Comment:
Could could match Index and Fields in the function head since we already match Opts there.
An even cleaner way is to match on `#cursor{} = Cursor` in the function head.
Then on the first function line:
```
#cursor{index = Index, fields = Fields, opts = Opts} = Cursor
```
##########
src/mango/src/mango.hrl:
##########
@@ -12,6 +12,8 @@
-define(MANGO_ERROR(R), throw({mango_error, ?MODULE, R})).
+-type maybe(A) :: A | undefined.
Review Comment:
This one is a bit as A can be undefined also. I'd would expect Dialyzer should emit some kind of warning here but perhaps our Dialyzer options are not configured properly.
##########
src/mango/src/mango_cursor_view.erl:
##########
@@ -157,7 +197,8 @@ execute(#cursor{db = Db, index = Idx, execution_stats = Stats} = Cursor0, UserFu
BaseArgs = base_args(Cursor),
#cursor{opts = Opts, bookmark = Bookmark} = Cursor,
Args0 = apply_opts(Opts, BaseArgs),
- Args = mango_json_bookmark:update_args(Bookmark, Args0),
+ Args1 = consider_index_coverage(Idx, Cursor#cursor.fields, Args0),
Review Comment:
Could just add `fields = Fields` to the `#cursor{...} = Cursor` match line above?
##########
src/mango/src/mango_cursor_view.erl:
##########
@@ -482,6 +564,32 @@ apply_opts([{_, _} | Rest], Args) ->
% Ignore unknown options
apply_opts(Rest, Args).
+-spec consider_index_coverage(#idx{}, fields(), #mrargs{}) -> #mrargs{}.
+consider_index_coverage(Index, Fields, #mrargs{include_docs = IncludeDocs} = Args) ->
+ Covering = mango_idx_view:covers(Index, Fields),
+ Args0 = Args#mrargs{include_docs = IncludeDocs andalso (not Covering)},
Review Comment:
It is a bit odd to have `Args` then `Args0`.
We usually have an `Args0` when we need to pre-process the value and want to keep the rest of the code using `Args`. Or if we keep updating the value Args, we'd have `Args, Args1, Args2, ...`
So in this case we might have Arg0 in the function clause, then after updating it have `Args = Args0#mrargs{include_docs = ...}`
##########
src/mango/src/mango_cursor_view.erl:
##########
@@ -482,6 +564,32 @@ apply_opts([{_, _} | Rest], Args) ->
% Ignore unknown options
apply_opts(Rest, Args).
+-spec consider_index_coverage(#idx{}, fields(), #mrargs{}) -> #mrargs{}.
+consider_index_coverage(Index, Fields, #mrargs{include_docs = IncludeDocs} = Args) ->
Review Comment:
I had noticed that we always calls `apply_opts` then right after always `call consider_index_coverage`. It might make sense to have a function like `apply_cursor_opts` which then may still call `apply_opts` and `consider_index_coverage` (since both are tested separately they can stay that way to minimize the changes diff).
##########
src/mango/src/mango_cursor_view.erl:
##########
@@ -541,6 +651,746 @@ update_bookmark_keys(Cursor, _Props) ->
-ifdef(TEST).
-include_lib("eunit/include/eunit.hrl").
+viewcbargs_test() ->
+ ViewCBArgs = viewcbargs_new(selector, fields, index),
+ ?assertEqual(selector, viewcbargs_get(selector, ViewCBArgs)),
+ ?assertEqual(fields, viewcbargs_get(fields, ViewCBArgs)),
+ ?assertEqual(index, viewcbargs_get(covering_index, ViewCBArgs)),
+ ?assertError(function_clause, viewcbargs_get(something_else, ViewCBArgs)).
+
+maybe_replace_max_json_test() ->
+ ?assertEqual([], maybe_replace_max_json([])),
+ ?assertEqual(<<"<MAX>">>, maybe_replace_max_json(?MAX_STR)),
+ ?assertEqual(
+ [val1, val2, <<"<MAX>">>, val3], maybe_replace_max_json([val1, val2, ?MAX_JSON_OBJ, val3])
+ ),
+ ?assertEqual(something, maybe_replace_max_json(something)).
+
+base_opts_test() ->
+ Index =
+ #idx{
+ type = <<"json">>,
+ def = {[{<<"fields">>, {[{field1, undefined}, {field2, undefined}]}}]}
+ },
+ Fields = [field1, field2],
+ Cursor =
+ #cursor{
+ index = Index,
+ selector = selector,
+ fields = Fields,
+ ranges = [{'$gte', start_key, '$lte', end_key}]
+ },
+ Extra =
+ [
+ {callback, {mango_cursor_view, view_cb}},
+ {selector, selector},
+ {callback_args, #{
+ selector => selector,
+ fields => Fields,
+ covering_index => undefined
+ }},
+ {ignore_partition_query_limit, true}
+ ],
+ MRArgs =
+ #mrargs{
+ view_type = map,
+ reduce = false,
+ start_key = [start_key],
+ end_key = [end_key, ?MAX_JSON_OBJ],
+ include_docs = true,
+ extra = Extra
+ },
+ ?assertEqual(MRArgs, base_args(Cursor)).
+
+apply_opts_empty_test() ->
+ ?assertEqual(args, apply_opts([], args)).
+
+apply_opts_r_test() ->
+ Args = #mrargs{},
+ ArgsWithDocs = Args#mrargs{include_docs = true},
+ ?assertEqual(ArgsWithDocs, apply_opts([{r, "1"}], Args)),
+ ArgsWithoutDocs = Args#mrargs{include_docs = false},
+ ?assertEqual(ArgsWithoutDocs, apply_opts([{r, "3"}], Args)).
+
+apply_opts_conflicts_test() ->
+ Args = #mrargs{},
+ ArgsWithConflicts = Args#mrargs{conflicts = true},
+ ?assertEqual(ArgsWithConflicts, apply_opts([{conflicts, true}], Args)),
+ ArgsWithoutConflicts = Args#mrargs{conflicts = undefined},
+ ?assertEqual(ArgsWithoutConflicts, apply_opts([{conflicts, false}], Args)).
+
+apply_opts_sort_test() ->
+ Args =
+ #mrargs{
+ start_key = start_key,
+ start_key_docid = start_key_docid,
+ end_key = end_key,
+ end_key_docid = end_key_docid
+ },
+ ?assertEqual(Args, apply_opts([{sort, {[]}}], Args)),
+ ?assertEqual(Args, apply_opts([{sort, {[{field1, <<"asc">>}]}}], Args)),
+ ?assertEqual(Args, apply_opts([{sort, {[{field1, <<"asc">>}, {field2, <<"desc">>}]}}], Args)),
+ ArgsWithSort =
+ Args#mrargs{
+ direction = rev,
+ start_key = end_key,
+ start_key_docid = end_key_docid,
+ end_key = start_key,
+ end_key_docid = start_key_docid
+ },
+ ?assertEqual(ArgsWithSort, apply_opts([{sort, {[{field1, <<"desc">>}]}}], Args)).
+
+apply_opts_stale_test() ->
+ Args = #mrargs{},
+ ArgsWithStale = Args#mrargs{stable = true, update = false},
+ ?assertEqual(ArgsWithStale, apply_opts([{stale, ok}], Args)).
+
+apply_opts_stable_test() ->
+ Args = #mrargs{},
+ ArgsWithStable = Args#mrargs{stable = true},
+ ?assertEqual(ArgsWithStable, apply_opts([{stable, true}], Args)),
+ ArgsWithoutStable = Args#mrargs{stable = false},
+ ?assertEqual(ArgsWithoutStable, apply_opts([{stable, false}], Args)).
+
+apply_opts_update_test() ->
+ Args = #mrargs{},
+ ArgsWithUpdate = Args#mrargs{update = true},
+ ?assertEqual(ArgsWithUpdate, apply_opts([{update, true}], Args)),
+ ArgsWithoutUpdate = Args#mrargs{update = false},
+ ?assertEqual(ArgsWithoutUpdate, apply_opts([{update, false}], Args)).
+
+apply_opts_partition_test() ->
+ Args = #mrargs{},
+ ArgsWithPartition = Args#mrargs{extra = [{partition, <<"partition">>}]},
+ ?assertEqual(ArgsWithPartition, apply_opts([{partition, <<"partition">>}], Args)),
+ ArgsWithoutPartition = Args#mrargs{extra = []},
+ ?assertEqual(ArgsWithoutPartition, apply_opts([{partition, <<>>}], Args)).
+
+consider_index_coverage_positive_test() ->
+ Index =
+ #idx{
+ type = <<"json">>,
+ def = {[{<<"fields">>, {[]}}]}
+ },
+ Fields = [<<"_id">>],
+ MRArgs =
+ #mrargs{
+ include_docs = true,
+ extra = [{callback_args, viewcbargs_new(selector, fields, undefined)}]
+ },
+ MRArgsRef =
+ MRArgs#mrargs{
+ include_docs = false,
+ extra = [{callback_args, viewcbargs_new(selector, fields, Index)}]
+ },
+ ?assertEqual(MRArgsRef, consider_index_coverage(Index, Fields, MRArgs)),
+ MRArgs1 = MRArgs#mrargs{include_docs = false},
+ ?assertEqual(MRArgsRef, consider_index_coverage(Index, Fields, MRArgs1)).
+
+consider_index_coverage_negative_test() ->
+ Index = undefined,
+ Fields = all_fields,
+ MRArgs = #mrargs{include_docs = true},
+ ?assertEqual(MRArgs, consider_index_coverage(Index, Fields, MRArgs)),
+ MRArgs1 = #mrargs{include_docs = false},
+ ?assertEqual(MRArgs1, consider_index_coverage(Index, Fields, MRArgs1)),
+ % no extra attributes hence no effect
+ Index1 =
+ #idx{
+ type = <<"json">>,
+ def = {[{<<"fields">>, {[]}}]}
+ },
+ MRArgs2 = #mrargs{include_docs = false},
+ ?assertEqual(MRArgs1, consider_index_coverage(Index1, [<<"_id">>], MRArgs2)).
+
+derive_doc_from_index_test() ->
+ Index =
+ #idx{
+ type = <<"json">>,
+ def = {[{<<"fields">>, {[{<<"field1">>, undefined}, {<<"field2">>, undefined}]}}]}
+ },
+ DocId = doc_id,
+ Keys = [key1, key2],
+ ViewRow = #view_row{id = DocId, key = Keys},
+ Doc = {[{<<"_id">>, DocId}, {<<"field2">>, key2}, {<<"field1">>, key1}]},
+ ?assertEqual(Doc, derive_doc_from_index(Index, ViewRow)).
+
+composite_indexes_test() ->
+ ?assertEqual([], composite_indexes([], [])),
+ Index1 =
+ #idx{
+ type = <<"json">>,
+ def = {[{<<"fields">>, {[{field1, undefined}, {field2, undefined}]}}]}
+ },
+ Index2 =
+ #idx{
+ type = <<"json">>,
+ def = {[{<<"fields">>, {[{field1, undefined}, {field3, undefined}, {field4, range4}]}}]}
+ },
+ Index3 =
+ #idx{
+ type = <<"json">>,
+ def = {[{<<"fields">>, {[{field3, undefined}, {field4, undefined}]}}]}
+ },
+ Indexes = [Index1, Index2, Index3],
+ Ranges = [{field1, range1}, {field3, range3}, {field4, range4}],
+ Result = [
+ {Index3, [range3, range4], 1}, {Index2, [range1, range3, range4], 0}, {Index1, [range1], 2}
+ ],
+ ?assertEqual(Result, composite_indexes(Indexes, Ranges)).
+
+create_test() ->
+ Index = #idx{type = <<"json">>, def = {[{<<"fields">>, {[]}}]}},
+ Indexes = [Index],
+ Ranges = [],
+ Selector = {[]},
+ Options = [{limit, limit}, {skip, skip}, {fields, fields}, {bookmark, bookmark}],
+ Cursor =
+ #cursor{
+ db = db,
+ index = Index,
+ ranges = Ranges,
+ selector = Selector,
+ opts = Options,
+ limit = limit,
+ skip = skip,
+ fields = fields,
+ bookmark = bookmark
+ },
+ ?assertEqual({ok, Cursor}, create(db, Indexes, Selector, Options)).
+
+explain_test() ->
+ Cursor =
+ #cursor{
+ ranges = [empty],
+ fields = all_fields,
+ opts = []
+ },
+ Response =
+ [
+ {mrargs,
+ {[
+ {include_docs, true},
+ {view_type, map},
+ {reduce, false},
+ {partition, null},
+ {start_key, null},
+ {end_key, null},
+ {direction, fwd},
+ {stable, false},
+ {update, true},
+ {conflicts, undefined}
+ ]}},
+ {covered, false}
+ ],
+ ?assertEqual(Response, explain(Cursor)).
+
+execute_empty_test() ->
+ Cursor = #cursor{ranges = [empty]},
+ meck:new(fabric),
+ meck:expect(fabric, all_docs, ['_', '_', '_', '_', '_'], meck:val(error)),
+ meck:expect(fabric, query_view, ['_', '_', '_', '_', '_', '_'], meck:val(error)),
+ ?assertEqual({ok, accumulator}, execute(Cursor, undefined, accumulator)),
+ ?assertNot(meck:called(fabric, all_docs, '_')),
+ ?assertNot(meck:called(fabric, query_view, '_')),
+ meck:unload(fabric).
Review Comment:
If test crashes half-way would meck automatically unload fabric and other mocks? If there is a chance it won't we should probably wrap the tests into a suite with a `{foreach, setup/0, teardown/1, [?TDEF_FE(...),...]}` construct and use `meck:unload()` in `teardown/1`, that would unload all the mocks.
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -298,6 +304,8 @@ indexable({[{<<"$gt">>, _}]}) ->
true;
indexable({[{<<"$gte">>, _}]}) ->
true;
+indexable({[{<<"$exists">>, _}]}) ->
Review Comment:
What's the intuition about making `$exists` indexable as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org