nickva commented on code in PR #4984:
URL: https://github.com/apache/couchdb/pull/4984#discussion_r1512986792
##########
src/fabric/include/fabric.hrl:
##########
@@ -42,3 +42,17 @@
-record(view_row, {key, id, value, doc, worker}).
-record(change, {key, id, value, deleted=false, doc, worker}).
+
+-type row_property_key() :: id | key | value | doc | worker.
+-type row_properties() :: [{row_property_key(), any()}].
+
+-type view_row_map() ::
+ #{
+ id => term(),
+ key => term(),
+ value => term(),
+ doc => term(),
+ worker => term()
Review Comment:
Are any of those mandatory, perhaps those could be `:=`?
##########
src/fabric/src/fabric_view_row.erl:
##########
@@ -0,0 +1,494 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(fabric_view_row).
+-export([from_props/2]).
+-export([get_id/1, get_key/1, get_value/1, get_doc/1, get_worker/1]).
+-export([set_key/2, set_doc/2, set_worker/2]).
+-export([transform/1, merge/3]).
+
+-include_lib("fabric/include/fabric.hrl").
+
+from_props(Props, Options) ->
+ case couch_util:get_value(view_row_map, Options, false) of
+ true ->
+ Row = lists:foldl(
+ fun({Key, Value}, ViewRow) -> ViewRow#{Key => Value} end,
+ #{},
+ Props
+ ),
+ {view_row, Row};
+ false ->
+ #view_row{
+ key = couch_util:get_value(key, Props),
+ id = couch_util:get_value(id, Props),
+ value = couch_util:get_value(value, Props),
+ doc = couch_util:get_value(doc, Props),
+ worker = couch_util:get_value(worker, Props)
+ }
+ end.
+
+get_id(#view_row{id = Id}) ->
+ Id;
+get_id({view_row, #{id := Id}}) ->
+ Id;
+get_id({view_row, #{}}) ->
+ undefined.
+
+get_key(#view_row{key = Key}) ->
+ Key;
+get_key({view_row, #{key := Key}}) ->
+ Key;
+get_key({view_row, #{}}) ->
+ undefined.
+
+set_key(#view_row{} = Row, Key) ->
+ Row#view_row{key = Key};
+set_key({view_row, #{} = Row}, Key) ->
+ {view_row, Row#{key => Key}}.
+
+get_value(#view_row{value = Value}) ->
+ Value;
+get_value({view_row, #{value := Value}}) ->
+ Value;
+get_value({view_row, #{}}) ->
+ undefined.
+
+get_doc(#view_row{doc = Doc}) ->
+ Doc;
+get_doc({view_row, #{doc := Doc}}) ->
+ Doc;
+get_doc({view_row, #{}}) ->
+ undefined.
+
+set_doc(#view_row{} = Row, Doc) ->
+ Row#view_row{doc = Doc};
+set_doc({view_row, #{} = Row}, Doc) ->
+ {view_row, Row#{doc => Doc}}.
+
+get_worker(#view_row{worker = Worker}) ->
+ Worker;
+get_worker({view_row, #{worker := Worker}}) ->
+ Worker;
+get_worker({view_row, #{}}) ->
+ undefined.
+
+set_worker(#view_row{} = Row, Worker) ->
+ Row#view_row{worker = Worker};
+set_worker({view_row, #{} = Row}, Worker) ->
+ {view_row, Row#{worker => Worker}}.
+
+transform(#view_row{value = {[{reduce_overflow_error, Msg}]}}) ->
+ {row, [{key, null}, {id, error}, {value, reduce_overflow_error}, {reason,
Msg}]};
+transform(#view_row{key = Key, id = reduced, value = Value}) ->
+ {row, [{key, Key}, {value, Value}]};
+transform(#view_row{key = Key, id = undefined}) ->
+ {row, [{key, Key}, {id, error}, {value, not_found}]};
+transform(#view_row{key = Key, id = Id, value = Value, doc = undefined}) ->
+ {row, [{id, Id}, {key, Key}, {value, Value}]};
+transform(#view_row{key = Key, doc = {error, Reason}}) ->
+ {row, [{id, error}, {key, Key}, {value, Reason}]};
+transform(#view_row{key = Key, id = Id, value = Value, doc = Doc}) ->
+ {row, [{id, Id}, {key, Key}, {value, Value}, {doc, Doc}]};
+transform({view_row, #{} = Row0}) ->
+ Id = maps:get(id, Row0, undefined),
+ Key = maps:get(key, Row0, undefined),
+ Value = maps:get(value, Row0, undefined),
+ Doc = maps:get(doc, Row0, undefined),
+ Worker = maps:get(worker, Row0, undefined),
+ Row = #view_row{id = Id, key = Key, value = Value, doc = Doc, worker =
Worker},
+ transform(Row).
+
+merge(Dir, Row, Rows) ->
+ lists:merge(
+ fun(RowA, RowB) ->
+ IdA = get_id(RowA),
+ IdB = get_id(RowB),
+ case Dir of
+ fwd -> IdA =< IdB;
+ rev -> IdA > IdB
Review Comment:
Hmm, I wonder if it should be `A<B` and `A>B`?
Erlang lists module expect the comparison operator to basically be `=<` or
`>=`. However our btrees use `less` (strictly `<`). I don't know if that's an
issue here since we don't expect two equal document IDs to come through once we
start streaming for all_docs case. But it might be good to stick to `<` and `>
`just to emphasize that
think our sorting function on the btrees is a strictly `less than` function
and we want to make sure we use the same sorting function in the workers as we
do in the coordinator.
##########
src/fabric/src/fabric_view_row.erl:
##########
@@ -0,0 +1,494 @@
+% Licensed under the Apache License, Version 2.0 (the "License"); you may not
+% use this file except in compliance with the License. You may obtain a copy of
+% the License at
+%
+% http://www.apache.org/licenses/LICENSE-2.0
+%
+% Unless required by applicable law or agreed to in writing, software
+% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+% License for the specific language governing permissions and limitations under
+% the License.
+
+-module(fabric_view_row).
+-export([from_props/2]).
+-export([get_id/1, get_key/1, get_value/1, get_doc/1, get_worker/1]).
+-export([set_key/2, set_doc/2, set_worker/2]).
+-export([transform/1, merge/3]).
+
+-include_lib("fabric/include/fabric.hrl").
+
+from_props(Props, Options) ->
+ case couch_util:get_value(view_row_map, Options, false) of
+ true ->
+ Row = lists:foldl(
+ fun({Key, Value}, ViewRow) -> ViewRow#{Key => Value} end,
+ #{},
+ Props
+ ),
+ {view_row, Row};
+ false ->
+ #view_row{
+ key = couch_util:get_value(key, Props),
+ id = couch_util:get_value(id, Props),
+ value = couch_util:get_value(value, Props),
+ doc = couch_util:get_value(doc, Props),
+ worker = couch_util:get_value(worker, Props)
+ }
+ end.
+
+get_id(#view_row{id = Id}) ->
+ Id;
+get_id({view_row, #{id := Id}}) ->
+ Id;
+get_id({view_row, #{}}) ->
+ undefined.
+
+get_key(#view_row{key = Key}) ->
+ Key;
+get_key({view_row, #{key := Key}}) ->
+ Key;
+get_key({view_row, #{}}) ->
+ undefined.
+
+set_key(#view_row{} = Row, Key) ->
+ Row#view_row{key = Key};
+set_key({view_row, #{} = Row}, Key) ->
+ {view_row, Row#{key => Key}}.
+
+get_value(#view_row{value = Value}) ->
+ Value;
+get_value({view_row, #{value := Value}}) ->
+ Value;
+get_value({view_row, #{}}) ->
+ undefined.
+
+get_doc(#view_row{doc = Doc}) ->
+ Doc;
+get_doc({view_row, #{doc := Doc}}) ->
+ Doc;
+get_doc({view_row, #{}}) ->
+ undefined.
+
+set_doc(#view_row{} = Row, Doc) ->
+ Row#view_row{doc = Doc};
+set_doc({view_row, #{} = Row}, Doc) ->
+ {view_row, Row#{doc => Doc}}.
+
+get_worker(#view_row{worker = Worker}) ->
+ Worker;
+get_worker({view_row, #{worker := Worker}}) ->
+ Worker;
+get_worker({view_row, #{}}) ->
+ undefined.
+
+set_worker(#view_row{} = Row, Worker) ->
+ Row#view_row{worker = Worker};
+set_worker({view_row, #{} = Row}, Worker) ->
+ {view_row, Row#{worker => Worker}}.
+
+transform(#view_row{value = {[{reduce_overflow_error, Msg}]}}) ->
+ {row, [{key, null}, {id, error}, {value, reduce_overflow_error}, {reason,
Msg}]};
+transform(#view_row{key = Key, id = reduced, value = Value}) ->
+ {row, [{key, Key}, {value, Value}]};
+transform(#view_row{key = Key, id = undefined}) ->
+ {row, [{key, Key}, {id, error}, {value, not_found}]};
+transform(#view_row{key = Key, id = Id, value = Value, doc = undefined}) ->
+ {row, [{id, Id}, {key, Key}, {value, Value}]};
+transform(#view_row{key = Key, doc = {error, Reason}}) ->
+ {row, [{id, error}, {key, Key}, {value, Reason}]};
+transform(#view_row{key = Key, id = Id, value = Value, doc = Doc}) ->
+ {row, [{id, Id}, {key, Key}, {value, Value}, {doc, Doc}]};
+transform({view_row, #{} = Row0}) ->
+ Id = maps:get(id, Row0, undefined),
+ Key = maps:get(key, Row0, undefined),
+ Value = maps:get(value, Row0, undefined),
+ Doc = maps:get(doc, Row0, undefined),
+ Worker = maps:get(worker, Row0, undefined),
+ Row = #view_row{id = Id, key = Key, value = Value, doc = Doc, worker =
Worker},
+ transform(Row).
+
+merge(Dir, Row, Rows) ->
+ lists:merge(
+ fun(RowA, RowB) ->
+ IdA = get_id(RowA),
+ IdB = get_id(RowB),
+ case Dir of
+ fwd -> IdA =< IdB;
+ rev -> IdA > IdB
+ end
+ end,
+ [Row],
+ Rows
+ ).
Review Comment:
I think this is specifically merging rows by ID. We also merge view rows by
key and ID, with and without a keydict and with or without a unicode collator.
I wonder if it would make sense to move all merging to the fabric_view_row
then, or move this back to all_docs handler. Mainly so it looks consistent.
##########
src/fabric/src/fabric_view.erl:
##########
@@ -239,14 +234,15 @@ possibly_embed_doc(
Resp
end
end;
- _ ->
+ false ->
Row
end.
-detach_partition(#view_row{key = {p, _Partition, Key}} = Row) ->
- Row#view_row{key = Key};
-detach_partition(#view_row{} = Row) ->
- Row.
+detach_partition(Row) ->
+ case fabric_view_row:get_key(Row) of
+ {p, _Partition, Key} -> fabric_view_row:set_key(Row, Key);
+ _PrimitiveKey -> Row
Review Comment:
Primitive is a bit of an odd name, to avoid introducing new terminology
maybe just use `_`?
--
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-unsubscr...@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
