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 IterableOps.groupFlatMap method #135

Open
ashleymercer opened this issue Oct 17, 2022 · 2 comments · May be fixed by #136
Open

Add IterableOps.groupFlatMap method #135

ashleymercer opened this issue Oct 17, 2022 · 2 comments · May be fixed by #136

Comments

@ashleymercer
Copy link

ashleymercer commented Oct 17, 2022

We already have IterableOps.groupMap and groupMapReduce methods, does it make sense to add a groupFlatMap method as well?

In the simplest case this could just be an alias for groupMapReduce(f)(g)(_ ++ _) but if implemented separately it can be more efficient:

  • we can avoid an extra function call to the reducer "wrapper" function for each element
  • we can use a mutable buffer internally to build up the list of values

Indeed groupMapReduce already states in its doc that it exists (solely?) because it's more efficient than rolling the behaviour by hand using groupBy(f).map(...). It also somehow "feels" like this is missing from the API since we generally expect map and flatMap to come as a pair.

@SethTisue SethTisue transferred this issue from scala/bug Oct 17, 2022
@BalmungSan
Copy link
Contributor

It could be added as part of my proposal here: #53

@ashleymercer ashleymercer changed the title Add Iterable.groupFlatMap method Add IterableOps.groupFlatMap method Oct 17, 2022
@ashleymercer
Copy link
Author

ashleymercer commented Oct 17, 2022

@BalmungSan looks like there's some overlap of ideas here, but your proposal is more complex since you're changing the target type of collection. I would expect groupFlatMap to be much simpler - I already have a draft implementation which I'm using locally - so hopefully much easier to get it accepted / merged.

For example, I'd expect to keep the existing behaviour of flatMap that you end up with the source type of collection, for example:

val as = Seq(List(1), List(2))
val bs = as.flatMap(identity)

The resulting type is bs: Seq[Int] rather than bs: List[Int], so I'd expect groupFlatMap to behave similarly (and obviously changing this for regular flatMap would be a whole separate discussion).

Of course, at some point you could propose adding groupFlatMapTo as well :)

(Also updated issue title and description to reflect the fact that this should really live on IterableOps, not on Iterable)

@ashleymercer ashleymercer linked a pull request Oct 22, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants