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