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

Non panicking push method #99

Merged
merged 1 commit into from
Apr 29, 2021

Conversation

MarinPostma
Copy link
Contributor

@MarinPostma MarinPostma commented Apr 28, 2021

Changes the push method to make it not panic when inserting a value that is not greater than the previous. Also make some optimizations that make the push method roughly 20% faster than before.

@MarinPostma MarinPostma changed the title Better push method Non panicking push method Apr 28, 2021
@MarinPostma MarinPostma marked this pull request as ready for review April 28, 2021 10:13
@Kerollmops Kerollmops added the breaking This change will require a bump of the minor or major version. label Apr 28, 2021
@Kerollmops
Copy link
Member

Kerollmops commented Apr 28, 2021

Thank you very much 😺

bors try

bors bot added a commit that referenced this pull request Apr 28, 2021
@bors
Copy link
Contributor

bors bot commented Apr 28, 2021

try

Build failed:

@MarinPostma
Copy link
Contributor Author

sorry, forgot to run fmt. fixed now :)

@Kerollmops
Copy link
Member

Thank you 🧷,

However, I will not be able to merge it before #97, as this is a breaking PR, and #97 is not.
I would like to release a 0.6.7 for #97 and then a 0.7.0 for this one.

bors try

bors bot added a commit that referenced this pull request Apr 28, 2021
@bors
Copy link
Contributor

bors bot commented Apr 28, 2021

try

Build failed:

Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

Could you please fixup (with only one commit message) the first three commits, please?
Keep the clippy fixes in one separate commit.

@bors
Copy link
Contributor

bors bot commented Apr 29, 2021

👊

@MarinPostma MarinPostma force-pushed the better-push-method branch 3 times, most recently from fcac350 to 0fa6be0 Compare April 29, 2021 14:56
@MarinPostma MarinPostma marked this pull request as draft April 29, 2021 14:57
@MarinPostma MarinPostma force-pushed the better-push-method branch 4 times, most recently from d2085ac to 1d7088e Compare April 29, 2021 15:32
optimize push and fix correctness

min returns option
@MarinPostma MarinPostma marked this pull request as ready for review April 29, 2021 15:33
Copy link
Member

@Kerollmops Kerollmops 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, LGTM!

bors merge

@bors
Copy link
Contributor

bors bot commented Apr 29, 2021

Build succeeded:

@bors bors bot merged commit 1828d46 into RoaringBitmap:master Apr 29, 2021
bors bot added a commit that referenced this pull request Oct 21, 2021
106: Modify the from_sorted_iter and append methods to return a Result r=Kerollmops a=Kerollmops

This PR fixes #103, which means that the `from_sorted_iter` and `append` methods of the `RoaringBit/Treemap` types now return a `Result`. The `Err` value corresponds to the number of values that we appended to the set until we found an erroneous value, it eases debugging.

These two methods were containing bugs in the sense that they were silently discarding invalid values instead of panicking, this bug was introduced by #99 which introduced a non-panicking push method.

A possible improvement could be to introduce a new error type, something like a `NonSortedIntegers` struct with a `stopped_at`/`valid_until` method that returns the `u64` we currently return as the `Err`.

`@MarinPostma` could you please take a look at this PR? If you got the time?

Co-authored-by: Kerollmops <clement@meilisearch.com>
Co-authored-by: Clément Renault <renault.cle@gmail.com>
not-jan pushed a commit to not-jan/roaring-rs that referenced this pull request Aug 31, 2022
99: Non panicking push method r=Kerollmops a=MarinPostma

Changes the `push` method to make it not panic when inserting a value that is not greater than the previous. Also make some optimizations that make the push method roughly 20% faster than before.


Co-authored-by: Clément Renault <clement@meilisearch.com>
not-jan pushed a commit to not-jan/roaring-rs that referenced this pull request Aug 31, 2022
106: Modify the from_sorted_iter and append methods to return a Result r=Kerollmops a=Kerollmops

This PR fixes RoaringBitmap#103, which means that the `from_sorted_iter` and `append` methods of the `RoaringBit/Treemap` types now return a `Result`. The `Err` value corresponds to the number of values that we appended to the set until we found an erroneous value, it eases debugging.

These two methods were containing bugs in the sense that they were silently discarding invalid values instead of panicking, this bug was introduced by RoaringBitmap#99 which introduced a non-panicking push method.

A possible improvement could be to introduce a new error type, something like a `NonSortedIntegers` struct with a `stopped_at`/`valid_until` method that returns the `u64` we currently return as the `Err`.

`@MarinPostma` could you please take a look at this PR? If you got the time?

Co-authored-by: Kerollmops <clement@meilisearch.com>
Co-authored-by: Clément Renault <renault.cle@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will require a bump of the minor or major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants