[couchdb] 10/11: mango: fix definition of index coverage

[va...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

vatamane pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/couchdb.git

commit 28480f9a438fa27e35219b9e09fcaaebe8010047
Author: Gabor Pali <ga...@ibm.com>
AuthorDate: Mon Apr 17 19:49:34 2023 +0200

    mango: fix definition of index coverage
    
    Covering indexes shall provide all the fields that the selector
    may contain, otherwise the derived documents would get dropped on
    the "match and extract" phase even if they were matching.  Extend
    the integration tests to check this case as well.
---
 src/mango/src/mango_cursor_view.erl      | 45 +++++++++++++++++++++++++++--
 src/mango/src/mango_selector.erl         | 49 +++++++++++++++++++++++++++++++-
 src/mango/test/22-covering-index-test.py | 18 ++++++++++--
 3 files changed, 107 insertions(+), 5 deletions(-)

diff --git a/src/mango/src/mango_cursor_view.erl b/src/mango/src/mango_cursor_view.erl
index 474d3bfe6..4dbea77c8 100644
--- a/src/mango/src/mango_cursor_view.erl
+++ b/src/mango/src/mango_cursor_view.erl
@@ -108,11 +108,18 @@ create(Db, Indexes, Selector, Opts) ->
         bookmark = Bookmark
     }}.
 
