-
Notifications
You must be signed in to change notification settings - Fork 8
Introduce a unified LSMTree
API
#469
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
Conversation
174d23a
to
469f398
Compare
7bedfcc
to
36b4a77
Compare
type Table' :: (Type -> Type) -> Type -> Type -> Type -> Type | ||
data Table' m k v b = forall h. Typeable h => Table' (Table m h) | ||
|
||
instance NFData (Table' m k v b) where | ||
rnf (Table' t) = rnf t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a big fan of the pattern of defining these types here and using synonyms in the API modules, as synonyms can leak into error messages and it also makes the diff between them larger vs having three data Table
there.
But the pattern already exists, so it makes sense to follow it here. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In our current setup, we need to expose the constructors for our NoThunks
assertions, but @dcoutts suggested we do not expose the constructors from the public API. That's why I moved them here and added type synonyms in the public APIs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it would have been fine to expose the constructors in the public API, since if you want to do anything with the internal Table
then you'll have to import Internal
modules anyway
65df8a0
to
ea4dd27
Compare
The new API unifies the `Normal` and `Monoidal` APIs, supporting both blob references and mupserts, and is strictly more expressive. If a user does not want to use blobs, then the blob type can be set to `Void`. If a user does not intend to use the full potential of `Mupserts`, then the `ResolveAsFirst` wrapper provides a sensible default instance for the `ResolveValue` class.
ea4dd27
to
669e7f5
Compare
No description provided.