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

Replace Monoid constraint by CommutativeSemigroup in the reduce syntax #203

Merged
merged 11 commits into from
Jan 16, 2018
Merged

Replace Monoid constraint by CommutativeSemigroup in the reduce syntax #203

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
8363460
Use a CommutativeMonoid in the reduce operations
Nov 1, 2017
b4dae3d
Use a commutative semigroup when reducing
Nov 1, 2017
8756a59
Add tests for RDD reductions by key
Nov 2, 2017
9b5c8dd
Verify the contents regardless of the order
Nov 2, 2017
f76c246
Upgrade Cats MTL
Nov 25, 2017
08010dc
Update cats
Dec 20, 2017
a136d69
Use sorted maps as they are commutative
Dec 20, 2017
c92df2f
empty-safe RDD reductions
Dec 21, 2017
2e43003
relax constraint on csumByKey
Dec 21, 2017
606282f
Upgrade Cats and related deps
Jan 2, 2018
067fbbd
Replace <+> by orElse
Jan 2, 2018
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
4 changes: 2 additions & 2 deletions build.sbt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
val sparkVersion = "2.2.0"
val catsCoreVersion = "1.0.0-MF"
val catsEffectVersion = "0.4"
val catsCoreVersion = "1.0.0-RC1"
val catsEffectVersion = "0.5"
val catsMtlVersion = "0.0.2"
val scalatest = "3.0.3"
val shapeless = "2.3.2"
Expand Down
7 changes: 4 additions & 3 deletions cats/src/main/scala/frameless/cats/implicits.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,22 @@
package frameless
package cats

import _root_.cats.implicits._
import _root_.cats._
import _root_.cats.kernel.CommutativeSemigroup
import _root_.cats.implicits._

import scala.reflect.ClassTag
import org.apache.spark.rdd.RDD

object implicits extends FramelessSyntax with SparkDelayInstances {
implicit class rddOps[A: ClassTag](lhs: RDD[A]) {
def csum(implicit m: Monoid[A]): A = lhs.reduce(_ |+| _)
def csum(implicit m: CommutativeSemigroup[A]): A = lhs.reduce(_ |+| _)
def cmin(implicit o: Order[A]): A = lhs.reduce(_ min _)
def cmax(implicit o: Order[A]): A = lhs.reduce(_ max _)
}

implicit class pairRddOps[K: ClassTag, V: ClassTag](lhs: RDD[(K, V)]) {
def csumByKey(implicit m: Monoid[V]): RDD[(K, V)] = lhs.reduceByKey(_ |+| _)
def csumByKey(implicit m: CommutativeSemigroup[V]): RDD[(K, V)] = lhs.reduceByKey(_ |+| _)
def cminByKey(implicit o: Order[V]): RDD[(K, V)] = lhs.reduceByKey(_ min _)
def cmaxByKey(implicit o: Order[V]): RDD[(K, V)] = lhs.reduceByKey(_ max _)
}
Expand Down
14 changes: 7 additions & 7 deletions cats/src/test/scala/frameless/cats/test.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ class Test extends PropSpec with Matchers with PropertyChecks with SparkTests {
}
}

property("rdd simple numeric monoid example") {
property("rdd simple numeric commutative semigroup example") {
import frameless.cats.implicits._
val theSeq = 1 to 20
val toy = theSeq.toRdd
Expand All @@ -83,13 +83,13 @@ class Test extends PropSpec with Matchers with PropertyChecks with SparkTests {
toy.csum shouldBe theSeq.sum
}

property("rdd Map[Int,Int] monoid example") {
import frameless.cats.implicits._
val rdd: RDD[Map[Int, Int]] = 1.to(20).zip(1.to(20)).toRdd.map(Map(_))
rdd.csum shouldBe 1.to(20).zip(1.to(20)).toMap
}
// property("rdd Map[Int,Int] monoid example") {
// import frameless.cats.implicits._
Copy link
Contributor

Choose a reason for hiding this comment

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

The Map instance was moved to alleycats in typelevel/cats@1f0cba0

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @alonsodomin, can we bring this back based on @iravid suggestion?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @imarios, sorry for leavin this unattended. I just pushed an upgrade of the cats-mtl package.

There is still the problem with the Map test as there is no CommutativeSemigroup instance possible of an arbitrary Map, there could be one for SortedMap but cats(and alleycats) do not provide with one.

You can see @iravid comment below in which he acknowledges that he was thinking of the Traversable instance, not the CommutativeSemigroup one. So, in this case, if frameless is interested on enforcing correctness for those methods, probably it's better to drop the test for the Map... either that, or provide to alleycats a CommutativeSemigroup instance for SortedMap and wait until it's merged...

Copy link
Contributor

Choose a reason for hiding this comment

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

What if we replace Map with SortedMap? Will we be able to have this test back?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @imarios, just made a PR to cats adding a CommutativeMonoid instance for SortedMap typelevel/cats#2047. Once it gets merged I can recover the commented out tests in here.

// val rdd: RDD[Map[Int, Int]] = 1.to(20).zip(1.to(20)).toRdd.map(Map(_))
// rdd.csum shouldBe 1.to(20).zip(1.to(20)).toMap
// }

property("rdd tuple monoid example") {
property("rdd tuple commutative semigroup example") {
import frameless.cats.implicits._
val seq = Seq( (1,2), (2,3), (5,6) )
val rdd = seq.toRdd
Expand Down