Method take for containers #1209

BorisBochkaryov · 2016-10-20T13:23:59Z

Preparation of the desired item from the container and its removal in a single pass.

psyeugenic · 2016-10-20T13:35:06Z

New features should be based on master-branch.
maps:take/2 is already implemented. It's tested in erts/emulator/test since it's a BIF.
Don't merge maint into your branch!

legoscia · 2016-10-20T16:13:46Z

lib/stdlib/doc/src/gb_sets.xml


    <func>
      <name name="delete_any" arity="2"/>
+<<<<<<< HEAD


There is a merge conflict here.

legoscia · 2016-10-20T16:14:00Z

lib/stdlib/doc/src/gb_sets.xml

  </funcs>

  <section>
+<<<<<<< HEAD


...and here

legoscia · 2016-10-20T16:14:09Z

lib/stdlib/doc/src/gb_trees.xml

  </funcs>

  <section>
+<<<<<<< HEAD


...and here

OTP-Maintainer · 2016-10-20T18:00:05Z

The summary line of the commit message is too long and/or ends with a "."
Make sure the whole message follows the guidelines here: https://github.com/erlang/otp/wiki/Writing-good-commit-messages.

Bad message: Method take and documentation with tests for this method in dict
Method take and documentation with tests for this method in sets

I am a script, I am not human

OTP-Maintainer · 2016-10-22T18:29:15Z

Patch has passed first testings and has been assigned to be reviewed

I am a script, I am not human

bjorng · 2016-10-24T13:27:27Z

We like the general idea of adding take/2 functions to each set and map module.

However, we have some comments about the API and on the implementation itself.

First about the API. We think that the 'ok' atom should be removed from returned tuple. That is:

dict:take(Key, Dict) -> {El,Dict'} | error
gb_trees:take(Key, Tree) -> {El,Tree'} | CRASH
gb_trees:take_any(Key, Tree) -> {El,Tree'} | error

gb_sets:take(Key, Set) -> {El,Set'} | CRASH
gb_sets:take_any(Key, Set) -> {El,Set'} | error
ordsets:take(Key, Set) -> {El,Set'} | error
sets:take(Key, Set) -> {El,Set'} | error

Also, I think that you have forgotten orddict:

orddict:take(Key, Dict) -> {El,Dict'} | error

bjorng · 2016-10-24T13:35:38Z

Here are some other comments.

Please read https://github.com/erlang/otp/wiki/Writing-good-commit-messages. When rewriting the commit message, change "method" to "function".

You can combine all changes to one commit. That will also make it easier to write a good commit message.

Please take out the unrelated changes to whitespace. If you are doing cleanups of whitespace changes, please make them in a separate commit.

I will add some more comments to the code itself.

bjorng · 2016-10-24T14:02:01Z

Thinking a little bit more of the API, the set functions don't seem very useful:

gb_sets:take(El, Set) -> {El,Set'} | CRASH
gb_sets:take_any(El, Set) -> {El,Set'} | error
ordsets:take(El, Set) -> {El,Set'} | error
sets:take(El, Set) -> {El,Set'} | error

Are those functions really useful?

If you have any good real-world use cases, please write about it in the commit. Otherwise I think that you should remove the set functions from the commit.

bjorng · 2016-10-31T13:16:36Z

Thanks for changes. But it is still not clear why the functions take functions in the sets module are useful. Why do you need them?

platinumthinker · 2016-11-01T06:53:32Z

Sets useful when you need remove Item from set and you won't know, is an element of Set or not.
sets:del_element return new sets always. Then we have to use search element and delete element separately.

bjorng · 2016-11-02T13:56:13Z

Sets useful when you need remove Item from set and you won't know, is an element of Set or not.
sets:del_element return new sets always. Then we have to use search element and delete element separately.

Yes, I realize that. What I want to know is in what real world usage that is useful. Have you encountered that need in practice or can you point to an algorithm where it would be useful? Before adding new functions that may never be used in practice, we would like to see one real world example where take would be useful for sets.

bjorng · 2016-11-02T14:00:07Z

Another thing. I looked at the description of Set in the Haskell library:

https://downloads.haskell.org/~ghc/6.12.3/docs/html/libraries/containers-0.3.0.0/Data-Set.html

I could not find any function that seems to do the same thing as the suggested take function.

OTP-Maintainer · 2016-11-02T19:07:37Z

Patch has passed first testings and has been assigned to be reviewed

I am a script, I am not human

hurd54 · 2016-11-03T03:35:16Z

Of course, some_sets:take/2 can be expressed through is_element/2 and del_element/2,
however some_sets:take/2 will allow to carry out this operation more efficiently.

A quick search found a few places in our internal projects, and a couple of places in the Erlang repository:

observer/src/observer_pro_wx.erl
compiler/v3_core.erl

So the real needs some_sets:take/2 is present.

Along the way, I will give an example of how the weak api enforce write not optimal code:
In the Erlang repository, I found dozens of places where used a bunch of: is_element+add_element like:

case some_sets:is_element/2 of
  true -> something_do();
  false -> some_sets:add_element/2, something_do2()
end.

that makes double-run on the sets, instead, that would make solution in a single pass.
More effective it would be to use a type of construction(similar to gb_trees:insert):

try
  some_sets:insert_element/2
  do2()
catch {error,{key_exists,_}} ->
  do()
end.

But some_sets:insert_element/2 is not yet available :(

So, lets start making a reach API.

bjorng · 2016-11-04T11:27:23Z

I agree that there are ome rough edges in the set APIs, for examples that some functions only exist in some of the sets modules. There are probably also some useful functions in libraries for Haskell or ML that we don't have. We plan to look over the sets APIs in OTP 20 and eliminate some of those rough edges.

Regarding this pull request, we are still unconvinced that a take/2 function is sufficiently useful to be worth adding to the sets modules and we are also not sure about the return value. We still find take/2 useful for the dictionary modules (and its already implemented for maps). Therefore, please take out take/2 from the sets modules in this pull request if you want give this pull request a chance to be included in Erlang/OTP.

bjorng · 2016-11-14T12:12:16Z

dict:take/2 and orddict:take/2 returns none when they fail, but all other functions in those modules return error to indicate a failure. For consistency, I think that they should return error. (gb_trees:take/2 should still return none, because it's consistent within gb_trees.)

Also, for symmetry, I think you should add maps:take/3.

platinumthinker · 2016-11-14T12:57:09Z

maps:take/2 already exists.
than the function to be different take/3 on the take/2?

bjorng · 2016-11-14T13:08:58Z

maps:take/2 already exists.

Yes, I know.

than the function to be different take/3 on the take/2?

Sorry, I don't understand.

platinumthinker · 2016-11-14T13:54:06Z

@bjorng What is the difference between take/2 and take/3?

psyeugenic · 2016-11-14T17:20:25Z

You should have symmetry in these APIs.

dict:take/2 should return error if Key is not found. none is not used in the dict API, see dict:find/2. This would also be consistent with maps:take/2

-spec take(Key, Dict) -> {Value, Dict1} | 'error' when
      Dict :: dict(Key, Value),
      Dict1 :: dict(Key, Value),
      Key :: term(),
      Value :: term().

orddict:take/2 should return error if Key is not found. none is not used in the orddict API, see orddict:find/2. This would also be consistent with maps:take/2

-spec take(Key, Orddict) -> {Value, Orddict1} | 'error' when
      Orddict :: orddict(Key, Value),
      Orddict1 :: orddict(Key, Value),
      Key :: term(),
      Value :: term().

Is there sufficient motivation for a take/3 implementation? I'm not sure there is.

If take/3 should be implemented then it should also be implemented for maps, i.e. returning a default if the search Key is not found. It would be sufficient to implement this in Erlang (in maps.erl).

Something along the lines of ..

-spec take(Key,Map1,Default) -> {Value,Map2} when
    Key :: term(),
    Map1 :: map(),
    Value :: term(),
    Map2 :: map(),
    Default :: term().

take(Key,Map1,Default) ->
    case maps:take(Key,Map1) of
        error -> {Default,Map1};
        Res -> Res
    end.

But again, I'm not convinced that dict:take/3, orddict:take/3, gb_trees:take/3 nor maps:take/3 is needed. Is there a use case? I can see it leading to doubtful usage. I'm on the fence.

BorisBochkaryov · 2016-11-19T13:25:26Z

@psyeugenic We changed return values.
For dict:take/2:

-spec take(Key, Dict) -> {Value, Dict1} | error when
       Dict :: dict(Key, Value),
       Dict :: dict(Key, Value),
       Dict1 :: dict(Key, Value),
       Dict1 :: dict(Key, Value),
       Key :: term(),
       Key :: term(),

This method is useful in places where it is necessary to find the value for key and remove this key from the dictionary.
Section of code:

case dict:find(Key,Dict) of
    {ok, Val} -> {Val, dict:erase(Key,Dict)};
    error -> error
end.

will be replaced by:
dict:take(Key,Dict)
It will also increase the efficiency in time due to the lack of double pass on dictionary. As @thehurd mentioned above.

For dict:take/3:

-spec take(Key, Dict, Default) -> {Value, Dict1} | Default when
      Dict :: dict(Key, Value),
      Dict1 :: dict(Key, Value),
      Key :: term(),
      Value :: term(),
      Default :: term().

Just 'Default'. Using argument 'Default' in function allow do everything your needs in case of an error.
Example:
dict:take(Key,Dict,begin log:error("Error in ...",[]), default end)

log:error/2 - function from chronica (Logger framework)

This code will make an entry in the log and return to the default value.
Similar examples is also directed to the orddict.

platinumthinker · 2016-11-22T13:47:55Z

@psyeugenic ping

platinumthinker · 2016-11-24T05:44:12Z

@bjorng ping

gootik · 2016-11-29T12:00:10Z

lib/stdlib/src/dict.erl

+      Key :: term(),
+      Value :: term().
+
+take(Key, D0) ->


this can be changed to

take(Key, D0) -> take(Key, D0, error).

I has made the recommended changes.

platinumthinker · 2016-12-15T02:12:10Z

This PR waiting R20 or our changes?

bjorng · 2016-12-15T06:49:21Z

We are waiting for your changes and answers to our questions.

To summarize what we have said before:

We like dict:take/2, orddict:take/2, gb_trees:take/2, and maps:take/2, and understand why they are useful.

We are not sure that dict:take/2, orddict:take/2, gb_trees:take/2 are useful enough to be included. But if they are to be included, why no maps:take/3? That is why we have not done any more.

platinumthinker · 2016-12-15T07:41:10Z

@bjorng I understand correctly, for PR merged you wanted maps:take/3?

bjorng · 2016-12-15T08:01:21Z

Yes.

…trees

BorisBochkaryov · 2016-12-16T07:39:27Z

@bjorng We removed dict:take/3, orddict:take/3, gb_trees:take/3 and added maps:take/3.

Similar to maps:take/2, add take/2 to the other dictionary modules in STDLIB: orddict:take(Key, Dict) -> {Val,NewDict} | 'error'. dict:take(Key, Dict) -> {Val,NewDict} | 'error'. gb_trees:take(Key, Dict) -> {Val,NewDict}. For gb_trees also add: gb_trees:take_any(Key, Dict) -> {Val,NewDict} | 'error'. gb_trees already has delete() and delete_any(), so we will follow that design pattern. Suggested by Boris Bochkaryov in erlang#1209.

bjorng · 2016-12-16T15:10:06Z

I will close this pull request now, since it seems we are not understanding each other very well. Not sure if I have been clear enough in my requests for changes.

Since we like the general idea of take/2, we have decided to implement it ourselves. See #1290.

Similar to maps:take/2, add take/2 to the other dictionary modules in STDLIB: orddict:take(Key, Dict) -> {Val,NewDict} | 'error'. dict:take(Key, Dict) -> {Val,NewDict} | 'error'. gb_trees:take(Key, Dict) -> {Val,NewDict}. For gb_trees also add: gb_trees:take_any(Key, Dict) -> {Val,NewDict} | 'error'. gb_trees already has delete() and delete_any(), so we will follow that design pattern. Suggested by Boris Bochkaryov in erlang#1209.

psyeugenic added the team:MW label Oct 20, 2016

BorisBochkaryov force-pushed the method_take_for_containers branch from c71adbc to 4a115ab Compare October 20, 2016 15:24

BorisBochkaryov changed the base branch from maint to master October 20, 2016 15:26

legoscia reviewed Oct 20, 2016

View reviewed changes

lib/stdlib/doc/src/gb_sets.xml Outdated

</funcs>

<section>

<<<<<<< HEAD

Copy link

Contributor

legoscia Oct 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and here

legoscia reviewed Oct 20, 2016

View reviewed changes

lib/stdlib/doc/src/gb_trees.xml Outdated

</funcs>

<section>

<<<<<<< HEAD

Copy link

Contributor

legoscia Oct 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and here

psyeugenic added the feature label Oct 20, 2016

BorisBochkaryov force-pushed the method_take_for_containers branch from 4a115ab to 820f73c Compare October 20, 2016 16:53

BorisBochkaryov force-pushed the method_take_for_containers branch from 820f73c to fe065bb Compare October 21, 2016 00:23

bjorng self-assigned this Oct 24, 2016

bjorng added the waiting waiting for changes/input from author label Oct 26, 2016

BorisBochkaryov force-pushed the method_take_for_containers branch 2 times, most recently from 5f4a3fb to d7984b2 Compare October 31, 2016 12:06

BorisBochkaryov force-pushed the method_take_for_containers branch 2 times, most recently from 71e35e8 to d99e374 Compare November 7, 2016 02:23

BorisBochkaryov added 2 commits November 10, 2016 13:26

Function take for gb_trees

3225fd7

Function take for dict

c1e3697

bjorng assigned psyeugenic Nov 14, 2016

BorisBochkaryov force-pushed the method_take_for_containers branch 3 times, most recently from 15694ea to dafee61 Compare November 19, 2016 12:06

BorisBochkaryov force-pushed the method_take_for_containers branch from dafee61 to d184d5d Compare November 19, 2016 13:29

gootik reviewed Nov 29, 2016

View reviewed changes

BorisBochkaryov added 2 commits December 1, 2016 11:49

Change function take for containers

76b88a2

Fix conflict in gb_trees

b8a1bfe

BorisBochkaryov force-pushed the method_take_for_containers branch from 0093608 to b8a1bfe Compare December 1, 2016 05:12

psyeugenic removed their assignment Dec 1, 2016

Function take/3 for maps and remove take/3 from dict, orddict and gb_…

bd7dbd6

…trees

bjorng mentioned this pull request Dec 16, 2016

Add take/2 to all dictionary modules #1290

Merged

bjorng closed this Dec 16, 2016

BorisBochkaryov deleted the method_take_for_containers branch December 17, 2016 06:28

Method take for containers #1209

Method take for containers #1209

Uh oh!

Conversation

BorisBochkaryov commented Oct 20, 2016

Uh oh!

psyeugenic commented Oct 20, 2016

Uh oh!

legoscia Oct 20, 2016

Choose a reason for hiding this comment

Uh oh!

legoscia Oct 20, 2016

Choose a reason for hiding this comment

Uh oh!

legoscia Oct 20, 2016

Choose a reason for hiding this comment

Uh oh!

OTP-Maintainer commented Oct 20, 2016

Uh oh!

OTP-Maintainer commented Oct 22, 2016

Uh oh!

bjorng commented Oct 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjorng commented Oct 24, 2016

Uh oh!

bjorng commented Oct 24, 2016

Uh oh!

bjorng commented Oct 31, 2016

Uh oh!

platinumthinker commented Nov 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjorng commented Nov 2, 2016

Uh oh!

bjorng commented Nov 2, 2016

Uh oh!

OTP-Maintainer commented Nov 2, 2016

Uh oh!

hurd54 commented Nov 3, 2016

Uh oh!

bjorng commented Nov 4, 2016

Uh oh!

bjorng commented Nov 14, 2016

Uh oh!

platinumthinker commented Nov 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjorng commented Nov 14, 2016

Uh oh!

platinumthinker commented Nov 14, 2016

Uh oh!

psyeugenic commented Nov 14, 2016

Uh oh!

BorisBochkaryov commented Nov 19, 2016

Uh oh!

platinumthinker commented Nov 22, 2016

Uh oh!

platinumthinker commented Nov 24, 2016

Uh oh!

gootik Nov 29, 2016

Choose a reason for hiding this comment

Uh oh!

BorisBochkaryov Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

platinumthinker commented Dec 15, 2016

Uh oh!

bjorng commented Dec 15, 2016

Uh oh!

platinumthinker commented Dec 15, 2016

Uh oh!

bjorng commented Dec 15, 2016

Uh oh!

BorisBochkaryov commented Dec 16, 2016

Uh oh!

bjorng commented Dec 16, 2016

Uh oh!

Reviewers

Assignees

bjorng commented Oct 24, 2016 •

edited

Loading

platinumthinker commented Nov 1, 2016 •

edited

Loading

platinumthinker commented Nov 14, 2016 •

edited

Loading