You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by "pgj (via GitHub)" <gi...@apache.org> on 2023/06/30 16:14:17 UTC
[GitHub] [couchdb] pgj opened a new pull request, #4657: chore(`mango`): replace `foldl` with a `map`
pgj opened a new pull request, #4657:
URL: https://github.com/apache/couchdb/pull/4657
The definition of `mango_cursor_view:composite_indexes/2` does not exploit the possibilities of fold left, it behaves much like a mapping. Using `lists:map` here makes this explicit, which may help with readability and may be implemented in a more specialized manner.
--
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
[GitHub] [couchdb] nickva commented on pull request #4657: chore(`mango`): replace `foldl` with a `map`
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on PR #4657:
URL: https://github.com/apache/couchdb/pull/4657#issuecomment-1615180766
> This is not about the ordering of elements.
The test is failing because the order of the elements changes. So we should at least fix that part.
```
[2023-06-30T16:40:36.458Z] module 'mango_cursor_view'
[2023-06-30T16:40:36.458Z] mango_cursor_view: viewcbargs_test...ok
[2023-06-30T16:40:36.458Z] mango_cursor_view: maybe_replace_max_json_test...ok
[2023-06-30T16:40:36.458Z] mango_cursor_view: base_opts_test...ok
[2023-06-30T16:40:36.458Z] mango_cursor_view: apply_opts_empty_test...ok
[2023-06-30T16:40:36.458Z] mango_cursor_view: apply_opts_r_test...ok
[2023-06-30T16:40:36.458Z] mango_cursor_view: apply_opts_conflicts_test...ok
[2023-06-30T16:40:36.458Z] mango_cursor_view: apply_opts_sort_test...ok
[2023-06-30T16:40:36.458Z] mango_cursor_view: apply_opts_stale_test...ok
[2023-06-30T16:40:36.458Z] mango_cursor_view: apply_opts_stable_test...ok
[2023-06-30T16:40:36.458Z] mango_cursor_view: apply_opts_update_test...ok
[2023-06-30T16:40:36.458Z] mango_cursor_view: apply_opts_partition_test...[0.006 s] ok
[2023-06-30T16:40:36.458Z] mango_cursor_view: consider_index_coverage_positive_test...[0.007 s] ok
[2023-06-30T16:40:36.458Z] mango_cursor_view: consider_index_coverage_negative_test...ok
[2023-06-30T16:40:36.458Z] mango_cursor_view: derive_doc_from_index_test...ok
[2023-06-30T16:40:36.458Z] mango_cursor_view: derive_doc_from_index_partitioned_test...ok
[2023-06-30T16:40:36.458Z] mango_cursor_view: composite_indexes_test...*failed*
[2023-06-30T16:40:36.458Z] in function mango_cursor_view:composite_indexes_test/0 (src/mango_cursor_view.erl, line 903)
[2023-06-30T16:40:36.458Z] in call from eunit_test:'-mf_wrapper/2-fun-0-'/2 (eunit_test.erl, line 273)
[2023-06-30T16:40:36.458Z] in call from eunit_test:run_testfun/1 (eunit_test.erl, line 71)
[2023-06-30T16:40:36.458Z] in call from eunit_proc:run_test/1 (eunit_proc.erl, line 531)
[2023-06-30T16:40:36.458Z] in call from eunit_proc:with_timeout/3 (eunit_proc.erl, line 356)
[2023-06-30T16:40:36.458Z] in call from eunit_proc:handle_test/2 (eunit_proc.erl, line 514)
[2023-06-30T16:40:36.458Z] in call from eunit_proc:tests_inorder/3 (eunit_proc.erl, line 456)
[2023-06-30T16:40:36.458Z] in call from eunit_proc:with_timeout/3 (eunit_proc.erl, line 346)
[2023-06-30T16:40:36.458Z] **error:{assertEqual,[{module,mango_cursor_view},
[2023-06-30T16:40:36.458Z] {line,903},
[2023-06-30T16:40:36.458Z] {expression,"composite_indexes ( Indexes , Ranges )"},
[2023-06-30T16:40:36.458Z] {expected,[{{idx,undefined,undefined,undefined,<<"json">>,
[2023-06-30T16:40:36.458Z] {[{<<...>>,...}]},
[2023-06-30T16:40:36.458Z] undefined,undefined},
[2023-06-30T16:40:36.458Z] [range3,range4],
[2023-06-30T16:40:36.458Z] 1},
[2023-06-30T16:40:36.458Z] {{idx,undefined,undefined,undefined,<<"json">>,
[2023-06-30T16:40:36.458Z] {[{...}]},
[2023-06-30T16:40:36.458Z] undefined,undefined},
[2023-06-30T16:40:36.458Z] [range1,range3,range4],
[2023-06-30T16:40:36.458Z] 0},
[2023-06-30T16:40:36.458Z] {{idx,undefined,undefined,undefined,<<"json">>,
[2023-06-30T16:40:36.458Z] {[...]},
[2023-06-30T16:40:36.458Z] undefined,...},
[2023-06-30T16:40:36.458Z] [range1],
[2023-06-30T16:40:36.458Z] 2}]},
[2023-06-30T16:40:36.458Z] {value,[{{idx,undefined,undefined,undefined,<<"json">>,
[2023-06-30T16:40:36.458Z] {[{...}]},
[2023-06-30T16:40:36.458Z] undefined,undefined},
[2023-06-30T16:40:36.458Z] [range1],
[2023-06-30T16:40:36.458Z] 2},
[2023-06-30T16:40:36.458Z] {{idx,undefined,undefined,undefined,<<"json">>,
[2023-06-30T16:40:36.458Z] {[...]},
[2023-06-30T16:40:36.458Z] undefined,...},
[2023-06-30T16:40:36.458Z] [range1,range3,range4],
[2023-06-30T16:40:36.458Z] 0},
[2023-06-30T16:40:36.458Z] {{idx,undefined,undefined,undefined,<<"json">>,{...},...},
[2023-06-30T16:40:36.458Z] [range3,range4],
[2023-06-30T16:40:36.458Z] 1}]}]}
[2023-06-30T16:40:36.458Z] output:<<"">>
```
> One of the key properties of mappings is that they do not lose elements
We were not losing elements here either, they were pre-pended to the accumulator list. The only visible difference is that we ended up with a different order of elements.
>> I think we'd need to fix the tests as some assume a particular results order. We could potentially generalize those by sorted the results before comparing?
> With a mapping, this is not required, I believe. It should just maintain the original ordering as it walks the list and applies the function on each element.
I was trying to point out that the output order in this particular function doesn't matter as much. We always run a custom sort on the result and pick out the head only. But since that's more about how the result is used, and not the function itself the best way is to just update all the unit tests to pass.
--
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
[GitHub] [couchdb] pgj commented on pull request #4657: chore(`mango`): replace `foldl` with a `map`
Posted by "pgj (via GitHub)" <gi...@apache.org>.
pgj commented on PR #4657:
URL: https://github.com/apache/couchdb/pull/4657#issuecomment-1615235540
Thanks @nickva !
--
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
[GitHub] [couchdb] pgj commented on pull request #4657: chore(`mango`): replace `foldl` with a `map`
Posted by "pgj (via GitHub)" <gi...@apache.org>.
pgj commented on PR #4657:
URL: https://github.com/apache/couchdb/pull/4657#issuecomment-1615169763
> I think we'd need to fix the tests as some assume a particular results order. We could potentially generalize those by sorted the results before comparing?
With a mapping, this is not required, I believe. It should just maintain the original ordering as it walks the list and applies the function on each element.
--
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
[GitHub] [couchdb] pgj commented on pull request #4657: chore(`mango`): replace `foldl` with a `map`
Posted by "pgj (via GitHub)" <gi...@apache.org>.
pgj commented on PR #4657:
URL: https://github.com/apache/couchdb/pull/4657#issuecomment-1615194313
> The test is failing because the order of the elements changes. So we should at least fix that part.
Sorry, I was answering the part about the statement that prepending with fold left reverses the order. You are right that the ordering does not matter in this case. But replacing this with a `map` breaks the unit tests. I did not notice that one, but I have pushed a fix for that since then.
> We were not losing elements here either, they were pre-pended to the accumulator list. The only visible difference is that we ended up with a different order of elements.
Ah, my concern is not the final result but the readability mainly. It acts like a map, why not to use the `map` function here? This is made evident. I believe such minor details could contribute to the overall understanding of the code.
--
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
[GitHub] [couchdb] pgj commented on pull request #4657: chore(`mango`): replace `foldl` with a `map`
Posted by "pgj (via GitHub)" <gi...@apache.org>.
pgj commented on PR #4657:
URL: https://github.com/apache/couchdb/pull/4657#issuecomment-1615162895
This is not about ordering of elements. One of the key properties of mappings is that they do not lose elements (assuming that the function used for mapping is total, i.e. it always produces an answer). The folding-based approach does not exclude this possibility. For example, it can turn the elements of the list into any other data structure, even some scalar, or exclude elements from the input. It may take a while for the reader to see that it is just about extending the indexes with their prefixes and prefix differences on the field ranges.
--
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
[GitHub] [couchdb] nickva merged pull request #4657: chore(`mango`): replace `foldl` with a `map`
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva merged PR #4657:
URL: https://github.com/apache/couchdb/pull/4657
--
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