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 missing Chain#distinctBy method #4156

Merged
merged 2 commits into from
Apr 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
64 changes: 54 additions & 10 deletions core/src/main/scala/cats/data/Chain.scala
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ import cats.kernel.instances.StaticMethods

import scala.annotation.tailrec
import scala.collection.immutable.SortedMap
import scala.collection.immutable.TreeSet
import scala.collection.immutable.{IndexedSeq => ImIndexedSeq}
import scala.collection.mutable
import scala.collection.mutable.ListBuffer

import Chain.{
Expand Down Expand Up @@ -151,6 +151,10 @@ sealed abstract class Chain[+A] extends ChainCompat[A] {
*/
final def nonEmpty: Boolean = !isEmpty

// Quick check whether the chain is either empty or contains one element only.
@inline private def isEmptyOrSingleton: Boolean =
isEmpty || this.isInstanceOf[Chain.Singleton[_]]

/**
* Concatenates this with `c` in O(1) runtime.
*/
Expand Down Expand Up @@ -760,19 +764,59 @@ sealed abstract class Chain[+A] extends ChainCompat[A] {

/**
* Remove duplicates. Duplicates are checked using `Order[_]` instance.
*
* Example:
* {{{
* scala> import cats.data.Chain
* scala> val chain = Chain(1, 2, 2, 3)
* scala> chain.distinct
* res0: cats.data.Chain[Int] = Chain(1, 2, 3)
* }}}
*/
def distinct[AA >: A](implicit O: Order[AA]): Chain[AA] = {
implicit val ord: Ordering[AA] = O.toOrdering

var alreadyIn = TreeSet.empty[AA]
if (isEmptyOrSingleton) this
else {
implicit val ord: Ordering[AA] = O.toOrdering

val bldr = Vector.newBuilder[AA]
val seen = mutable.TreeSet.empty[AA]
val it = iterator
while (it.hasNext) {
val next = it.next()
if (seen.add(next))
bldr += next
}
// Result can contain a single element only.
Chain.fromSeq(bldr.result())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Potentially this line (and the similar one below) could be expanded into something more efficient like

bldr.result() match {
  case Vector(a) => Singleton(a)
  case seq       => Wrap(seq)
}

but... I'm not really sure it is worth it – I mean, it is unlikely that such a single call can lead to substantial (or even noticeable) performance decrease, IMO.

}
}

foldLeft(Chain.empty[AA]) { (elementsSoFar, b) =>
if (alreadyIn.contains(b)) {
elementsSoFar
} else {
alreadyIn += b
elementsSoFar :+ b
/**
* Remove duplicates by a predicate. Duplicates are checked using `Order[_]` instance.
*
* Example:
* {{{
* scala> import cats.data.Chain
* scala> val chain = Chain(1, 2, 3, 4)
* scala> chain.distinctBy(_ / 2)
* res0: cats.data.Chain[Int] = Chain(1, 2, 4)
* }}}
*/
def distinctBy[B](f: A => B)(implicit O: Order[B]): Chain[A] = {
if (isEmptyOrSingleton) this
else {
implicit val ord: Ordering[B] = O.toOrdering

val bldr = Vector.newBuilder[A]
val seen = mutable.TreeSet.empty[B]
val it = iterator
while (it.hasNext) {
Copy link
Contributor

Choose a reason for hiding this comment

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

one issue: this is going to create a Chain the costs O(N) to uncons.

I wonder if the better approach would build a Vector and Wrap that. Vector seems to be the default collection we use in other places

e.g.

val bldr = Vector.newBuilder[A]
...

Wrap(bldr.result())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I wonder too. I didn't really think about it, perhaps you're right. Let's try this option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

val next = it.next()
if (seen.add(f(next)))
bldr += next
}
// Result can contain a single element only.
Chain.fromSeq(bldr.result())
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
* Copyright (c) 2015 Typelevel
*
* Permission is hereby granted, free of charge, to any person obtaining a copy of
* this software and associated documentation files (the "Software"), to deal in
* the Software without restriction, including without limitation the rights to
* use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of
* the Software, and to permit persons to whom the Software is furnished to do so,
* subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in all
* copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
* FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
* COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER
* IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
* CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
*/

package cats.tests.compat

import scala.collection.immutable.Seq

private[tests] trait ScalaVersionSpecificSyntax {
implicit final private[tests] def catsTestsCompatSeqOps[C[a] <: Seq[a], A](self: C[A]) = new SeqOps[C, A](self)
}
47 changes: 47 additions & 0 deletions tests/shared/src/test/scala-2.12/cats/tests/compat/SeqOps.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/*
* Copyright (c) 2015 Typelevel
*
* Permission is hereby granted, free of charge, to any person obtaining a copy of
* this software and associated documentation files (the "Software"), to deal in
* the Software without restriction, including without limitation the rights to
* use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of
* the Software, and to permit persons to whom the Software is furnished to do so,
* subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in all
* copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
* FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
* COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER
* IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
* CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
*/

package cats.tests.compat

import scala.collection.generic.CanBuildFrom
import scala.collection.immutable.Seq
import scala.collection.mutable

final private[tests] class SeqOps[C[a] <: Seq[a], A] private[compat] (private val self: C[A]) extends AnyVal {

// Scala v2.12.x does not have `distinctBy` implemented.
// Therefore this implementation is copied (and adapted) from Scala Library v2.13.8 sources:
// https://github.com/scala/scala/blob/v2.13.8/src/library/scala/collection/immutable/StrictOptimizedSeqOps.scala#L26-L39
def distinctBy[B](f: A => B)(implicit cbf: CanBuildFrom[C[A], A, C[A]]): C[A] = {
if (self.lengthCompare(1) <= 0) self
else {
val builder = cbf()
val seen = mutable.HashSet.empty[B]
val it = self.iterator
var different = false
while (it.hasNext) {
val next = it.next()
if (seen.add(f(next))) builder += next else different = true
}
if (different) builder.result() else self
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/*
* Copyright (c) 2015 Typelevel
*
* Permission is hereby granted, free of charge, to any person obtaining a copy of
* this software and associated documentation files (the "Software"), to deal in
* the Software without restriction, including without limitation the rights to
* use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of
* the Software, and to permit persons to whom the Software is furnished to do so,
* subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in all
* copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
* FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
* COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER
* IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
* CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
*/

package cats.tests.compat

private[tests] trait ScalaVersionSpecificSyntax {}
6 changes: 6 additions & 0 deletions tests/shared/src/test/scala/cats/tests/ChainSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,12 @@ class ChainSuite extends CatsSuite {
}
}

test("Chain#distinctBy is consistent with List#distinctBy") {
forAll { (a: Chain[Int], f: Int => String) =>
assertEquals(a.distinctBy(f).toList, a.toList.distinctBy(f))
}
}

test("=== is consistent with == (issue #2540)") {
assertEquals(Chain.one(1) |+| Chain.one(2) |+| Chain.one(3), Chain.fromSeq(List(1, 2, 3)))

Expand Down
24 changes: 24 additions & 0 deletions tests/shared/src/test/scala/cats/tests/package.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/*
* Copyright (c) 2015 Typelevel
*
* Permission is hereby granted, free of charge, to any person obtaining a copy of
* this software and associated documentation files (the "Software"), to deal in
* the Software without restriction, including without limitation the rights to
* use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of
* the Software, and to permit persons to whom the Software is furnished to do so,
* subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in all
* copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
* FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
* COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER
* IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
* CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
*/

package cats

package object tests extends cats.tests.compat.ScalaVersionSpecificSyntax