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

Prefixer sub prefix #215

Merged
merged 3 commits into from
Jan 19, 2021
Merged

Prefixer sub prefix #215

merged 3 commits into from
Jan 19, 2021

Conversation

maurolacy
Copy link
Contributor

@maurolacy maurolacy commented Jan 6, 2021

Implemented first variant for #214.

Update: This turned out to be simpler, as the same prefix() method can be used for both, prefixes and sub-prefixes, after signalling which one through its related type alias / helper function.

Only missing piece is de-serialization of remaining keys, as they are being returned encoded. This can be tricky, as we don't have a regular format for keys, i. e. the first ones are length prefixed, but the last one isn't.
I think we can live with this: it's an issue only for Map, and probably IndexedMap. For UniqueIndex, and upcoming MultiIndex with sub-prefixes, this is already taken care of by the custom de-serialization function.

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks

@@ -38,8 +40,9 @@ impl<'a> PrimaryKey<'a> for &'a [u8] {
// Provide a string version of this to raw encode strings
impl<'a> PrimaryKey<'a> for &'a str {
type Prefix = ();
type SubPrefix = ();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simple. Just add the 2-step variant rather than some generic N-step construction. Should be plenty for any case I can think of.


fn key<'b>(&'b self) -> Vec<&'b [u8]> {
fn key(&self) -> Vec<&[u8]> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And all that was unneeded?
Looks like a better clippy/linter got this working well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something the idea built-in linter recommends. These are called "elided lifetimes", and the linter's recommendation is to remove them.

all,
vec![
(
[to_length_prefixed(b"\x09"), b"recipient".to_vec()].concat(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the big-endian encoding of U8key manually...
This is fine for now. It would be interesting to see how to auto-parse these keys, but the caller should know how in the end

Copy link
Contributor Author

@maurolacy maurolacy Jan 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It could be handled automatically by the deserialization function (returning a vec! of (properly deserialized) tuples). But, for that we need to know if the sub-prefix is composed by one or more keys... if all the keys where length-prefixed that would be straightforward: we just honour the length-prefixed information. But currently, the last key is not length-prefixed. So, this introduces ambiguity.

I think an easy fix would be to make all keys length-prefixed, and for the deserialization fn to handle that directly.

Copy link
Contributor Author

@maurolacy maurolacy Jan 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also consider reducing the max length of each length-prefixed key to one byte (255) (which is more than enough for a prefix), to save some bytes / processing.

@ethanfrey ethanfrey merged commit 7cb45e8 into master Jan 19, 2021
@ethanfrey ethanfrey deleted the prefixer-sub-prefix branch January 19, 2021 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants