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

feat: Add Table-Store (aka ORM) package - Table and Indexable #9751

Merged
merged 35 commits into from
Oct 21, 2021

Conversation

blushi
Copy link
Contributor

@blushi blushi commented Jul 22, 2021

Description

ref: #9237, #9156

This is the first step towards the migration of the Table-Store (ORM) package currently in regen-ledger into the SDK. This won't be exposed as a public API to start with but rather be internal to x/group.

This first PR brings these core concepts:

  • table is the high level object for storage mapper functionality where entities are stored by an unique identifier called RowID
  • Indexable types can be used to setup new tables.

There will be follow-up PRs for adding the following features:

  • table types with auto-incrementing uint64 primary keys and natural primary keys
  • secondary indexes
  • iterator and pagination
  • import/export genesis

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules not applicable
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md not applicable
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@codecov
Copy link

codecov bot commented Jul 22, 2021

Codecov Report

Merging #9751 (7c63cbd) into master (bbe724e) will increase coverage by 0.03%.
The diff coverage is n/a.

❗ Current head 7c63cbd differs from pull request most recent head d2f9a52. Consider uploading reports for the commit d2f9a52 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9751      +/-   ##
==========================================
+ Coverage   64.38%   64.42%   +0.03%     
==========================================
  Files         573      573              
  Lines       54625    54625              
==========================================
+ Hits        35172    35191      +19     
+ Misses      17457    17437      -20     
- Partials     1996     1997       +1     
Impacted Files Coverage Δ
types/errors/errors.go 83.17% <ø> (ø)
crypto/keys/internal/ecdsa/privkey.go 84.21% <0.00%> (+1.75%) ⬆️
x/distribution/simulation/operations.go 90.27% <0.00%> (+9.72%) ⬆️

@orijbot
Copy link

orijbot commented Jul 22, 2021

Visit https://dashboard.github.orijtech.com?pr=9751&repo=cosmos%2Fcosmos-sdk to see benchmark details.

docs/core/store.md Outdated Show resolved Hide resolved
@blushi blushi marked this pull request as ready for review July 22, 2021 12:31
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

I wonder how close this is to what we'll eventually want for ORM in #9156. One option could be to make this internal to x/group if we don't want to start with this as the public API. But maybe there is enough overlap. At least I would like to align on key prefix formats and left a comment about that.

store/table/index_key_codec.go Outdated Show resolved Hide resolved
@aaronc
Copy link
Member

aaronc commented Jul 22, 2021

@fdymylja wonder if you have any thoughts here regarding my comment above ☝️

store/README.md Outdated Show resolved Hide resolved
@blushi blushi requested review from fdymylja and jgimeno July 26, 2021 07:28
Copy link
Contributor

@fdymylja fdymylja left a comment

Choose a reason for hiding this comment

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

Left few comments.

I wonder how close this is to what we'll eventually want for ORM in #9156. One option could be to make this internal to x/group if we don't want to start with this as the public API.

I've checked the group module at regen ledger, and based on how group is using ORM, I think the proposed changes of ORM as per #9156 will be a major disruption at API level. Not necessarily for the worst because the user would need to define a lot less stuff (only the cosmos_proto.table options on protobuf objects) and would get for free the clients to interact with ORM specific objects.

If we think this disruption might be a blocker for future upgrades then I think we should internalize table for now.

At least I would like to align on key prefix formats and left a comment about that.

In this regard I think we should go for [2]byte for object identifier. Then in the futures upgrade if indexes are marked by protobuf object fields we can use the field number as index identifier, then regarding the index value I think length prefixing should work fine for bytes and null termination for strings. Other kinds have fixed lengths (uint64,int64...etc). I think also support for max length index keys makes sense in order to provide ways to defend from spam attacks that might increase store size by a lot.

Regarding where we save index keys: I think we should save index keys (which are pointers to primary keys in the end) in a separate layer that does not get committed into the state commitment store. Although this might be complex to execute and it would break the atomicity promise of the commit layer.

Also some other questions on top of my head right now:

  • How to drop a table? (remove all objects in the namespace)
  • What happens if the model type changes, and I keep using an object prefix which identifies another object namespace?
  • Eventually will we want to support migrations?

store/table/table.go Outdated Show resolved Hide resolved
store/table/table.go Outdated Show resolved Hide resolved
@blushi
Copy link
Contributor Author

blushi commented Jul 29, 2021

Also some other questions on top of my head right now:

  • How to drop a table? (remove all objects in the namespace)