+-spec required_fields(#cursor{}) -> fields().
+required_fields(#cursor{fields = all_fields}) ->
+    all_fields;
+required_fields(#cursor{fields = Fields, selector = Selector}) ->
+    lists:usort(Fields ++ mango_selector:fields(Selector)).
+
 -spec explain(#cursor{}) -> nonempty_list(term()).
 explain(#cursor{opts = Opts} = Cursor) ->
     BaseArgs = base_args(Cursor),
     Args0 = apply_opts(Opts, BaseArgs),
-    #cursor{index = Index, fields = Fields} = Cursor,
+    #cursor{index = Index} = Cursor,
+    Fields = required_fields(Cursor),
     Args = consider_index_coverage(Index, Fields, Args0),
 
     [
@@ -197,7 +204,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),
-            Args1 = consider_index_coverage(Idx, Cursor#cursor.fields, Args0),
+            Fields = required_fields(Cursor),
+            Args1 = consider_index_coverage(Idx, Fields, Args0),
             Args = mango_json_bookmark:update_args(Bookmark, Args1),
             UserCtx = couch_util:get_value(user_ctx, Opts, #user_ctx{}),
             DbOpts = [{user_ctx, UserCtx}],
@@ -859,6 +867,39 @@ create_test() ->
         },
     ?assertEqual({ok, Cursor}, create(db, Indexes, Selector, Options)).
 
+to_selector(Map) ->
+    test_util:as_selector(Map).
+
+required_fields_all_fields_test() ->
+    Cursor = #cursor{fields = all_fields},
+    ?assertEqual(all_fields, required_fields(Cursor)).
+
+required_fields_disjoint_fields_test() ->
+    Fields1 = [<<"field1">>, <<"field2">>, <<"field3">>],
+    Selector1 = to_selector(#{}),
+    Cursor1 = #cursor{fields = Fields1, selector = Selector1},
+    ?assertEqual([<<"field1">>, <<"field2">>, <<"field3">>], required_fields(Cursor1)),
+    Fields2 = [<<"field1">>, <<"field2">>],
+    Selector2 = to_selector(#{<<"field3">> => undefined, <<"field4">> => undefined}),
+    Cursor2 = #cursor{fields = Fields2, selector = to_selector(Selector2)},
+    ?assertEqual(
+        [<<"field1">>, <<"field2">>, <<"field3">>, <<"field4">>], required_fields(Cursor2)
+    ).
+
+required_fields_overlapping_fields_test() ->
+    Fields1 = [<<"field1">>, <<"field2">>, <<"field3">>],
+    Selector1 = to_selector(#{<<"field3">> => undefined, <<"field4">> => undefined}),
+    Cursor1 = #cursor{fields = Fields1, selector = Selector1},
+    ?assertEqual(
+        [<<"field1">>, <<"field2">>, <<"field3">>, <<"field4">>], required_fields(Cursor1)
+    ),
+    Fields2 = [<<"field3">>, <<"field1">>, <<"field2">>],
+    Selector2 = to_selector(#{<<"field4">> => undefined, <<"field1">> => undefined}),
+    Cursor2 = #cursor{fields = Fields2, selector = Selector2},
+    ?assertEqual(
+        [<<"field1">>, <<"field2">>, <<"field3">>, <<"field4">>], required_fields(Cursor2)
+    ).
+
 explain_test() ->
     Cursor =
         #cursor{
diff --git a/src/mango/src/mango_selector.erl b/src/mango/src/mango_selector.erl
index 7de16bd51..59be7a6eb 100644
--- a/src/mango/src/mango_selector.erl
+++ b/src/mango/src/mango_selector.erl
@@ -16,7 +16,8 @@
     normalize/1,
     match/2,
     has_required_fields/2,
-    is_constant_field/2
+    is_constant_field/2,
+    fields/1
 ]).
 
 -include_lib("couch/include/couch_db.hrl").
@@ -638,6 +639,14 @@ is_constant_field([{[{Field, {[{Cond, _Val}]}}]} | _Rest], Field) ->
 is_constant_field([{[{_UnMatched, _}]} | Rest], Field) ->
     is_constant_field(Rest, Field).
 
+-spec fields(selector()) -> fields().
+fields({[{<<"$", _/binary>>, Args}]}) when is_list(Args) ->
+    lists:flatmap(fun fields/1, Args);
+fields({[{Field, _Cond}]}) ->
+    [Field];
+fields({[]}) ->
+    [].
+
 %%%%%%%% module tests below %%%%%%%%
 
 -ifdef(TEST).
@@ -1007,4 +1016,42 @@ match_demo_test_() ->
         ?_assertEqual(false, Check({[{<<"_id">>, <<"foo">>}, {<<"_rev">>, <<"quux">>}]}))
     ].
 
+fields_of(Selector) ->
+    fields(test_util:as_selector(Selector)).
+
+fields_empty_test() ->
+    ?assertEqual([], fields_of(#{})).
+
+fields_primitive_test() ->
+    Selector = #{<<"field">> => undefined},
+    ?assertEqual([<<"field">>], fields_of(Selector)).
+
+fields_nested_test() ->
+    Selector = #{<<"field1">> => #{<<"field2">> => undefined}},
+    ?assertEqual([<<"field1.field2">>], fields_of(Selector)).
+
+fields_and_test() ->
+    Selector1 = #{<<"$and">> => []},
+    ?assertEqual([], fields_of(Selector1)),
+    Selector2 = #{
+        <<"$and">> => [#{<<"field1">> => undefined}, #{<<"field2">> => undefined}]
+    },
+    ?assertEqual([<<"field1">>, <<"field2">>], fields_of(Selector2)).
+
+fields_or_test() ->
+    Selector1 = #{<<"$or">> => []},
+    ?assertEqual([], fields_of(Selector1)),
+    Selector2 = #{
+        <<"$or">> => [#{<<"field1">> => undefined}, #{<<"field2">> => undefined}]
+    },
+    ?assertEqual([<<"field1">>, <<"field2">>], fields_of(Selector2)).
+
+fields_nor_test() ->
+    Selector1 = #{<<"$nor">> => []},
+    ?assertEqual([], fields_of(Selector1)),
+    Selector2 = #{
+        <<"$nor">> => [#{<<"field1">> => undefined}, #{<<"field2">> => undefined}]
+    },
+    ?assertEqual([<<"field1">>, <<"field2">>], fields_of(Selector2)).
+
 -endif.
diff --git a/src/mango/test/22-covering-index-test.py b/src/mango/test/22-covering-index-test.py
index b2f0202ed..52a7f3612 100644
--- a/src/mango/test/22-covering-index-test.py
+++ b/src/mango/test/22-covering-index-test.py
@@ -21,8 +21,8 @@ class CoveringIndexTests(mango.UserDocsTests):
         self.assertEqual(resp["mrargs"]["include_docs"], False)
         self.assertEqual(resp["covered"], True)
 
-    def is_not_covered(self, selector, fields):
-        resp = self.db.find(selector, fields=fields, explain=True)
+    def is_not_covered(self, selector, fields, use_index=None):
+        resp = self.db.find(selector, fields=fields, use_index=use_index, explain=True)
         self.assertEqual(resp["mrargs"]["include_docs"], True)
         self.assertEqual(resp["covered"], False)
 
@@ -74,6 +74,20 @@ class CoveringIndexTests(mango.UserDocsTests):
     def test_index_does_not_cover_query_partial_selector(self):
         self.is_not_covered({"name.last": "Hernandez"}, ["name.first"])
 
+    def test_index_does_not_cover_selector_with_more_fields(self):
+        self.is_not_covered(
+            {
+                "$and": [
+                    {"age": {"$ne": 23}},
+                    {"twitter": {"$not": {"$regex": "^@.*[0-9]+$"}}},
+                    {"location.address.number": {"$gt": 4288}},
+                    {"location.city": {"$ne": "Pico Rivera"}},
+                ]
+            },
+            ["twitter"],
+            use_index="twitter",
+        )
+
     def test_covering_index_provides_correct_answer_id(self):
         docs = self.db.find({"age": {"$gte": 32}}, fields=["_id"])
         expected = [