feat/btree: add BTree with scale

[kanishkatn]

Changes

Adds BTree with scale encoding and decoding apis by creating a wrapper around the tidwall's library.

Tests

go test pkg/scale

Issues

Closes #3480

Primary Reviewer

@timwu20

[codecov]

Codecov Report

Attention: 46 lines in your changes are missing coverage. Please review.

Comparison is base (619be1b) 50.18% compared to head (5615c8a) 50.21%.
Report is 24 commits behind head on development.

Additional details and impacted files

@@               Coverage Diff               @@
##           development    #3536      +/-   ##
===============================================
+ Coverage        50.18%   50.21%   +0.02%     
===============================================
  Files              229      230       +1     
  Lines            28595    28692      +97     
===============================================
+ Hits             14350    14407      +57     
- Misses           12721    12748      +27     
- Partials          1524     1537      +13     

[edwardmack]

I'm concerned about adding dependency on "github.com/tidwall/btree" to scale package, I think they is a way to define scale encoding/decoding outside of the scale package.

[timwu20]

I'm curious as to where we need the btree.BTree? We utilize btree.Map pretty extensively in feat/grandpa.

[kanishkatn]

I'm curious as to where we need the btree.BTree? We utilize btree.Map pretty extensively in feat/grandpa.

Have you seen #3314?

[kanishkatn]

I'm concerned about adding dependency on "github.com/tidwall/btree" to scale package, I think they is a way to define scale encoding/decoding outside of the scale package.

I looked into moving BTree outside the scale package; in order to do that, we'll have to export encodeState, decodeState and fieldScaleIndicesCache.

Could you elaborate on your concerns?

[edwardmack]

I'm concerned about adding dependency on "github.com/tidwall/btree" to scale package, I think they is a way to define scale encoding/decoding outside of the scale package.

I looked into moving BTree outside the scale package; in order to do that, we'll have to export encodeState, decodeState and fieldScaleIndicesCache.

Could you elaborate on your concerns?

It's just that this is the first time (that I'm aware of) that we've added outside packages to the scale package, so that piqued my curiosity. I was more curious than concerned and now I have a better understanding.

[jimjbrettj]

I'm curious as to where we need the btree.BTree? We utilize btree.Map pretty extensively in feat/grandpa.

Have you seen #3314?

@kanishkatn Could you please elaborate more on this? While I see that you use the btree.BTree I do not see any scale interactions with respect to btree.BTree so I am still left with the question of where we need scale to have btree.BTree support.

[kanishkatn]

I'm curious as to where we need the btree.BTree? We utilize btree.Map pretty extensively in feat/grandpa.

Have you seen #3314?

@kanishkatn Could you please elaborate more on this? While I see that you use the btree.BTree I do not see any scale interactions with respect to btree.BTree so I am still left with the question of where we need scale to have btree.BTree support.

Here's another one: #3344. It has a custom encoding right now; I might look into changing it once I implement pebbleDB.
I think that's about it since I recently switched to the Map in a few structs.

I hope that helps!

[jimjbrettj]

Looks good to me, just have some questions and nits

[jimjbrettj]

I am going to change the primary reviewer of this PR to @timwu20. Given that Tim is the author of this package and has the most knowledge of it I would like to double-check with him before merging this in.

[timwu20]

I think we should take a different approach to supporting the tidwall/btree package. I propose implementing custom unmarshalling/marshalling support in pkg/scale (#3558) first, and then creating a thin wrapper around the tidwall/btree package that implements the Unmarshaller and Marshaller interfaces for the types that we need. We should make it clear that we're just doing this to support SCALE for the btree types. This should also reduce the changes required within scale to support the btree types.

[kanishkatn]

I've updated this to use the changes from #3617

[timwu20]

Why do we need to hold *btree.Map in a struct with the Degree? I was expecting this Map to just be btree.Map which reflects the same interface with the added MarshalSCALE and UnmarshalSCALE methods. I took a stab at what I was expecting in this branch.

[kanishkatn]

It is so that the user can set the degree beforehand and not worry about filtering the data after unmarshal.

[kanishkatn]

Let me rephrase. It gives the option for the user to set the Degree. One usecase that I see it supporting is, if there are 100 entries in the db and I care about only top 10, I can set that during the creation.

The usecase leads to lesser memory usage.

[kanishkatn]

But yeah, I guess I don't need to store it.

I'm not familiar with your implementation style, feels like a lot of repetitive code.

[timwu20]

Not sure why you need this attribute given it's not used in any of the MarshalSCALE or UnmarshalSCALE methods.

[kanishkatn]

Please find the follow up questions below to get a better understanding.

Do you mean the comparator? Is the comment not descriptive enough?