In a follow-up PR, I'll add support for iterators so you could delete all entries in a table using that (see for instance https://github.com/regen-network/regen-ledger/blob/c99dbedd1ff6b5b08ae7da63eec8dd19bff8273e/orm/genesis.go#L78)

  • What happens if the model type changes, and I keep using an object prefix which identifies another object namespace?

Do you mean if we were to change certain fields in the corresponding protobuf definition?

  • Eventually will we want to support migrations?

This relates to the question just above and I guess we would just implement module specific migration to migrate keys and state or do you foresee some more generalized way to handle this at the ORM package level?

@fdymylja
Copy link
Contributor

Note: the following feedback is not a blocker for me, I just want to highlight some areas in which we should work to make ORM more robust and friendly.

Do you mean if we were change certain fields in the corresponding protobuf definition?

Yes, I was imagining a context in which, node is running->node is stopped->protobuf definition of an object is updated. What happens is that prefix 0x1 might contain (this happens is a dev is not careful enough) both v1 and v2 versions of the state object. Then during unmarshalling things are going to fail in an unpredictable way.

There should be a way to version into state somewhere how objects look, and then have an invariance check that triggers on node restart to assert that the namespaces saved in the node's memory match the ones saved in state. Then upgrades to tables should be handled via migrations (this should be straightforward in export->import genesis upgrades, it's not trivial to do for other upgrades types).

In a follow-up PR, I'll add support for iterators so you could delete all entries in a table using that (see for instance https://github.com/regen-network/regen-ledger/blob/c99dbedd1ff6b5b08ae7da63eec8dd19bff8273e/orm/genesis.go#L78)

Maybe it should be part of the Table API, for reasons defined above in which a table drop might be associated with some other logic.

@github-actions github-actions bot removed the C:depinject Issues and PR related to depinject label Oct 8, 2021
@blushi
Copy link
Contributor Author

blushi commented Oct 14, 2021

@fdymylja @jgimeno @robert-zaremba @AmauryM @marbar3778 @aaronc @alexanderbez It'd be great to have another round of reviews so we can move forward with the ORM follow-up PRs, thanks!

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

utACK

Copy link
Collaborator

@robert-zaremba robert-zaremba 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. Added 2 suggestions.

types/errors/errors.go Outdated Show resolved Hide resolved
x/group/internal/orm/table.go Outdated Show resolved Hide resolved
blushi and others added 2 commits October 21, 2021 11:47
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
@blushi blushi added the A:automerge Automatically merge PR once all prerequisites pass. label Oct 21, 2021
@mergify mergify bot merged commit c7a933c into master Oct 21, 2021
@mergify mergify bot deleted the marie/9237-table-store-1 branch October 21, 2021 10:19
mergify bot pushed a commit that referenced this pull request Oct 27, 2021
…aryKeyTable` (#10415)

<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

ref: #9237, #9156

This PR is a follow-up of #9751.
It introduces 2 new public table types: `AutoUInt64Table` and `PrimaryKeyTable` based on the parent `table` struct introduced by #9751.

Upcoming work will include:
- multi-key secondary indexes
- iterator and pagination
- import/export genesis

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [x] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [x] updated the relevant documentation or specification
- [x] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
mergify bot pushed a commit that referenced this pull request Nov 9, 2021
<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

ref: #9237, #9156

This PR is a follow-up of #10415 and #9751.
It adds multi-key secondary indexes, iterator and pagination support.

There will be one last follow-up PR for adding import/export genesis features.

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [x] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [x] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [x] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
mergify bot pushed a commit that referenced this pull request Nov 16, 2021
<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

closes: #9237
ref: #9156

This PR is a follow-up of #10451, #10415 and #9751 and the last PR for adding the ORM package to `x/group`.
It adds import/export genesis features.

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [x] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [x] updated the relevant documentation or specification
- [x] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
blewater pushed a commit to e-money/cosmos-sdk that referenced this pull request Dec 8, 2021
…10451)

<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

ref: cosmos#9237, cosmos#9156

This PR is a follow-up of cosmos#10415 and cosmos#9751.
It adds multi-key secondary indexes, iterator and pagination support.

There will be one last follow-up PR for adding import/export genesis features.

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [x] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [x] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [x] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
blewater pushed a commit to e-money/cosmos-sdk that referenced this pull request Dec 8, 2021
<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

closes: cosmos#9237
ref: cosmos#9156

This PR is a follow-up of cosmos#10451, cosmos#10415 and cosmos#9751 and the last PR for adding the ORM package to `x/group`.
It adds import/export genesis features.

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [x] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [x] updated the relevant documentation or specification
- [x] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants