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

Make Bounds type safe #462

Closed
maurolacy opened this issue Oct 1, 2021 · 3 comments · Fixed by #627
Closed

Make Bounds type safe #462

maurolacy opened this issue Oct 1, 2021 · 3 comments · Fixed by #627

Comments

@maurolacy
Copy link
Contributor

maurolacy commented Oct 1, 2021

Current Bounds take a Vec<u8> as parameter, which is not related to the types the bound will apply to. This forces the user to build the proper bound manually, which requires knowledge of the internal structure used to store the different types. This is particularly annoying with composite keys, where a helper (joined_key) must be used to concatenate the key components.

Let's improve on this by making Bound type-safe. This may be tricky to do, as currently Bound is independent of the map-like structure it can be applied to.

There's a key() (index_key in the case if indexed maps) method in map-like structs, that can be used as an alternative to building the key manually. But, it only supports the full key, not the prefix / sub-prefix keys. An alternative could be to extend this with a couple of prefix_key() and sub_prefix_key() helpers.

Let's evaluate first if we can do better than that.

@maurolacy
Copy link
Contributor Author

maurolacy commented Oct 5, 2021

A simple way to improve here would be to provide more helpers over PrimaryKey (or a specific trait).

So that, instead of something like

                .range(
                    &store,
                    Some(Bound::inclusive((U64Key::new(0), "").joined_key())),
                    Some(Bound::exclusive((U64Key::new(2), "").joined_key())),
                    Order::Ascending,
                )

the user can write instead

                .range(
                    &store,
                    (U64Key::new(0), "").inclusive_bound(),
                    (U64Key::new(2), "").exclusive_bound(),
                    Order::Ascending,
                )

This looks better. It requires knowledge of the key structure, but this info is public already. This is still not type-safe, in that the bound is still over Vec<u8> but is a step forward in usability.

From there, making these type-safe (i. e. building Bounds over K, K::Suffix and K::SuperSuffix) is achievable. It will require no_prefix() prefix() and sub_prefix() returning different objects (or having multiple range impls?). So that their ranges are associated to specific types. It will also requiere Bound to preserve type information, i.e. to be implemented using generics.

Need to think more about this, but it looks achievable.

All this means, more "boiler plate" code on our side. But definitely, a better / safer user experience.

@maurolacy
Copy link
Contributor Author

maurolacy commented Dec 1, 2021

A simpler alternative would be to provide helpers over Bound itself. Like it's currently being done for PrefixBound.

Update: It turns out those helpers are already there(!) So, the issue here would be to enforce the use of the helpers. i.e. to prevent or discourage calling Bound::Inclusive / Bound::Exclusive directly.
And, to improve the helpers for the integer key case (if needed), and deprecate the associated *_int helpers.

The limitation of this approach is that its K would not be tied to the K in the Map-like struct being used.

@ethanfrey
Copy link
Member

This is a nice idea. I would like to highlight it for 0.12.0 storage-plus improvements.

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

Successfully merging a pull request may close this issue.

2 participants