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

proposal: x/exp/maps: add Merge #57191

Closed
louisportay opened this issue Dec 9, 2022 · 8 comments
Closed

proposal: x/exp/maps: add Merge #57191

louisportay opened this issue Dec 9, 2022 · 8 comments

Comments

@louisportay
Copy link
Contributor

Prior to my PR, I didn't see that this feature was already discussed here.

Even though the change was retracted in favor of maps.Copy, I believe that for a large number of maps to merge, the semantics of a Merge function are more meaningful.

The implementation I've proposed has no mechanism for key conflicts resolution.
But this mechanism (if implemented) could possibly justify this function existence alongside maps.Copy.

@gopherbot gopherbot added this to the Proposal milestone Dec 9, 2022
@seankhliao
Copy link
Member

what's the proposed API?
and do you have numbers to back up your claim?

@louisportay
Copy link
Contributor Author

louisportay commented Dec 9, 2022

Here's the proposed API
func Merge[M ~map[K]V, K comparable, V any](m ...M) M

What I'm saying is

m := maps.Merge(m1,m2,m3,m4,m5)





is more explicit IMO than









m := maps.Clone(m1)
maps.Copy(m, m2)
maps.Copy(m, m3)
maps.Copy(m, m4)
maps.Copy(m, m5)


The other benefit of this function is the ability to allocate one time to hold every values in a single map.

@rsc
Copy link
Contributor

rsc commented Mar 15, 2023

I don't know: the maps.Clone+maps.Copy implementation seems more explicit to me. There is no ambiguity about which maps override which other maps.

I don't understand the comment about the benefit of "the ability to allocate one time". Perhaps this is talking about precalculating the size of the map and preallocating, but that doesn't end up working that well for maps, and it's difficult to predict the amount of overlap anyway. If m1 through m5 contain mostly the same keys, you need max(len(mX)) for the result. If they contain mostly different keys, you need sum(len(mX)) for the result. Those are very different preallocated sizes.

@rsc
Copy link
Contributor

rsc commented Mar 29, 2023

This also doesn't seem like a very common operation. How important is this? How often does it come up?

@rsc
Copy link
Contributor

rsc commented Mar 29, 2023

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Apr 6, 2023

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@louisportay
Copy link
Contributor Author

Sorry, I forgot to answer !

It is quite right for the rule about the overrides.
For the allocation part, I think that most users would not want to discard values and will use this function to merge Set values. Yet, I can't predict how this will be used.

It is probably not frequent enough to justify its use instead of using a loop with maps.Copy.
(I've never massively sampled code to identify this pattern)

@rsc
Copy link
Contributor

rsc commented Apr 12, 2023

No change in consensus, so declined.
— rsc for the proposal review group

@rsc rsc closed this as completed Apr 12, 2023
@golang golang locked and limited conversation to collaborators Apr 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants