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 method eitherPar to the type class CommutativeEither #372

Closed
sideeffffect opened this issue Nov 14, 2020 · 6 comments
Closed

Add method eitherPar to the type class CommutativeEither #372

sideeffffect opened this issue Nov 14, 2020 · 6 comments

Comments

@sideeffffect
Copy link
Member

sideeffffect commented Nov 14, 2020

Difficulty: Easy, familiarity with the Type class design pattern is an advantage

After adding eitherPar to CommutativeEither, implement it for all the types implementing the Type class. The signature is like this.

  /**
   * Combines two values of types `F[A]` and `F[B]` in a parallel manner to produce an `F[Either[A, B]]`.
   */
  def eitherPar[A, B](fa: => F[A], fb: => F[B]): F[Either[A, B]]

Make sure, that the other helper and extension methods (like orElseEitherPar) in CommutativeEither use eitherPar instead of just either. Note, that for some types, eitherPar can be the same implementation as either.

As a stretch goal, move the CommutativeEither instances (all that can be, which are all in this case) to the supre-classes' AssociativeEither companion object. Read about the reasoning where to put Type class instance and the Implicit scope in this article https://meta.plasm.us/posts/2019/09/30/implicit-scope-and-cats/

As a stretch goal # 2, add tests for missing instances to CommutativeEitherSpec (tests for some basic instances, like Option or List are already present).

@lbdlbdlbdl
Copy link

I would like to work on this one. :)

@sideeffffect
Copy link
Member Author

sideeffffect commented Nov 26, 2020

By this ticket, we would have an CommutativeEither with both either and eitherPar, where either would express the sequential merging while eitherPar would express the "parallel" one (concurrent would be more apt, though :D ).

@adamgfraser in we merged #437, we would loose the ability to merge certain types in a sequential manner (like ZIO, Future, etc), which would be unfortunate.

But if you really wanted either on CommutativeEither to be the parallel/concurrent variant, we could "do it the other way around" and instead of adding eitherPar, we could have an eitherSeq member on CommutativeEither which would still enable use to the sequential way.

Or we could drop CommutativeEither for mere ZIOs and Futures etc. and introduce Sequential and Concurrent subtypes, where we could implement different strategies for Sequential[ZIO] and Concurrent[ZIO] and so on for the other. Similarly to the trick how we have both additive and multiplicative Magmas for integers, i.e. Sum[Int] and Prod[Int]

So I see 4 options:

  • 0: Lose the ability to merge ZIO/Future/... sequentially, but nobody wants that, I guess
  • 1: either is sequential, eitherPar is concurrent
  • 2: either is concurrent, eitherSeq is sequential
  • 3: Sequential and Concurrent subtypes, maybe the best, cleanest and has precedents in other types
    • 3a: Only use Sequential subtype.
    • 3b: Or only Concurrent subtype. 🤷

@adamgfraser @jdegoes so what do you guys think, which would you prefer? Or do you see some other option?

@adamgfraser
Copy link
Contributor

So I think we definitely want the ability to support both commutative and non-commutative combining operations, and those are important even for data types that don't do actual concurrency (e.g. zip versus zipPar on Validation). I see a couple of ways to do this:

  1. Just don't consolidate. Today the following works fine:
trait Validation[+E, +A]

object Validation extends LowPriorityValidationImplicits {
    def validationIdentityBoth[E]: IdentityBoth[Validation[E, _]] = ???
}

trait LowPriorityValidationImplicits {
  def validationCommutativeBoth[E]: CommutativeBoth[Validation[E, _]] = ???
}

This results in summoning AssociativeBoth giving you zip semantics and CommutativeBoth giving you zipPar semantics, which seems right. It also avoids us having to conflate these two operators. It does interfere a little with our efforts to push defining instances as high up in the hierarchy as possible but I think that is primarily our concern as library authors and can easily be worked around.

  1. Introduce new types

Just like we introduced new types like Sum and Prod we could introduce Par for the parallel version of type classes with two different versions and summon those when you wanted the explicitly parallel version. This would also work with the rest of our infrastructure.

  1. Have CommutativeBoth implement a new bothPar operator in addition to both so bothPar is parallel and both is sequential

I think this definitely works but I worry that it is the best design. We are conflating two different operations, sequential combination and parallel combination, that don't necessarily have any relationship to each other. I don't think there are really well defined laws that describe how zip and zipPar are supposed to relate to each other, they are just two different operators with their own laws. I'm not sure why implementing a type class for parallel combination should also require implementing logic for sequential combination or vice versa.

I am inclined to go with the first option.

@jdegoes
Copy link
Member

jdegoes commented Dec 12, 2020

I do like using "priority" to choose between commutative and non-commutative. The main problem is the clash. There is no way to define both associative (non-commutative) and associative (commutative) together (i.e. coherently). Maybe that matters less with Scala 3.

Another possibility is the newtyping. With curried type constructors, you could imagine doing A: Commutative[Associative]. That could be really nice.

Overall, priority seems the least risky.

@sideeffffect
Copy link
Member Author

Alright, keeping separate instances of Commutative* for things that have parallel/concurrent implementation, aka (1), could work 👍

  • method names will still be both/either; bothPar/eitherPar won't be needed
  • if the implementation can be either sequential of parallel/concurrent
    • sequential implementation will be Associative* or Identity*, located in Associative*.scala
    • parallel/concurrent implementation will be Commutative* in Commutative*.scala (it will be Commutative* only, not Commutative* with Identity*)
  • if implementation can be done in only one way, there's no need to split anything, the single instance will be located in Associative*.scala
  • extension methods will be still "split"
    • for Associative*, there will be <*>, zip, <+>, orElseEither, (a, b, c).mapN etc
    • for Commutative*, there will be <&>, zipPar, <|>, orElseEitherPar, (a, b, c).mapParN, etc

Btw, for Validation, there's not reason to have separate IdentityBoth and CommutativeBoth, the implementation for both has just one sensible implementation.

@adamgfraser
Copy link
Contributor

Sounds good.

The only thing I would add is I don't think that is accurate with regard to Validation. There is an AssociativeBoth instance that fails fast analogous to zipWith that is implemented in terms of flatMap and a CommutativeBoth instance that corresponds to zipWithPar and accumulates errors. We will pick that up as part of #461 though.

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

No branches or pull requests

4 participants