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

Group by key and fold each group simultaneously #23

Closed
ilya-g opened this issue Jun 7, 2016 · 10 comments
Closed

Group by key and fold each group simultaneously #23

ilya-g opened this issue Jun 7, 2016 · 10 comments
Assignees
Labels

Comments

@ilya-g
Copy link
Member

ilya-g commented Jun 7, 2016

Discussions about the Operation to group by key and fold each group simultaneously will be held here.

@ilya-g ilya-g added the stdlib label Jun 7, 2016
@ilya-g ilya-g self-assigned this Jun 7, 2016
@mikehearn
Copy link

I'm understanding from the 'alternatives' section that the motivation for this is performance. It'd be nice if there was a motivation section at the top of the KEEP template to make the reasoning for introducing a new feature clearer.

The reason I say it must be performance is that the only 'con' listed against groupBy { }.mapValues { } is the construction of a separate list.

Is the assumption that implementing a form of stream fusion in the compiler would break code that relies on side effects? I mean, just recognising the groupBy {}.mapValues {} construct and swapping in the group-and-fold implementation on the fly, rather than adding it as a public method in the standard library.

@ilya-g
Copy link
Member Author

ilya-g commented Jun 9, 2016

@mikehearn Yes, performance (namely the allocations) is the reason why groupBy {}.mapValues {} isn't a good alternative.

I mean, just recognising the groupBy {}.mapValues {} construct and swapping in the group-and-fold implementation on the fly

Not every operation available on List could be easily rewritten as fold.

@ilya-g
Copy link
Member Author

ilya-g commented Jul 4, 2016

TODO: Experiment with fluent API

@aembleton
Copy link

Rather than creating a Grouping interface, can't you return a Map<T, List<K>>? This would allow for any operations that can be carried out on a Map, be carried out on the results from this.

@ilya-g
Copy link
Member Author

ilya-g commented Jul 14, 2016

@aembleton

can't you return a Map<T, List<K>>

Isn't it what groupBy() returns? The goal is to avoid creating intermediate map of lists.

@ilya-g
Copy link
Member Author

ilya-g commented Sep 16, 2016

keySelector isn't suitable name for member function of Grouping. Considering other naming options:

  • keyOf(element) — get key of element (but can also mean create key of elements, like listOf)
  • keyFrom(element) — extract key from element
  • keyFor(element) — looks like we're trying to brute-force pick a key for element
  • getKey(element) — like an indexed property getter

Quite more verbose:

  • applyKeySelector(element) — reflecting keySelector parameter name from groupingBy
  • elementKey(element)

@voddan
Copy link
Contributor

voddan commented Sep 17, 2016

Name groupingBy is confusing because it clashes with groupingBy from Java8. Also it is too close to groupBy IMO.

I propose naming it toGroupingBy. I think it is suitable because this is the one function in stdlib which returns a new type Grouping instead of a collection.

@ilya-g
Copy link
Member Author

ilya-g commented Sep 19, 2016

groupingBy can clash with Collectors.groupingBy but the conditions under which it is observed are rather exotic:

  • Collectors.groupingBy must be imported statically, IDEA doesn't add such static imports by default
  • there must be an implicit receiver suitable for groupingBy like Iterable, Sequence or Array.
import java.util.stream.Collectors.groupingBy

    val list = listOf("ab", "cde", "x")
    with (list) {
        stream().collect(groupingBy<String, Int> { it.length })
                     //  ^^^^^^^ required Collector, found Grouping
    }

@ilya-g
Copy link
Member Author

ilya-g commented Nov 25, 2016

We consider such situation, when the extension groupingBy could clash with Collectors.groupingBy, to be quite rare, and there's a workaround to disambiguate it both ways: either add explicit Collectors or explicit this before groupingBy.

@dzharkov
Copy link
Contributor

dzharkov commented Feb 14, 2018

The declarations have been released in Kotlin 1.1, thus I'm closing the KEEP. Do not hesitate to open a YouTrack issue for any additional suggestions.

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

No branches or pull requests

5 participants