Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new lists:mapkeyfind/3 function to map finding #4831

Closed
wants to merge 1 commit into from

Conversation

paulozulato
Copy link

Stdlib's lists module has some functions for searching a list for a
tuple whose given element position compares equal to a given value, but
it lacks the same functionality for maps, i.e., a function for searching
a list for a map whose given key compares equal to the given value.

This new function allow us to find such a map in a list without the need
to create a fun to filter it every time a map search is required.

Usage:

1> lists:mapkeyfind(42, id, [#{id => 1, person => jane},
                             #{id => 42, answer => 42}]).
#{id => 42, answer => 42}

2> lists:mapkeyfind(not_found, id, [#{id => 1, person => jane},
                                    #{id => 42, answer => 42}]).
false

3> lists:mapkeyfind(not_found, id, []).
false

Stdlib's lists module has some functions for searching a list for a
tuple whose given element position compares equal to a given value, but
it lacks the same functionality for maps, i.e., a function for searching
a list for a map whose given key compares equal to the given value.

This new function allow us to find such a map in a list without the need
to create a fun to filter it every time a map search is required.

Usage:

1> lists:mapkeyfind(42, id, [#{id => 1, person => jane},
                             #{id => 42, answer => 42}]).
#{id => 42, answer => 42}

2> lists:mapkeyfind(not_found, id, [#{id => 1, person => jane},
                                    #{id => 42, answer => 42}]).
false

3> lists:mapkeyfind(not_found, id, []).
false
@CLAassistant
Copy link

CLAassistant commented May 14, 2021

CLA assistant check
All committers have signed the CLA.

@rickard-green rickard-green added the team:VM Assigned to OTP team VM label May 15, 2021
@garazdawi
Copy link
Contributor

A while after maps were added there was a pretty long discussion on the erlang-questions mailing list about adding this function, together with all the other variants of lists:key*. At the time the only thing that was decided was that having fun variants of all the lists:key* was a good thing. That is you can write your code as:

mapfind(Key, Value, ListOfMaps) ->
  lists:search(fun(M) -> try maps:get(Key, M) == Value catch _:_ -> false end, ListOfMaps).

or something similar. This quite a lot more verbose than what you are proposing, so I personally do not have a problem with adding functions to handle lists of maps.

For me the largest and hardest question for this is naming it correctly as we have multiple options:

  1. Should we have a separate module from lists, and if so what should it be called? listmaps, lofm (list of maps), maps?
  2. lists:mapfind?
  3. Should we make lists:keyfind accept a mix? (probably not).
  4. Unknown possibly better alternative 4.

We also should consider how this fits into the greater scheme of things, such as #2952 or a more general lens library for erlang.

This was just my way of saying that although the addition of this function is simple, it may take some thinking to know that this is the solution that we want.

@paulozulato
Copy link
Author

Hi, @garazdawi,

I can understand your point. I agree we can use fun variants to achieve the goal, but it's also true for the lists:key*.
So, my rationale was in this way: if we have such facilitators for tuples/records, why not to have them also for maps? After all, as Joe Armstrong once said, "Records are dead - long live maps !" (https://joearms.github.io/published/2014-02-01-big-changes-to-erlang.html). In short, I believe lists really should have some map-related alternatives to the lists:key* once it's being widely adopted.

About your question on naming scheme, I believe we should keep it in the same lists module once it's a lookup in a generic list, not a strict list of only maps. Also, I think maps module handles actions more related to maps itself, not collections of maps, so I wouldn't add these function there too. Furthermore, I agree with you that it's not a good idea to make lists:keyfind accept a mix once it would be a little messy. Finally, I suggested mapkeyfind to follow the lists:keyfind naming scheme and parameters (only changing element position to key) and to make it explicit that it's a map version of the lists:keyfind.

A while after maps were added there was a pretty long discussion on the erlang-questions mailing list about adding this function

I've started learning Erlang two years ago, so I'm not completely aware of this discussion. Does someone proposed it again recently or this discussion occurred only that time? Should we discuss it again or I should close this pull request as duplicated?

Thank you very much.

@zxq9
Copy link

zxq9 commented May 18, 2021

+1
These functions are completely unambiguous, the argument order is the same, etc. (meaning, it is a very different issue than trying to argue for inclusion of a JSON encoder/decoder in the stdlib). I have a one-module project that does exactly this ("Lists of Maps": https://gitlab.com/zxq9/lom). It makes sense to have "lists of maps" functions in the stdlib instead of having to be either an extra dependency or ad hoc stuff each different project has to decide how to do. The output of maps-based JSON decoders like zj are very often lists of maps, and I imagine this is a very common use case for people who handle any web data.

@asabil
Copy link
Contributor

asabil commented May 18, 2021

May I suggest another alternative (4?):

-spec mapfind(map(), list()) -> {ok, map()} | error

This function would returns the first element that is a superset of the map given as parameter. I haven't given much thought to the exact function naming or in which module should such a function reside, I just think that a match based interface would probably be more appropriate for a list of maps. Any thoughts?

@garazdawi
Copy link
Contributor

So, my rationale was in this way: if we have such facilitators for tuples/records, why not to have them also for maps?

In my current opinion adding tuple support to the lists module was a mistake and that it should instead have been a separate module such as tuplelist. So for me the question boils down to, should we put the function in lists for consistency with the way tuples were done, or should we try to make something better and create a mapslist module?

I've started learning Erlang two years ago, so I'm not completely aware of this discussion. Does someone proposed it again recently or this discussion occurred only that time?

It has been discussed multiple times in different forums, though I don't think anyone yet has submitted a PR.

Should we discuss it again or I should close this pull request as duplicated?

You can leave it open and I'll schedule a meeting of the OTP technical board to see what opinion we have about this.

@zxq9
Copy link

zxq9 commented May 19, 2021

In my current opinion adding tuple support to the lists module was a mistake and that it should instead have been a separate module such as tuplelist.

Being so records-oriented it would make sense to have made a new module called lor (Lists of Records) or lot (Lists of Tuples).
The lists module is pretty huge already. I'm not opposed to huge modules, but it does make it a bit intimidating for newcomers to see a module with a gajillion functions in it, some of them being listy staples of functional programming, and others really specialized like the lists:key*/N family.

In my own little version of this here https://gitlab.com/zxq9/lom/-/blob/master/src/lom.erl there has arisen the need in one project that uses it to provide a second set of functions that permits a key to not exist at all (so maps:get/2 would raise an exception, which is too strict for that case). This can't happen with records, of course, because you either have the element or it isn't defined as the same type of record, but in sparse JSON data this can come up and needs to be handled. I'm thinking about expanding the return type from false | Result to error | false | Result to cover both cases and allow the caller to not match error in the case they want to crash on what would be considered bad data in a strictly defined schema.

Anyway, my point in mentioning this is there are a few weird little corners with this based on the difference between records and maps, and how you decide to deal with them can mean doubling the number of functions added to the module if it were to be included in lists directly.

@zuiderkwast
Copy link
Contributor

If we anticipate more functions in the list of maps family, a new module would be preferable IMHO. (Craig's lib has 10 functions, but you can imagine filter, partition and more.) I suggest the name maplists, a name similar to proplists.

Regarding @asabil's mapfind/3, it's interesting, but what exactly does "subset" mean here? I suppose it can match on multiple keys (as in mapfind(#{fruit => banana, angle => 45}, BentFruits)), not keys and values being subsets of other keys and values, in terms of nested maps or sets, right? That could become quite complex... It would be better to write a recursive function matching on the each map using map patterns.

@asabil
Copy link
Contributor

asabil commented May 20, 2021

Regarding @asabil's mapfind/3, it's interesting, but what exactly does "subset" mean here? I suppose it can match on multiple keys (as in mapfind(#{fruit => banana, angle => 45}, BentFruits)), not keys and values being subsets of other keys and values, in terms of nested maps or sets, right? That could become quite complex... It would be better to write a recursive function matching on the each map using map patterns.

@zuiderkwast I meant it exactly as you describe it:

Fruits = [
    #{fruit => banana, color => yellow},
    #{fruit => banana, angle => 45, color => yellow},
    #{fruit => {apple}, color => red}
].

mapfind(#{fruit => banana, angle => 45}, Fruits).
{ok, #{fruit => banana, angle => 45, color => yellow}}

mapfind(#{fruit => banana}, Fruits).
{ok, #{fruit => banana, color => yellow}}

mapfind(#{fruit => apple, color=> red}, Fruits).
error

It is basically a generalisation of keyfind to allow matching multiple keys at the same time.

@garazdawi garazdawi self-assigned this May 24, 2021
@garazdawi
Copy link
Contributor

Hello!

We have just completed our OTP technical board meeting. We want to have this type of functionality in stdlib, but we do not want to have it in the lists module. So, therefore, we have decided to close this PR.

Instead what we would like to see is a new module similar (or identical) to the lom module linked by @zxq9. This would have to be accompanied by an EEP. We would be very happy to work with someone from the community on this if anyone is up to it. The hard part will be to come up with a good name for this. I like lom, but it is not very descriptive. So maybe maplists is a better option.

In the long run, we do intend to start looking at something like functional lenses which could also be used for this type of functionality, but that far away so adding this module first is a step in the right direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:VM Assigned to OTP team VM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants