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: A fully featured btree implementation #3126

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

wyhaines
Copy link
Contributor

This is a fully featured btree implementation. A friend gave me some incomplete (and broken) btree code for Go, and when I started reworking it, I discovered that it was a broken semi-copy of an old version of Google's btree for Go.

I finished reworking it so that it adhere's to that original Google version's API, though there are some differences internally in places, and I think that my version is much easier to follow and to understand.

This implementation is quite a bit faster than the AVL tree. I will add links to some benchmarks that I did in a comment. This implementation supports copy-on-write for the trees, for inexpensively creating copies of a tree that are effectively isolated from each other with respect to changes that happen after the fork.

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests

@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Nov 14, 2024
@n0izn0iz
Copy link
Contributor

Thanks! I need this for that https://github.com/TERITORI/teritori-dapp/blob/2a5c8390438e6aad332f71410bcf8aabaaecab4b/gno/p/havl/havl.gno#L18
:D

@wyhaines wyhaines changed the title A a full features btree implementation A fully featured btree implementation Nov 15, 2024
@wyhaines wyhaines changed the title A fully featured btree implementation feat: A fully featured btree implementation Nov 15, 2024
@salmad3 salmad3 mentioned this pull request Nov 17, 2024
33 tasks
@Kouteki Kouteki added this to the 🚀 Mainnet launch milestone Nov 17, 2024
@Kouteki Kouteki added the in focus Core team is prioritizing this work label Nov 17, 2024
Copy link

codecov bot commented Nov 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@notJoon
Copy link
Member

notJoon commented Nov 19, 2024

related with #3021

@@ -0,0 +1 @@
module gno.land/demo/p/btree
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
module gno.land/demo/p/btree
module gno.land/p/demo/btree

// Any type that implements this interface can be stored in the BTree. This allows considerable
// flexiblity in storage within the BTree.
type Record interface {
// Less compares self to `than`, returning true if self is less than `than`
Copy link
Member

Choose a reason for hiding this comment

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

gno fmt please

@leohhhn
Copy link
Contributor

leohhhn commented Nov 26, 2024

Would be really cool to see a mini tutorial in r/docs about how to use this. Also, a burning question for newbies will be why use this over map or AVL. the r/docs corresponding to this pkg would be a good place to explain this.

@wyhaines
Copy link
Contributor Author

Would be really cool to see a mini tutorial in r/docs about how to use this. Also, a burning question for newbies will be why use this over map or AVL. the r/docs corresponding to this pkg would be a good place to explain this.

I will plan on writing one. Thanks for the suggestion.

Comment on lines +21 to +26
// The btree package implements in-memory B-Trees of arbitrary degree.
//
// It has a flatter structure than an equivalent red-black or other binary tree,
// which may yield better memory usage and/or performance.

package btree
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// The btree package implements in-memory B-Trees of arbitrary degree.
//
// It has a flatter structure than an equivalent red-black or other binary tree,
// which may yield better memory usage and/or performance.
package btree
// Package btree implements in-memory B-Trees of arbitrary degree.
//
// It has a flatter structure than an equivalent red-black or other binary tree,
// which may yield better memory usage and/or performance.
package btree

https://go.dev/doc/comment#package

Copy link
Member

Choose a reason for hiding this comment

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

  • mention that it is a fork of google/btree

// BTree stores Record instances in an ordered structure, allowing easy insertion,
// removal, and iteration.
type BTree struct {
Degree int
Copy link
Member

Choose a reason for hiding this comment

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

Should this be modifiable by a user after creation?

Comment on lines +170 to +177
func New(params ...int) *BTree {
if len(params) == 0 {
return NewWithFreeNodeList(16, NewFreeNodeList(DefaultFreeNodeListSize))
} else if len(params) == 1 {
return NewWithFreeNodeList(params[0], NewFreeNodeList(DefaultFreeNodeListSize))
} else {
return NewWithFreeNodeList(params[0], NewFreeNodeList(params[1]))
}
Copy link
Member

Choose a reason for hiding this comment

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

This constructor is not explicit and is attempting to use variadic arguments for "overloading". This is generally bad practice in Go.

Can we simply make this work in the case for New(), just returning NewWithFreeNodeList(16, NewFreeNodeList(DefaultFreeNodeListSize))?

Comment on lines +1106 to +1107
// TODO: Is there any advantage to implementing this interface for all of the standard
// orderable types?
Copy link
Member

Choose a reason for hiding this comment

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

This can make sense as a subpackage, like p/demo/btree/cmp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in focus Core team is prioritizing this work 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

7 participants