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.

Sign up for GitHub

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

Backport #2380 Add combineAllOption to Foldable #3340

Merged
merged 4 commits into from
Mar 11, 2020
Merged

Backport #2380 Add combineAllOption to Foldable #3340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
29c83fd
backported #2380 Added combineAllOption to Foldable
gagandeepkalra Mar 2, 2020
092b0d3
addressed review feedback, removed FoldableCompat
gagandeepkalra Mar 4, 2020
0ba553b
removed toIterable override for `Stream`
gagandeepkalra Mar 5, 2020
41f6306
Merge branch 'scala_2.11' into backport/foldable/combineAllOption
gagandeepkalra Mar 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions core/src/main/scala/cats/Foldable.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package cats
import cats.Foldable.sentinel
import cats.instances.either._
import cats.kernel.CommutativeMonoid
import cats.syntax.foldable
import simulacrum.typeclass

import scala.collection.mutable
Expand Down Expand Up @@ -239,9 +240,7 @@ import scala.collection.mutable
* Fold implemented using the given Monoid[A] instance.
*/
def fold[A](fa: F[A])(implicit A: Monoid[A]): A =
foldLeft(fa, A.empty) { (acc, a) =>
A.combine(acc, a)
}
A.combineAll(foldable.catsSyntaxFoldableOps0(fa).toIterable(this))

/**
* Alias for [[fold]].
Expand Down
12 changes: 12 additions & 0 deletions core/src/main/scala/cats/syntax/foldable.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,18 @@ final class FoldableOps0[F[_], A](private val fa: F[A]) extends AnyVal {
def maximumByOption[B: Order](f: A => B)(implicit F: Foldable[F]): Option[A] =
F.maximumOption(fa)(Order.by(f))

def combineAllOption(implicit ev: Semigroup[A], F: Foldable[F]): Option[A] =
if (F.isEmpty(fa)) None else ev.combineAllOption(toIterable)

/**
* Convert F[A] to an Iterable[A].
*
* This method may be overridden for the sake of performance, but implementers should take care
* not to force a full materialization of the collection.
*/
def toIterable(implicit F: Foldable[F]): Iterable[A] =
F.foldRight[A, Stream[A]](fa, Eval.now(Stream.empty))((a, eb) => eb.map(Stream.cons(a, _))).value
Copy link
Contributor Author

@gagandeepkalra gagandeepkalra Mar 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@travisbrown

Hello, sorry to bother you again on this PR.

I was looking at the toIterable implementation to understand why would we use Stream.

These are the possible arguments that I read from the original PR-

Still aiming at mitigating the possible cost of building a list, it could be delayed up to when the iterator is traversing it

and

@travisbrown .toList.iterator forces a full materialization, and we don't control the API of List. So, the uses of a lazy structure (like Iterator, Stream, LazyList) are very different since we can interact with other tools that will not force a full materialization of the data. This is useful in things like spark or scalding.

That said, I think in a library focused on FP, we should use an immutable lazy structure if possible, so I would lean towards Stream/LazyList. Users can then call .iterator on that, but we don't control that API. Alternatively, they can use foldLeft to tear down the Stream without materializing the whole thing in memory at once.

this is when I added a few printLn in between, confirming my suspicion that the Stream is created from the end to the first element, this means that the entire Stream is already materialised yet not visible.

Screenshot 2020-03-05 at 9 09 28 PM

when replacing Stream with List, no difference.

Screenshot 2020-03-05 at 9 16 09 PM

and given this evidence we can replace this entire implementation with-

F.foldLeft(fa, List.empty[A])((acc, a) => a :: acc).reverse

Stream is materialised yet not visible then re-materialised (going through the whole synchronise thing).

Do you see value here?

Copy link
Contributor Author

@gagandeepkalra gagandeepkalra Mar 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@travisbrown if you get a chance to go through this one, that's the last of it


/**
* Implementers are responsible for ensuring they maintain consistency with foldRight; this is not checked by laws on Scala 2.11
*/
Expand Down
14 changes: 14 additions & 0 deletions tests/src/test/scala/cats/tests/FoldableSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,20 @@ abstract class FoldableSuite[F[_]: Foldable](name: String)(implicit ArbFInt: Arb
}
}

test(s"Foldable[$name].combineAllOption") {
forAll { (fa: F[Int]) =>
fa.combineAllOption should ===(fa.toList.combineAllOption)
fa.combineAllOption should ===(iterator(fa).toList.combineAllOption)
}
}

test(s"Foldable[$name].iterable") {
forAll { (fa: F[Int]) =>
fa.toIterable.toList should ===(fa.toList)
fa.toIterable.toList should ===(iterator(fa).toList)
}
}

test(s"Foldable[$name].intercalate") {
forAll { (fa: F[String], a: String) =>
fa.intercalate(a) should ===(fa.toList.mkString(a))
Expand Down