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

Run containers part 2 #66

Draft
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

Kerollmops
Copy link
Member

I started working on the run containers implementation, my starting point is #56 by @josephglanville.

I fixed some bugs but I'm sure I introduced new ones with the operations on runs, I need to add a lot of new tests for runs.

@josephglanville
Copy link
Contributor

Awesome! I don't have much time to help atm but will review and comment where I can. :)

.map(|iv| iv.start..iv.end)
.flatten()
.collect(),
intervals.iter().flat_map(|iv| iv.start..=iv.end).collect()
Copy link
Contributor

Choose a reason for hiding this comment

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

Doh. Good catch.

@josephglanville
Copy link
Contributor

I'm going to start picking this back up @Kerollmops. I noticed the remove_range implementation is most likely not working yet which is preventing the new tests I have added from passing so I will focus on fixing that up and adding the remaining run container store operations. I will open a PR against your branch once I have some progress to show.

@Kerollmops
Copy link
Member Author

Thanks a lot @josephglanville, indeed it seems like I didn't take enough time to make this method work. I hope you fix it, and wait for your PR, thank you 😄

@saik0
Copy link
Contributor

saik0 commented Feb 11, 2022

@josephglanville I was taking a look through history in #12

I think storing start, end will result in slightly nicer code vs storing start, run_length.
Serialisation will of course follow the format specification

I haven't made an opportunity to look through the code for this PR yet. Is that what were doing?

My concern is if we deviate we wont be able to use the same "frozen" format for lazy containers

@gcxfd
Copy link

gcxfd commented Mar 3, 2022

when use insert_range in a empty RoaringBitmap , maybe can use RunContainer ?

@josephglanville
Copy link
Contributor

@saik0 yeah that is a valid concern and not something I had thought of at the time but think it makes sense as to make it easier to support mmaped bitmaps without needing separate code for them. I don't think the code is too hard to modify to use start + len instead just have to adjust when searching for intervals etc.

If you want to work on this go ahead, my progress has been slow because I am (as usual) occupied with other things sadly.

@tgruben
Copy link
Member

tgruben commented Dec 22, 2023

I'm unclear what's needed to finish this, is there anything I can do to help?

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.

None yet

5 participants