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

Dynamic node removal #49

Merged
merged 24 commits into from
Apr 28, 2024
Merged

Conversation

Totodore
Copy link
Contributor

@Totodore Totodore commented Jan 14, 2024

Add a remove fn to the radix tree

For the moment this feature consist only of :

  • Finding the node that need to be removed
  • Once found, if the node has any children, only the value si taken back
  • Otherwise the entire leaf is dropped.

Remaining things to do, are:

  • Testing node removal and ensuring that there is no false positive for the node research
  • Removing parent nodes if they became empty / useless (maybe with backtracking ?)

@ibraheemdev I would like your feedback on the implementation as I don't master radix tree.

Side note: The Display implementation is only for testing purposes and can be gated with a CFG(test) or removed

@Totodore Totodore marked this pull request as draft January 15, 2024 13:01
@Totodore Totodore marked this pull request as ready for review January 15, 2024 15:37
Copy link
Owner

@ibraheemdev ibraheemdev left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this. Looks good in general, just a few nits, and I would prefer more test coverage before merging.

src/tree.rs Outdated Show resolved Hide resolved
src/tree.rs Outdated Show resolved Hide resolved
src/tree.rs Outdated Show resolved Hide resolved
tests/tree.rs Outdated Show resolved Hide resolved
@Totodore Totodore requested a review from ibraheemdev January 19, 2024 17:05
@Totodore
Copy link
Contributor Author

I tried to think about the "node shrinking optimisation" when we remove a node (merging nodes together).
What we could do is having a visited node stack and then unwind the stack and check if nodes can be merged.
My main issue is that it requires to hold multiple mutable references so the rust borrowing rules don't let me do that...

@Totodore
Copy link
Contributor Author

Totodore commented Feb 2, 2024

Hello @ibraheemdev, do you have an idea when you will be able to review the tests I added after your feedback? Thanks! :)

@ibraheemdev
Copy link
Owner

The changes look good, thanks. I'll try to have this merged as part of 0.8, which should be finished soon.

@Totodore Totodore mentioned this pull request Mar 10, 2024
3 tasks
@ibraheemdev
Copy link
Owner

@Totodore I think you need to make sure the denormalized params match before doing a removal. Right now remove("/{x}") will match and remove a route registered as "/{id}". If you can fix that bug and the merge conflicts (the integration test setup was reworked), I think this can be merged.

@Totodore
Copy link
Contributor Author

Totodore commented Mar 12, 2024

@ibraheemdev I have updated the remove fn to work with {x} params. I have two remaining questions:

  • If an error occurs when normalizing the path, it is discarded and None is returned as if the value was not found. I think it is ok to do that but maybe you would prefer to return a Result<Option<T>>?
  • Because the path is normalized and that currently I don't check the original param name provided. If I insert /foo/{bar}/test and that I want to remove it later I can do it with any param name, e.g. /foo/{azd}/test. Should I check the original one when searching the node?

Also I added some tests but I'll try to add some more as soon as possible.

@ibraheemdev
Copy link
Owner

If an error occurs when normalizing the path, it is discarded and None is returned as if the value was not found.

I think that's fine, an invalid route could never be in the router. Maybe add a note in the documentation that the function also returns None for invalid routes.

Should I check the original one when searching the node?

Yes, I think this would be more intuitive behavior. You can just check that the parameter remapping on the final node is equal to the remapping created for the route.

@Totodore
Copy link
Contributor Author

Done :)

@Totodore
Copy link
Contributor Author

Totodore commented Apr 4, 2024

@ibraheemdev I'm currently working on optimizing the tree after a node removal. It consists of two actions:

  • Rewind the branch and check if nodes can be removed (only one child without value). Stop at the first non-removable node.
  • Rewind the branch and check if nodes can be merged together (only one, non-wild child that doesn't have a value).

These two actions are a bit more difficult to implement and might compromise the tree in case of error. Therefore do you think it is necessary to have them for this PR or it is better to implement this later in another one?

@ibraheemdev
Copy link
Owner

@Totodore thanks for continuing to work on this. It looks good for now, I'll take a closer look and try to have it merged this weekend.

@Totodore
Copy link
Contributor Author

Hey @ibraheemdev any knews ? If you don't have the time to review this at the moment, I'll would still be interested to work on these two features:

  • Rewind the branch and check if nodes can be removed (only one child without value). Stop at the first non-removable node.
  • Rewind the branch and check if nodes can be merged together (only one, non-wild child that doesn't have a value).

These two actions are a bit more difficult to implement and might compromise the tree in case of error. Therefore do you think it is necessary to have them for this PR or it is better to implement this later in another one?

@ibraheemdev
Copy link
Owner

This looks great, thanks! Feel free to open a new PR if you are interested in working on this further, but you can just leave an issue for now describing the optimizations.

@ibraheemdev ibraheemdev merged commit 8c236ce into ibraheemdev:master Apr 28, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement Request: Dynamic Route Removal and Update Support in Matchit
2 participants