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

WIP: refactor AVL #450

Merged
merged 4 commits into from
Jan 10, 2023
Merged

WIP: refactor AVL #450

merged 4 commits into from
Jan 10, 2023

Conversation

jaekwon
Copy link
Contributor

@jaekwon jaekwon commented Jan 6, 2023

This PR makes AVL more intuitive to use.

  • Tree -> Node; MuTTree -> Tree. // Usually there is something called a "node".
  • An empty Tree struct can be used as zero, as long as it is addressable/modifiable.
  • Replaced usage of Node with Tree in demo/boards.

@jaekwon
Copy link
Contributor Author

jaekwon commented Jan 6, 2023

I'm wondering if we can do better than having Iterate() methods take *avl.Node as the parameter type.
I'm not sure it would help with portability any better.
Generics might help if we can declare a arbitrary type iterator, but Gno won't support generics yet.

Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I'll try to find better/alternative solutions for the iterate, but right now it looks great to me.

@moul moul merged commit f94e1c4 into master Jan 10, 2023
@moul moul deleted the dev/jae/avl_refactor branch January 10, 2023 16:37
@moul
Copy link
Member

moul commented Jan 10, 2023

Thinking about these ways:

for _, item := tree.Items() {
  _ = item.Key()
  _ = item.Value()
}
for key := tree.Keys() {
   item := tree.ByKey(key)
}
cur := tree.Root()
for item := tree.Root(); item != nil; {
    _ = item.Key()
   _ = item.Value()
}

@jaekwon
Copy link
Contributor Author

jaekwon commented Jan 10, 2023

regarding for _, item := tree.Items() that would be inefficient for large number of items as golang's for doesn't support iterators. But that's easy enough to modify to use iterators, like for itr:=tree.Iterator(...); !itr.Done(); itr.Next() or similar. But even this requires a common interface for iteration that must be imported as an import.

Another option is to replace type Node struct with type NodeI interface as in func (node *Node) Iterate(start, end string, cb func(NodeI) bool) bool, so it still takes a callback rather than returns a new object.

But I think best would be to implement the interface we already have:

pkgs/db/types.go ("tendermint")
pkgs/store/types.go ("sdk")

both use this iterator type. Domain() maybe can panic or be optional.

type Iterator interface {
	// The start & end (exclusive) limits to iterate over.
	// If end < start, then the Iterator goes in reverse order.
	//
	// A domain of ([]byte{12, 13}, []byte{12, 14}) will iterate
	// over anything with the prefix []byte{12, 13}.
	//
	// The smallest key is the empty byte array []byte{} - see BeginningKey().
	// The largest key is the nil byte array []byte(nil) - see EndingKey().
	// CONTRACT: start, end readonly []byte
	Domain() (start []byte, end []byte)

	// Valid returns whether the current position is valid.
	// Once invalid, an Iterator is forever invalid.
	Valid() bool

	// Next moves the iterator to the next sequential key in the database, as
	// defined by order of iteration.
	//
	// If Valid returns false, this method will panic.
	Next()

	// Key returns the key of the cursor.
	// If Valid returns false, this method will panic.
	// CONTRACT: key readonly []byte
	Key() (key []byte)

	// Value returns the value of the cursor.
	// If Valid returns false, this method will panic.
	// CONTRACT: value readonly []byte
	Value() (value []byte)

	// Close releases the Iterator.
	Close()
}

Usage is like:

itr := ...
defer itr.Close()
for ; itr.Valid(); itr.Next() { ... }

--

Recently there had been discussions about a standard iterator interface: golang/go#54245 ; but the above requires generics so it is yet out of scope.

Random tangent on generics: It would be nice if we could keep the Iterate(...,cb func(*Node) bool) implementation as is, and be able to assign var x NewInterface = (*Node)(xx); where NewInterface lists Iterate(...,cb func(NodeI) bool) as method signature, but this isn't solved by Go's generics, is it?

@moul moul mentioned this pull request Jan 11, 2023
@moul moul added this to the 🚀 main.gno.land (required) milestone Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants