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

content of case study :CRDTs got unexpected result #206

Open
chenghsienwen opened this issue Jun 13, 2021 · 3 comments
Open

content of case study :CRDTs got unexpected result #206

chenghsienwen opened this issue Jun 13, 2021 · 3 comments

Comments

@chenghsienwen
Copy link

this is about the result of case study is unexpected
my test of unexpected result
https://github.com/chenghsienwen/AmmSandbox/blob/master/cats-execise-crdt4.sc

expected result:
https://github.com/chenghsienwen/AmmSandbox/blob/master/cats-execise-crdt3.sc

does any one get correct result on "K.5 Abstracting a Key Value Store"?

Thank you

@RyotaBannai
Copy link

RyotaBannai commented Jul 11, 2021

I faced the same issue as @chenghsienwen mentioned, the BoundedSemiLattice |+| method should be called, but the default combine method from Monoid is called instead, assuming that scala compiler selects narrower one to apply, I'm not sure how to get around.

@nathanmbrown
Copy link

nathanmbrown commented Dec 14, 2022

I just ran into this also. The issue is that, for the merge to work correctly as coded with the abstracted GCounter instance, the Int-specific BoundedSemiLattice instance needs to be implicitly available when it is constructed, so that the combine operation of the Monoid/Semigroup for the KVS in the merge has it available when combining the Maps later. If you don't import it, then it will use catsKernelStdGroupForInt instead to merge items which just adds them instead, breaking the merge logic.
See the screenshot below that shows where it is going in:
image
The problem is, if you do make it implicitly available at that point then it is preferred over the catsKernelStdGroupForInt when calling the total method on the GCounter later. See screenshot below. One way to remedy is to then explicitly pass catsKernelStdGroupForInt when calling total but that is not great. (shown as green in screenshot)
image
It would be good to have some guidance from the authors on best practices on how to avoid or work around these issues - this sort of thing would make me nervous to start using cats in production code as these sorts of issues are pretty opaque and hard to understand!
cc: @noelwelsh / @davegurnell

@noelwelsh
Copy link
Contributor

This is an issue with this case study (and why I now think it's not a good example of using type classes): it requires two different type class instances on the same type to function correctly (one is the semilattice, which should behave like max and one is the monoid that should behave like +).

It's not something that shows up in usual practice using type classes, and going into this subtlety isn't really appropriate for the audience Scala with Cats is aimed at.

https://creativescala.github.io/case-study-parser/index.html is an example of new material that's in development, which I think is better.

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

No branches or pull requests

4 participants