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

R4R: Swap start/end in ReverseIterator #2913

Merged
merged 3 commits into from Nov 28, 2018
Merged

Conversation

jaekwon
Copy link
Contributor

@jaekwon jaekwon commented Nov 26, 2018

This makes dbm.DB.ReverseIterator have the same API as the cosmos sdk's store.Store.ReverseIterator (they share the same interface, as that was the intent).

It turns out that the CosmosSDK's store.Store.ReverseIterator API as currently implemented is both different and better than what we have now in tendermint/libs/db.

  • In the CosmosSDK's version, start/end have the same semantics as the forward Iterator.
  • This not only makes it easier to switch between forward and reverse iteration given the same range, but it also makes it possible to reverse-iterate on a prefix easily, whereas the current tendermint/libs/db.DB.ReverseIterator requires more finangling to make it iterate over a prefix, namely skipping the first item. This is reflected in the code change here as code simplification.
  • The CosmosSDK's version still allows for easy inclusive-end iteration, as all you need to do is append a 0x00 byte.

With this PR, the CosmosSDK can use the tendermint/libs/db.DB as a store.Store using the db adapter in the SDK, which is needed for faster simulation (as a non-production drop-in replacement for the IAVL store in cosmos-sdk/store.IAVLStore).

See also:


  • [n/a] Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

@melekes
Copy link
Contributor

melekes commented Nov 26, 2018

libs/db/util.go:40:1:warning: cpDecr is unused (deadcode)

Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

libs/db/types.go Outdated Show resolved Hide resolved
@codecov-io
Copy link

Codecov Report

Merging #2913 into develop will decrease coverage by 0.07%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #2913      +/-   ##
===========================================
- Coverage    62.15%   62.08%   -0.08%     
===========================================
  Files          212      212              
  Lines        17369    17255     -114     
===========================================
- Hits         10796    10712      -84     
+ Misses        5663     5639      -24     
+ Partials       910      904       -6
Impacted Files Coverage Δ
libs/db/types.go 100% <ø> (ø) ⬆️
lite/dbprovider.go 77.55% <100%> (ø) ⬆️
libs/db/util.go 72.72% <100%> (+1.99%) ⬆️
libs/db/go_level_db.go 66.87% <100%> (+0.2%) ⬆️
libs/db/mem_db.go 82.9% <100%> (ø) ⬆️
libs/db/fsdb.go 69.78% <100%> (ø) ⬆️
libs/db/prefix_db.go 56.32% <100%> (-1.41%) ⬇️
rpc/lib/types/types.go 29.16% <0%> (-8.24%) ⬇️
evidence/pool.go 52.08% <0%> (-4.06%) ⬇️
node/node.go 63.35% <0%> (-3.24%) ⬇️
... and 11 more

@melekes melekes added the T:breaking Type: Breaking Change label Nov 27, 2018
var end = itr.end
var key = itr.source.Key()
if itr.isReverse {
if end != nil && bytes.Compare(key, end) <= 0 {
if start != nil && bytes.Compare(key, start) < 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

don't think we need the start != nil check here anymore, but doesn't hurt

@@ -310,7 +296,6 @@ func (itr *prefixIterator) Next() {
}
itr.source.Next()
if !itr.source.Valid() || !bytes.HasPrefix(itr.source.Key(), itr.prefix) {
itr.source.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

Was there some reason for this to be here in the first place ?

@ebuchman ebuchman merged commit 416d143 into develop Nov 28, 2018
@ebuchman ebuchman deleted the jae/reverseiterator branch November 28, 2018 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T:breaking Type: Breaking Change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants