AVLTree Implementation #118
Conversation
…ing keys() and entries() methods.
| * @param end Key for upper bound (exclusive). | ||
| * @returns Range of entries corresponding to keys within start and end bounds. | ||
| */ | ||
| entries(start: K, end: K): MapEntry<K, V>[] { |
There was a problem hiding this comment.
| entries(start: K, end: K): MapEntry<K, V>[] { | |
| entries(start: K = this.min(), end: K this.max()): MapEntry<K, V>[] { |
After writing this, I think we should make the end of the range be inclusive so that by default this will include the max key. Same goes for values and keys.
There was a problem hiding this comment.
Ah yes I had the same initial approach, but unfortunately it results in an unexpected behavior where calling entries() will throw instead of returning an empty array when there are no keys in the tree. This happens because this.min() (and this.max()) throws if the tree is empty.
…ernal avl tree methods private
| @@ -0,0 +1,46 @@ | |||
| import { AVLTree } from "../runtime"; | |||
|
|
|||
There was a problem hiding this comment.
Any top level code is executed at the start before the contract methods are called. So you can just define it at the top level once. However, I've found that it's best to import it from another file so that if you wanted to unit test this, the test could also import the tree. We currently have an issue with unit testing, where even though we wipe away the storage and state of the Mock VM that handles the env functions, we don't wipe away the state of the contract. This means you need access to the same tree object as the "contract" file so you can clear it. It's fine in your case because since the trees are not in the global scope they won't be used again.
There was a problem hiding this comment.
I think I understand. Am I correct in thinking that it's imperative that unit tests and contracts use distinct prefixes when creating a collection (e.g. AVLTree)?
There was a problem hiding this comment.
The issue is that the persistent collections usually have a length or size field, which when not set reads from the storage, and when the storage is refreshed but the local collection isn't it'll have the incorrect size. So reconstructing the collection before each test ensures that the length is uninitialized and will agree with the storage. If you were to make a unit test that tests your avlTreeContract, since it's reconstructed inside each function you're fine. However, if it were a global variable that is initialized when the contract starts, this initialization will only happen once for the unit tests and since you don't have access to the tree from the unit test file, it can't change it. In this case it's best to have a third file that exports the tree, this way after each unit test you can clear the tree.
There was a problem hiding this comment.
Furthermore, since this contract isn't tested with unit tests, it's fine to just use a global variable for the tree.
There was a problem hiding this comment.
Ah I see! Changes you made make sense.
|
I made some changes to the API so that |
There was a problem hiding this comment.
After playing around with it and looking at the rust example. The last thing to do (and it should be quick) is to add a third optional parameter for any of the range methods. e.g.
keys(start: K, end: K, inclusive: boolean = false) {This way users who want to get all the keys can call,map.keys(map.min(), map.max(), true)
All and all great job and glad you could use the simulation testing!
|
Thanks for all your help! Changes for having inclusive end bounds are pushed. Let me know if there's anything else and I will get to it asap. |
Resolves #112