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

Secondary indexes support #557

Closed
wants to merge 34 commits into from
Closed

Secondary indexes support #557

wants to merge 34 commits into from

Conversation

ethanfrey
Copy link
Member

@ethanfrey ethanfrey commented Oct 1, 2020

Closes #468

Merge #559 first, then I will rebase on top.

This is a work in four steps

  1. allow exactly 1 secondary index, no multi-prefix on primary key
  2. allow multiple named secondary indexes, no multi-prefix on primary key
  3. add support for unique indexes
  4. allow multiple named secondary indexes, clean composite key support

All are now done (4th was done in #558 as a side-effect of re-using bucket code)

Update:

As part of an attempt to make ReadOnlyIndexedBucket, I came across the annoyance that Bucket and ReadonlyBucket are different types, but only based on the mutabilty of the storage object. If we didn't store that, we could have one class that just took methods that were either &Storage or &mut Storage. And this would improve composition.

For PrefixedStorage, I see the desire to have the Storage object inside (this is a separate wrapper), but for the more complex cases that do not implement Storage themselves, this seems much overhead. I made PR #559 to implement this (on master, independent of the secondary index work)

@ethanfrey ethanfrey added this to Awaiting review in Blockchain Development via automation Oct 1, 2020
@ethanfrey ethanfrey moved this from Awaiting review to In progress in Blockchain Development Oct 1, 2020
@ethanfrey
Copy link
Member Author

This is in a mostly-working state to show an approach to automatic indexing as a new bucket type. I would love some feedback on the design and API, as well as any implementation improvements you see - this is obviously a rough implementation, but point out any larger issues you see. Ways to simplify, optimize, or missing functionality.

@orkunkl you worked a lot with the weave orm, I'd love your opinion on API design here from that experience

@ethanfrey ethanfrey mentioned this pull request Oct 3, 2020
5 tasks
@ethanfrey ethanfrey mentioned this pull request Oct 3, 2020
5 tasks
@ethanfrey
Copy link
Member Author

After discussion, I will not continue this in cosmwasm, and not modify Bucket, but build the new structures with new names in cosmwasm-plus. We can extend cosmwasm-storage later when they are stable, but for now, let's keep it stable.

Leaving this PR until all logic has been moved to cosmwasm-plus

@ethanfrey ethanfrey added the Blocked Blocked on some external dependency label Oct 5, 2020
@ethanfrey
Copy link
Member Author

This has been done in https://github.com/CosmWasm/cosmwasm-plus in the cw-storage-plus package (in various PRs building on #108)

@ethanfrey ethanfrey closed this Oct 20, 2020
Blockchain Development automation moved this from In progress to Done Oct 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocked Blocked on some external dependency
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Add secondary indexes
1 participant