group-and-fold implementation #1004

ilya-g · 2016-12-27T20:43:27Z

An implementation of Kotlin/KEEP#23 proposal

ilya-g · 2016-12-28T05:13:55Z

TODO

add SinceKotlin
add headers for platform specialized functions.

Docs for group-and-fold operations Fix implementation for countEach and sumEachBy countEach and sumEachBy implementations for JS Rename keySelector to keyOf Generate additional sources for groupingBy + headers Rename countEach and sumEachBy to eachCount and eachSumOf.

cy6erGn0m · 2016-12-29T12:11:45Z

libraries/stdlib/src/kotlin/collections/Grouping.kt

+@SinceKotlin("1.1")
+public interface Grouping<T, out K> {
+    /** Returns an [Iterator] which iterates through the elements of the source. */
+    fun elementIterator(): Iterator<T>


@{18f15b45-4413-42a3-82e7-d057983d1e65,Ilya Gorbunov} sourceIterator() or just source() ? Need to be documented: whether it creates iterator every time or keep the same instance. If the behaviour is undefined we have to notice it here.

I believe elementIterator being a fun conveys the intent well. If that was the same instance every time, it would be a property.

It was called source in the beginning, that I renamed it to iterator and then to elementIterator

After some consideration, sourceIterator seems to look better.

cy6erGn0m · 2016-12-29T12:15:55Z

libraries/stdlib/src/kotlin/collections/Grouping.kt

+public inline fun <T, K, R> Grouping<T, K>.aggregate(
+        operation: (key: K, value: R?, element: T, first: Boolean) -> R
+): Map<K, R> {
+    val result = mutableMapOf<K, R>()


Why don't we use aggregateTo instead?

cy6erGn0m · 2016-12-29T12:16:41Z

libraries/stdlib/src/kotlin/collections/Grouping.kt

+@SinceKotlin("1.1")
+public inline fun <T, K, R, M : MutableMap<in K, R>> Grouping<T, K>.aggregateTo(
+        destination: M,
+        operation: (key: K, accumulator: R?, element: T, first: Boolean) -> R


value vs accumulator, v vs acc

cy6erGn0m · 2016-12-29T12:23:10Z

libraries/stdlib/src/kotlin/collections/Grouping.kt

+        fold(
+                initialValueSelector = { k, e -> kotlin.jvm.internal.Ref.IntRef() },
+                operation = { k, acc, e -> acc.apply { element += valueSelector(e) } })
+        .mapValues { it.value.element }


as we already use internal API hack we could do even more: we can create replace IntRef to Int inplace and then do unchecked cast.

I wouldn't call it internal API hack, because that API is used publicly by the compiler in the generated bytecode.
mapInPlace is possible as JVM-only optimization, otherwise it would make the code non-portable to platforms, where generics are reified.

cy6erGn0m · 2016-12-29T12:55:04Z

libraries/stdlib/common/src/generated/_Strings.kt

+ * using the specified [keySelector] function to extract a key from each character.
+ */
+@SinceKotlin("1.1")
+public header inline fun <K> CharSequence.groupingBy(crossinline keySelector: (Char) -> K): Grouping<Char, K>


why there is specialization here? the implementations look the same

It's how the code is generated now.

…ate implementation to aggregateTo.

ilya-g · 2017-01-02T13:17:27Z

`libraries/stdlib/src/kotlin/collections/Grouping.kt:35`
I'm not sure how could it be possible without wrapping each value. And wrapping each value is very ineffective in terms of number of allocations.

yole · 2017-01-02T17:00:37Z

`libraries/stdlib/src/kotlin/collections/Grouping.kt:35`
True. I don't have any better suggestion to improve this, so let's keep it as is.

yole · 2017-01-02T17:09:54Z

`libraries/stdlib/src/kotlin/collections/Grouping.kt:47`
You're using inconsistent wording - "aggregates with" or "accumulates with" - and neither of those is a standard usage of the corresponding verbs with the preposition. I think you need to provide a more detailed explanation of what happens here - something like applies [operation] to the elements of each group sequentially, passing the previously accumulated value and the current element as arguments, and stores the results in the given [destination] map". This applies to other functions as well.

yole · 2017-01-02T17:11:16Z

libraries/stdlib/src/kotlin/collections/Grouping.kt

+ * they group elements by their keys and then fold each group with some aggregating operation.
+ *
+ * It is created by attaching `keySelector: (T) -> K` function to a source of elements.
+ * To get an instance of [Grouping] use one of `groupingBy` extension functions:


line 32

"a group" -> "the group" here and below

yole · 2017-01-02T17:13:17Z

`libraries/stdlib/src/kotlin/collections/Grouping.kt:213`
count of element_s_

yole · 2017-01-02T17:13:38Z

`libraries/stdlib/src/kotlin/collections/Grouping.kt:219`
Why the space after the open parenthesis?

yole · 2017-01-02T17:15:42Z

libraries/stdlib/src/kotlin/collections/Grouping.kt

+ * It is created by attaching `keySelector: (T) -> K` function to a source of elements.
+ * To get an instance of [Grouping] use one of `groupingBy` extension functions:
+ * - [Iterable.groupingBy]
+ * - [Sequence.groupingBy]


line 61

Any chance for some @sample's?

aggregate and aggregateTo are not intended to be used much in user code, they are here more to implement other folding functions when fold or reduce do not fit for some reason.
Other functions (fold, reduce, eachCount, etc) will get samples eventually.

yole · 2017-01-02T17:17:11Z

`libraries/stdlib/src/kotlin/collections/Grouping.kt:240`
Why is this function called `eachSumOf` and not `eachSumBy`, which would be consistent with `Iterable.sumBy`?

ilya-g · 2017-01-03T13:32:15Z

Why is this function called eachSumOf and not eachSumBy, which would be consistent with Iterable.sumBy?

We're going to rename sumBy and sumByDouble to sumOf for Iterables, however it's blocked by KT-11265. Maybe we should not introduce eachSumOf until then.

…ameters of lambdas to _.

ilya-g self-assigned this Dec 27, 2016

ilya-g added 2 commits December 28, 2016 17:50

Tests for group-and-fold operations

16bff4c

ilya-g force-pushed the rr/stdlib/groupingBy branch from 80b5a39 to 16bff4c Compare December 28, 2016 15:06

cy6erGn0m reviewed Dec 29, 2016

View reviewed changes

ilya-g added 2 commits December 29, 2016 19:04

Minor refactoring: use mapValuesInPlace optimization, delegate aggreg…

3327c35

…ate implementation to aggregateTo.

Rename Grouping.elementIterator to sourceIterator

fed11aa

yole reviewed Jan 2, 2017

View reviewed changes

Improve docs, use consistent naming for parameters, rename unused par…

e7d9b79

…ameters of lambdas to _.

ilya-g closed this Jan 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

group-and-fold implementation #1004

group-and-fold implementation #1004

ilya-g commented Dec 27, 2016 •

edited

Loading

ilya-g commented Dec 28, 2016

cy6erGn0m Dec 29, 2016

ilya-g Dec 29, 2016

ilya-g Dec 29, 2016

ilya-g Dec 29, 2016

cy6erGn0m Dec 29, 2016

cy6erGn0m Dec 29, 2016

cy6erGn0m Dec 29, 2016

ilya-g Dec 29, 2016

cy6erGn0m Dec 29, 2016

ilya-g Dec 29, 2016

ilya-g commented Jan 2, 2017

yole commented Jan 2, 2017

yole commented Jan 2, 2017

yole Jan 2, 2017

yole commented Jan 2, 2017

yole commented Jan 2, 2017

yole Jan 2, 2017

ilya-g Jan 11, 2017

yole commented Jan 2, 2017

ilya-g commented Jan 3, 2017

group-and-fold implementation #1004

group-and-fold implementation #1004

Conversation

ilya-g commented Dec 27, 2016 • edited Loading

ilya-g commented Dec 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilya-g commented Jan 2, 2017

yole commented Jan 2, 2017

yole commented Jan 2, 2017

Choose a reason for hiding this comment

yole commented Jan 2, 2017

yole commented Jan 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yole commented Jan 2, 2017

ilya-g commented Jan 3, 2017

ilya-g commented Dec 27, 2016 •

edited

Loading