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

MultiIndex primary key spec removal #569

Merged
merged 9 commits into from
Dec 8, 2021
Merged

Conversation

maurolacy
Copy link
Contributor

@maurolacy maurolacy commented Dec 6, 2021

Closes #533.

Builds on top of #568. Makes it simpler to handle MultiIndex by removing the pk from the index key specification.

Solved it by collapsing the index key along with an extra element (the raw pk). This makes it possible to keep the old format in the store, as the new (internally generated) index key would be exactly the same as the one corresponding to the previous key spec.

Overall, I'm pretty happy with this implementation. It has some tricks, like shifting the definition of what is a prefix and a sub-prefix. So that prefixes can still be specified over the full index key (which doesn't include the primary key in its spec anymore). Despite (or because of) that, I think the end result is both clear and ergonomic.

This PR also renames K to IK for clarity. So, it also closes (the second part of) #531.

TODO:

  • Update related documentation / comments. (done)

@maurolacy maurolacy force-pushed the 533-multi_index-pk-spec-removal branch from 81236b5 to 9a517d3 Compare December 6, 2021 09:50
Base automatically changed from 532-index-keys-consistency-alt to main December 7, 2021 05:27
Copy link
Contributor

@ueco-jb ueco-jb left a comment

Choose a reason for hiding this comment

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

lgtm, mostly nitpicking comments regarding comments.

packages/storage-plus/README.md Outdated Show resolved Hide resolved
packages/storage-plus/src/indexed_map.rs Outdated Show resolved Hide resolved
Comment on lines +26 to +27
pub struct MultiIndex<'a, IK, T, PK = ()> {
index: fn(&T) -> IK,
Copy link
Contributor

@ueco-jb ueco-jb Dec 7, 2021

Choose a reason for hiding this comment

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

This K -> IK change is quite sudden. I understand that K stands for Key, but is IK an IndexKey?
I think it would be good to have some reference to it... I doubt it will be clear to people diving into this code from outside.

Copy link
Contributor Author

@maurolacy maurolacy Dec 7, 2021

Choose a reason for hiding this comment

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

It is an index key, yes. It's a way to differentiate it from both a PK (primary key), and from a generic K that could stand for any of them.

@ethanfrey
Copy link
Member

I don't have the time to dig into this, which it deserves.

If you and Jake agree, feel free to merge.
I feel this is getting to larger changes and should be wrapped into a 0.11 release along with the other pending stuff.

Meaning, go ahead and merge, but we commit to no more 0.10.x releases now. And start all the 0.11 cleanup stuff. (When we get to them). Let's get that utils rename in (finally)

@maurolacy maurolacy force-pushed the 533-multi_index-pk-spec-removal branch from 401e386 to 491af83 Compare December 8, 2021 06:44
@maurolacy maurolacy force-pushed the 533-multi_index-pk-spec-removal branch from 491af83 to 2271956 Compare December 8, 2021 09:15
@maurolacy maurolacy merged commit 9d32f74 into main Dec 8, 2021
@maurolacy maurolacy deleted the 533-multi_index-pk-spec-removal branch December 8, 2021 09:24
@ueco-jb ueco-jb mentioned this pull request Dec 22, 2021
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

Successfully merging this pull request may close these issues.

Remove the primary key from the MultiIndex key specification
3 participants