[couchdb] branch 3.x updated: Properly sort descending=true view results when a key list is provided

[va...@apache.org]

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

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


The following commit(s) were added to refs/heads/3.x by this push:
     new 22189ea  Properly sort descending=true view results when a key list is provided
22189ea is described below

commit 22189ea6ad3665639bd3223d9ef915fe2561e2d1
Author: Nick Vatamaniuc <va...@gmail.com>
AuthorDate: Fri Sep 17 16:43:42 2021 -0400

    Properly sort descending=true view results when a key list is provided
    
    Results should now be returned in descending {key, doc_id} order.
    
    The idea is to reverse the key list before sending it to the workers, so they
    will emit rows in reverse order. Also, we are using the same reversed list when
    building the KeyDict structure on the coordinator. That way the order of the
    sent rows and the expected coordinator sorting order will match.
    
    For testing, enhance an existing multi-key Elixir view test to test both
    ascending and descending cases and actually check that the rows are in the
    correct order each time.
---
 src/fabric/src/fabric_view_map.erl              | 18 +++++++----
 test/elixir/test/view_multi_key_design_test.exs | 40 +++++++++++++++++++++----
 2 files changed, 48 insertions(+), 10 deletions(-)

diff --git a/src/fabric/src/fabric_view_map.erl b/src/fabric/src/fabric_view_map.erl
index ff6aa8b..8ab8083 100644
--- a/src/fabric/src/fabric_view_map.erl
+++ b/src/fabric/src/fabric_view_map.erl
@@ -24,8 +24,14 @@ go(DbName, Options, GroupId, View, Args, Callback, Acc, VInfo)
     {ok, DDoc} = fabric:open_doc(DbName, <<"_design/", GroupId/binary>>, []),
     go(DbName, Options, DDoc, View, Args, Callback, Acc, VInfo);
 
-go(Db, Options, DDoc, View, Args, Callback, Acc, VInfo) ->
+go(Db, Options, DDoc, View, Args0, Callback, Acc, VInfo) ->
     DbName = fabric:dbname(Db),
+    Args = case Args0 of
+        #mrargs{keys = Keys, direction = rev} when is_list(Keys) ->
+            Args0#mrargs{keys = lists:reverse(Keys)};
+        #mrargs{} ->
+            Args0
+    end,
     {Shards, RingOpts} = fabric_view:get_shards(Db, Args),
     {CoordArgs, WorkerArgs} = fabric_view:fix_skip_and_limit(Args),
     DocIdAndRev = fabric_util:doc_id_and_rev(DDoc),
@@ -221,10 +227,12 @@ merge_row(Dir, Collation, KeyDict0, Row, Rows0) ->
                             IdA < IdB;
                         {rev, 0} ->
                             IdB < IdA;
-                        {fwd, _} ->
-                            dict:fetch(A, KeyDict1) < dict:fetch(B, KeyDict1);
-                        {rev, _} ->
-                            dict:fetch(B, KeyDict1) < dict:fetch(A, KeyDict1)
+                        {_, _} ->
+                            % We already have a reversed key dict, and sent the
+                            % workers the same reversed keys list. So here we
+                            % just enforce sorting according to the order in
+                            % the key dict
+                            dict:fetch(A, KeyDict1) < dict:fetch(B, KeyDict1)
                     end
                 end,
                 [Row],
diff --git a/test/elixir/test/view_multi_key_design_test.exs b/test/elixir/test/view_multi_key_design_test.exs
index ab57e89..c334916 100644
--- a/test/elixir/test/view_multi_key_design_test.exs
+++ b/test/elixir/test/view_multi_key_design_test.exs
@@ -209,16 +209,38 @@ defmodule ViewMultiKeyDesignTest do
     assert length(rows) == 99
   end
 
-  test "dir works", context do
+  test "dir ascending works", context do
     db_name = context[:db_name]
 
-    resp = view(db_name, "test/multi_emit", [descending: true], [1])
+    expect_rows = mk_rows(0..99, 1, &</2) ++ mk_rows(0..99, 2, &</2)
+
+    resp = view(db_name, "test/multi_emit", [descending: false], [1, 2])
     rows = resp.body["rows"]
-    assert length(rows) == 100
+    assert length(rows) == 200
+    assert expect_rows == rows
 
-    resp = view(db_name, "test/multi_emit", descending: true, keys: :jiffy.encode([1]))
+    keys = :jiffy.encode([1, 2])
+    resp = view(db_name, "test/multi_emit", descending: false, keys: keys)
     rows = resp.body["rows"]
-    assert length(rows) == 100
+    assert length(rows) == 200
+    assert expect_rows == rows
+  end
+
+  test "dir descending works", context do
+    db_name = context[:db_name]
+
+    expect_rows = mk_rows(0..99, 2, &>/2) ++ mk_rows(0..99, 1, &>/2)
+
+    resp = view(db_name, "test/multi_emit", [descending: true], [1, 2])
+    rows = resp.body["rows"]
+    assert length(rows) == 200
+    assert expect_rows == rows
+
+    keys = :jiffy.encode([1, 2])
+    resp = view(db_name, "test/multi_emit", descending: true, keys: keys)
+    rows = resp.body["rows"]
+    assert length(rows) == 200
+    assert expect_rows == rows
   end
 
   test "argument combinations", context do
@@ -313,4 +335,12 @@ defmodule ViewMultiKeyDesignTest do
     rows = resp.body["rows"]
     assert length(rows) == 2
   end
+
+  defp mk_rows(range, key, sort_fun) do
+    row_fun = fn(i) -> %{"id" => "#{i}", "key" => key, "value" => i} end
+    sort_mapper = fn(row) -> {row["key"], row["id"]} end
+    range
+    |> Enum.map(row_fun)
+    |> Enum.sort_by(sort_mapper, sort_fun)
+  end
 end