[6.0] Fix semantics of AvlTree.insert & new AvlTree.insertOrUpdate method #1038
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aslesarenko
approved these changes
Dec 24, 2024
| * | ||
| * @param key key to look up | ||
| * @param value value to check it was inserted or updated | ||
| * @return Success(Some(value)), Success(None), or Failure |
Member
There was a problem hiding this comment.
Make sense to describe each case of returned result.
| def insertOrUpdate( | ||
| entries: Coll[(Coll[Byte], Coll[Byte])], | ||
| proof: Coll[Byte]): Option[AvlTree] = { | ||
| if (!tree.isInsertAllowed || !tree.isUpdateAllowed) { |
Member
There was a problem hiding this comment.
Both flags are required, however for each given tree it will either be insert or update (but not both).
Is it possible to make it more flexible and require the flag only if the corresponding operation is happening for a given tree.
Member
Author
There was a problem hiding this comment.
" for each given tree it will either be insert or update (but not both)" - no, it could be insert for one key and update for another
| addSeqCost(InsertIntoAvlTree_Info, nItems) { () => | ||
| val insertRes = bv.performInsert(key.toArray, value.toArray) | ||
| // TODO v6.0: throwing exception is not consistent with update semantics | ||
| // however it preserves v4.0 semantics (see https://github.com/ScorexFoundation/sigmastate-interpreter/issues/908) |
| operations.forall { case (key, value) => | ||
| var res = true | ||
| // the cost of tree update is O(bv.treeHeight) | ||
| addSeqCost(UpdateAvlTree_Info, nItems) { () => |
Member
There was a problem hiding this comment.
Suggested change
| addSeqCost(UpdateAvlTree_Info, nItems) { () => | |
| // Here (and in the previous methods) the cost is not properly approximated. | |
| // When the tree is small (or empty), but there are many `operations`, the treeHeight will grow on every iteration. | |
| // So should the cost on every iteration. | |
| addSeqCost(UpdateAvlTree_Info, nItems) { () => |
…TreeEvaluator.scala Co-authored-by: Alexander Slesarenko <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
close #908
In this PR, semantics of AvlTree.insert fixed, and new AvlTree.insertOrUpdate method added