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

Indexed snapshot map #271

Merged
merged 6 commits into from
Apr 16, 2021
Merged

Indexed snapshot map #271

merged 6 commits into from
Apr 16, 2021

Conversation

maurolacy
Copy link
Contributor

@maurolacy maurolacy commented Apr 15, 2021

Closes #255.

Straightforward "clone" of IndexedMap.

@maurolacy maurolacy requested a review from ethanfrey April 15, 2021 15:35
@maurolacy maurolacy self-assigned this Apr 15, 2021
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.

You need to expose the SnapshotMap methods.
A straight copy is okay, but maybe a bit hard to maintain.


One thought (which may be misguided, but please consider) is to make some IndexedCollection which takes a trait for primary. It needs:

trait Indexable {
  save(&self, ...);
  remove(&self, ...);
  may_load(&self, ...);
  // and maybe
  load(&self, ...);
}

Then you can wrap either a Map or IndexedMap as "Indexable".

In order to expose the custom methods on SnapshotMap, one could make pub primary: T <T: Indexable>, and call indexed.primary.load_at_height(...) for example.

(Note, maybe it is best that Indexable uses different names, so the common names don't require importing a trait, just a trait wrapper that defers to those, but you get the idea).

Then again, the majority of the work is done inside the Indexes (not IndexedMap), so maybe this is the simplest and most stable solution.

packages/storage-plus/src/indexed_snapshot.rs Outdated Show resolved Hide resolved
packages/storage-plus/src/indexed_snapshot.rs Outdated Show resolved Hide resolved
@maurolacy maurolacy force-pushed the indexed-snapshot-map branch from 84389c2 to 4b01952 Compare April 16, 2021 08:03
@ethanfrey
Copy link
Member

Looks good, so this is the basic implementation, and I look at #272 for usage.

Do you want to merge those together and then go to master? Or merge this first, then the other when it is ready?

In either case, happy to review it as is, and give some pointers. But let's revert the pub primary once we have finalized the design and before merging to master.

@ethanfrey
Copy link
Member

Once #272 passes CI with full tests, I would just address this comment as part of this PR: #272 (comment)

Otherwise, looking good

@maurolacy
Copy link
Contributor Author

Thanks for the comments. I would merge this first, as it's done and contained.

And yes, we can always revert the pub primary in the other branch, when the proper methods are exposed.

@maurolacy maurolacy merged commit 735ec2f into master Apr 16, 2021
@maurolacy maurolacy deleted the indexed-snapshot-map branch April 16, 2021 10:40
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.

Add secondary index support to SnapshotMap.
2 participants