Remove Totality requirement for ResolveValue #333
Conversation
jorisdral
left a comment
jorisdral
left a comment
There was a problem hiding this comment.
LGTM. Small but unrelated comment below
There was a problem hiding this comment.
It might be better to move the ResolveValue Word64 instance to the test suite, since it might/will conflict with user instance. Or, alternatively, you can use a newtype in the style of Data.Monoid.Sum as an example, and it might be useful as a modifier as well for making number types into instances of ResolveValue
There was a problem hiding this comment.
I initially also wanted to provide an instance for Sum, but it's trickier than I thought. resolveDeserialised requires an instance SerialiseValue (Sum a) (and I didn't want to coerce things based on SerialiseValue a because the instances might disagree), but I'm not sure we generally want to have instance SerialiseValue a => SerialiseValue (Sum a). But if we don't, users would have to define an orphan instance for e.g. SerialiseValue (Sum Foo), which seems wrong.
There was a problem hiding this comment.
I can provide instances SerialiseValue (Sum Word64) and ResolveValue (Sum Word64) specifically, maybe that's an okay compromise?
There was a problem hiding this comment.
I added the SerialiseValue (Sum Word64) and ResolveValue (Sum Word64) instances. I squashed the commit and re-based with main in anticipation of merging.
There was a problem hiding this comment.
Per a discussion with @mheinzel:
Regarding the instance SerialiseValue a => SerialiseValue (Sum a), it essentially states that the Sum newtype is transparent to serialization. This assertion is not unreasonable, as it is highly improbable one would serialize Sum a differently than a.
Hence we can have the more useful polymorphic instance:
instance (Num a, SerialiseValue a) => ResolveValue (Sum a)
recursion-ninja
force-pushed
the
mheinzel/resolve-value-totality
branch
4 times, most recently
from
b5efea7 to
ada7ef0
Compare
mheinzel
left a comment
mheinzel
left a comment
There was a problem hiding this comment.
One last detail, then we can merge.
| tests = testGroup "Test.Database.LSMTree.Monoidal" | ||
| [ testGroup "Word64" (allProperties @Word64 False) | ||
| -- TODO: revisit totality (drop requirement or fix @SerialiseValue Word64@) | ||
| [ testGroup "Word64" (allProperties @Word64) |
There was a problem hiding this comment.
Ah, I thought we would just use the existing instance for ResolveValue (Sum Word64) instead of defining an orphan instance.
mheinzel
dismissed
jorisdral’s stale review
Has been addressed, but Joris is away.
recursion-ninja
force-pushed
the
mheinzel/resolve-value-totality
branch
3 times, most recently
from
0289087 to
488a9d6
Compare
recursion-ninja
force-pushed
the
mheinzel/resolve-value-totality
branch
from
488a9d6 to
bfd74ac
Compare
We can revisit this once we focus on robustness, including error handling during run merges.