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

fix: improve avl to take an interface as a key #2519

Closed
wants to merge 2 commits into from
Closed

fix: improve avl to take an interface as a key #2519

wants to merge 2 commits into from

Conversation

Er-Sadiq
Copy link

@Er-Sadiq Er-Sadiq commented Jul 6, 2024

This pull request introduces the following changes and fixes issue #2505

Updated Node Structure:

Modified the Node structure to accommodate a generic Key interface for AVL tree keys. This allows the AVL tree to support any type of key instead of being restricted to strings.
Key Interface:

Introduced a Key interface with two methods: LessThan(Key) bool and Equals(Key) bool. These methods allow comparison operations on keys of any type.
Node Methods:

NewNode: Updated to use the generic Key interface.
Get: Updated to use the generic Key interface for searching nodes.
Set: Modified to insert new nodes using the generic Key interface.
setLeaf: Updated to handle leaf nodes with the generic Key interface.
Remove: Adapted to remove nodes based on the generic Key interface.
Traversal Methods:

Updated TraverseInRange, TraverseByOffset, and their helper methods to work with the generic Key interface for traversing nodes within a range or by offset.
Balance and Rotation:

Updated balancing and rotation methods (rotateRight, rotateLeft, balance) to ensure the AVL tree remains balanced after insertions and deletions.

Contributors' checklist... Fix Issue #2505 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 Added references to related issues and PRs

@Er-Sadiq Er-Sadiq requested a review from jaekwon as a code owner July 6, 2024 10:35
@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Jul 6, 2024
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

I understand why you decided to make this an interface; but I think having to implement an interface to use avl makes it much more clunky than it currently is.

I honestly question the entire original issue itself, as:

  • Supporting arbitrary key/values would be trivial with generics (feat(gnovm): add generics #1748) and would allow us to simply have as a key anything that satisfies cmp.Ordered without having to implement an interface
  • proposal: ordered tree-structured maps #1374 is a proposal to bring an avl-like implementation to map, which is by definition of the language a "generic data type", and which would support all ordered data types as a key

So I think this PR as it is, makes general usages of avl.Tree worse, while we still have other prospective language improvements that can then be used to make avl.Trees better.

Personally, the other way I see this PR working, is accepting a interface{} as the key, and then type asserting on all primitive types (strings/integers/floats). Except, of course, that would not work with named types, and can easily cause runtime panics if there are keys of two different types.

@Er-Sadiq
Copy link
Author

Er-Sadiq commented Jul 6, 2024

Hi @thehowl and team,

I apologize for the confusion and the implementation change. I was referencing an article that mentioned Go's support for generics, which led me to explore implementing the avl.Tree with an interface. I now realize that this approach might complicate the usage of the AVL tree.

Based on your feedback, I will revert this PR and create a new fork to implement generics. This will allow us to support arbitrary key types more efficiently without requiring interfaces, aligning better with the language's capabilities and your suggestions.

Thank you for your understanding and patience. I appreciate your guidance on this matter.

Best regards,
Er-Sadiq

@moul
Copy link
Member

moul commented Jul 6, 2024

Please choose a more descriptive title.

@moul moul marked this pull request as draft July 6, 2024 14:25
@Er-Sadiq Er-Sadiq closed this by deleting the head repository Jul 7, 2024
@thehowl thehowl changed the title Main fix: improve avl to take an interface Jul 8, 2024
@thehowl thehowl changed the title fix: improve avl to take an interface fix: improve avl to take an interface as a key Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants