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

138 - implement first and last. https://github.com/RoaringBitmap/Roar… #148

Merged
merged 5 commits into from Feb 20, 2017

Conversation

Projects
None yet
5 participants
@richardstartin
Contributor

richardstartin commented Feb 17, 2017

…ingBitmap/issues/138

Following on from comments on issue page this implementation adds first() and last() to container implementations. The offset issue is resolved within RoaringArray, which is delegated to by RoaringBitmap. Tests added for a range of values for each container implementation and RoaringBitmap itself.

@lemire

This comment has been minimized.

Show comment
Hide comment
@lemire

lemire Feb 17, 2017

Member

+1

Member

lemire commented Feb 17, 2017

+1

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Feb 17, 2017

Coverage Status

Coverage increased (+0.02%) to 89.875% when pulling b0e6e66 on openkappa:RB138-first-last into d1e35fc on RoaringBitmap:master.

coveralls commented Feb 17, 2017

Coverage Status

Coverage increased (+0.02%) to 89.875% when pulling b0e6e66 on openkappa:RB138-first-last into d1e35fc on RoaringBitmap:master.

@gssiyankai

This comment has been minimized.

Show comment
Hide comment
@gssiyankai

gssiyankai Feb 17, 2017

Member

LGTM

Member

gssiyankai commented Feb 17, 2017

LGTM

@lemire

This comment has been minimized.

Show comment
Hide comment
@lemire

lemire Feb 17, 2017

Member

@gssiyankai Thanks.

This needs to be ported to the buffer package before we can produce a release. Not hard. Mostly copy and paste. I can do it later... or maybe someone wants to jump in and do it... ;-) (It is probably a 5-minute job.)

Member

lemire commented Feb 17, 2017

@gssiyankai Thanks.

This needs to be ported to the buffer package before we can produce a release. Not hard. Mostly copy and paste. I can do it later... or maybe someone wants to jump in and do it... ;-) (It is probably a 5-minute job.)

@richardstartin

This comment has been minimized.

Show comment
Hide comment
@richardstartin

richardstartin Feb 17, 2017

Contributor

I can do it later. I don't actually use the buffer package - is it effectively a carbon copy of the non persistent implementation?

Contributor

richardstartin commented Feb 17, 2017

I can do it later. I don't actually use the buffer package - is it effectively a carbon copy of the non persistent implementation?

@lemire

This comment has been minimized.

Show comment
Hide comment
@lemire

lemire Feb 17, 2017

Member

@openkappa

Yes, it is a carbon copy that works over ByteBuffers instead of arrays. It is annoying that we need to maintain two distinct versions of the same code, but there are significant performance trade-offs involved. The ByteBuffer approach works with off-heap processing... so it can avoid the garbage collection... but it is slower compared to working directly on Java arrays.

Mostly, it is a minor annoyance involving copying and pasting.

Member

lemire commented Feb 17, 2017

@openkappa

Yes, it is a carbon copy that works over ByteBuffers instead of arrays. It is annoying that we need to maintain two distinct versions of the same code, but there are significant performance trade-offs involved. The ByteBuffer approach works with off-heap processing... so it can avoid the garbage collection... but it is slower compared to working directly on Java arrays.

Mostly, it is a minor annoyance involving copying and pasting.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Feb 18, 2017

Coverage Status

Coverage decreased (-0.07%) to 89.782% when pulling c69f0eb on openkappa:RB138-first-last into d1e35fc on RoaringBitmap:master.

coveralls commented Feb 18, 2017

Coverage Status

Coverage decreased (-0.07%) to 89.782% when pulling c69f0eb on openkappa:RB138-first-last into d1e35fc on RoaringBitmap:master.

@richardstartin

This comment has been minimized.

Show comment
Hide comment
@richardstartin

richardstartin Feb 18, 2017

Contributor

Buffer version is much more verbose. The branch coverage is going to decrease with a port unless cases can be contrived where the buffer hasn't got an array. Any thoughts on contriving cases to cover array backed buffers and the converse?

Contributor

richardstartin commented Feb 18, 2017

Buffer version is much more verbose. The branch coverage is going to decrease with a port unless cases can be contrived where the buffer hasn't got an array. Any thoughts on contriving cases to cover array backed buffers and the converse?

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Feb 18, 2017

Coverage Status

Coverage decreased (-0.04%) to 89.814% when pulling b5284f2 on openkappa:RB138-first-last into d1e35fc on RoaringBitmap:master.

coveralls commented Feb 18, 2017

Coverage Status

Coverage decreased (-0.04%) to 89.814% when pulling b5284f2 on openkappa:RB138-first-last into d1e35fc on RoaringBitmap:master.

@Kineolyan

The main concern is about first/last returning 0 as value when the container is empty.
I would suggest throwing, as 0 is a valid value and it is possible to test the size of the container before calling first or last.

throw NoSuchElementException in first and last whenever the container…
… is empty to be consistent with SortedSet
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Feb 19, 2017

Coverage Status

Coverage decreased (-0.03%) to 89.826% when pulling 8d97166 on openkappa:RB138-first-last into d1e35fc on RoaringBitmap:master.

coveralls commented Feb 19, 2017

Coverage Status

Coverage decreased (-0.03%) to 89.826% when pulling 8d97166 on openkappa:RB138-first-last into d1e35fc on RoaringBitmap:master.

@Kineolyan

This comment has been minimized.

Show comment
Hide comment
@Kineolyan

Kineolyan Feb 19, 2017

Contributor

Good job @openkappa

Contributor

Kineolyan commented Feb 19, 2017

Good job @openkappa

reduce cost of emptiness checks for containers, add specific testing …
…of first and last on lazily mutated bitmaps
@richardstartin

This comment has been minimized.

Show comment
Hide comment
@richardstartin

richardstartin Feb 20, 2017

Contributor

The last commit incurred a performance penalty for run containers. I've fixed that and added explicit tests for cases where the bitmap has been lazily mutated.

Contributor

richardstartin commented Feb 20, 2017

The last commit incurred a performance penalty for run containers. I've fixed that and added explicit tests for cases where the bitmap has been lazily mutated.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Feb 20, 2017

Coverage Status

Coverage decreased (-0.03%) to 89.826% when pulling 966004d on openkappa:RB138-first-last into d1e35fc on RoaringBitmap:master.

coveralls commented Feb 20, 2017

Coverage Status

Coverage decreased (-0.03%) to 89.826% when pulling 966004d on openkappa:RB138-first-last into d1e35fc on RoaringBitmap:master.

@lemire

This comment has been minimized.

Show comment
Hide comment
@lemire

lemire Feb 20, 2017

Member

LGTM.

Member

lemire commented Feb 20, 2017

LGTM.

@lemire lemire merged commit ee897ac into RoaringBitmap:master Feb 20, 2017

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.03%) to 89.826%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@richardstartin richardstartin deleted the richardstartin:RB138-first-last branch Feb 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment