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

Implement .range() for RedBlackTreeMap/RedBlackTreeSet #23

Closed
FallingSnow opened this issue Apr 3, 2018 · 7 comments
Closed

Implement .range() for RedBlackTreeMap/RedBlackTreeSet #23

FallingSnow opened this issue Apr 3, 2018 · 7 comments
Milestone

Comments

@FallingSnow
Copy link

Is it possible to search RedBlackTreeMap like std's BTreeMap range function?

@orium
Copy link
Owner

orium commented Apr 3, 2018

Not currently, but it is something we want to implement.

@orium orium changed the title RedBlackTreeMap Range Implement .range() for RedBlackTreeMap/RedBlackTreeSet Apr 3, 2018
@FallingSnow
Copy link
Author

Are you open to pull requests?

@orium
Copy link
Owner

orium commented Apr 3, 2018

Sure, PRs are welcomed :)

@FallingSnow
Copy link
Author

So I've been looking into how best to implement the range function. Would it be worth it to add a parent reference to each node? That way we can create a range iterator that will be very optimized.

For example: suppose a tree of the following. If I range from 3..9 (all inclusive) I start an inorder traversal, arrive at 3 and give back an iterator. Now all I need to do on each .next() is continue the inorder traversal until I reach 9.

     5
    /  \
  3      8
 / \    / \
2   4  7   9

That said, this all requires that we add a parent reference to each node. This would offer big savings in very large trees.

@orium
Copy link
Owner

orium commented Apr 7, 2018

I don't think you need the a parent field to do this efficiently. Take a look at how the current iterator works: it maintains a stack that will let you go to the parent node.

Try to modify the current iterator (and the normal iterator just calls the "range" iterator with the range (Unbounded, Unbounded)).

You need to change dig():

fn dig(stack: &mut Vec<&Node<K, V>>, backwards: bool) {
let child = stack.last().and_then(|node| {
let c = if backwards {
&node.right
} else {
&node.left
};
Node::borrow(c)
});
if let Some(c) = child {
stack.push(c);
IterArc::dig(stack, backwards);
}
}

to not go left unless left is within the range.

@jneem
Copy link
Contributor

jneem commented Sep 22, 2018

I've taken a look at this, and it seems there's a choice to be made: if I try to reuse the existing ArcIter implementation then I need to get rid of size_hint, since there's no efficient way to initialize left_index and right_index unless we're iterating over the whole tree. An alternative would be to introduce a new struct (RangeIter say). That would cause some code duplication, though.

Do you have any thoughts?

@orium
Copy link
Owner

orium commented Sep 22, 2018

Good point: it will not be a implement ExactSizeIterator. I think we can have the best of both worlds: we can have a RangeIter that does not override size_hint() and does not implement ExactSizeIterator.

Then we can make Iter just a wrapper around a RangeIter (with range (Unbounded, Unbounded)). Iter will still implement ExactSizeIterator, but it only needs to have logic to maintain {right,left}_index

@orium orium added this to the 0.6.0 milestone Sep 30, 2018
@orium orium closed this as completed in 0ce6c6a Oct 10, 2018
orium added a commit that referenced this issue Oct 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants