opt(iterator): Optimise the iterator and introduce range iterators #19
Conversation
This reverts commit 18bcc50.
There was a problem hiding this comment.
Looks good. But, the code is hard to follow. Add comments / improve the variable names, perhaps.
I'll review it once again after that.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ahsanbarkati)
iterator.go, line 71 at r1 (raw file):
} key := it.keys[2*it.kidx] off := it.keys[2*it.kidx+1]
Add comments. This code isn't the easiest to follow.
There was a problem hiding this comment.
Reviewable status: 1 of 2 files reviewed, 5 unresolved discussions (waiting on @ahsanbarkati and @manishrjain)
iterator.go, line 31 at r2 (raw file):
cidx int bmHiIdx int // the index of current uint16 in the bitmap container, which is a list of uint16s
why not bitmapIdx? What does Hi imply? Is there a corresponding low index?
iterator.go, line 73 at r2 (raw file):
// it.keys is a list of the form [key1, offset1, key2, offset2 ... ] // the kidx'th key will be present at index 2*it.kidx and the offset corresponding to it will be // ont index after after it.
Can you fix this? ont index after after it
Also, why not just increment it.kidx by 2? Then, you can say
key := it.keys[it.keyIdx]; off := it.keys[it.keyIdx+1]
Easier to read.
iterator.go, line 85 at r2 (raw file):
return 0 } it.kidx++
it.keyIdx += 2
iterator.go, line 103 at r2 (raw file):
// A bitmap container is an array of uint16s. // If the container is bitmap, go to the index which has a non-zero value. for it.bitset == 0 && it.bmHiIdx+1 < len(cont[startIdx:]) {
bmHiIdx
There was a problem hiding this comment.
Reviewable status: 1 of 2 files reviewed, 5 unresolved discussions (waiting on @manishrjain)
iterator.go, line 71 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Add comments. This code isn't the easiest to follow.
Done.
iterator.go, line 31 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
why not bitmapIdx? What does Hi imply? Is there a corresponding low index?
Done.
iterator.go, line 73 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Can you fix this?
ont index after after itAlso, why not just increment it.kidx by 2? Then, you can say
key := it.keys[it.keyIdx]; off := it.keys[it.keyIdx+1]Easier to read.
Done.
iterator.go, line 85 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
it.keyIdx += 2
Done.
iterator.go, line 103 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
bmHiIdx
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ahsanbarkati)
|
@ahsanbarkati Sorry for resurrecting this but I'm doing upgrade work atm and am struggling to understand how to implement a reverse iterator on the latest release. Can I use the new range iterator for this purpose? Or do I need to do a If handier, I have opened up https://discuss.dgraph.io/t/how-to-implement-a-reverse-roaring-bitmap-iterator-sroar-bitmap-newreverseiterator-went-away/17901 👍 |
This PR improves the iterator by doing in-place iteration. It also introduces range iterators.
This change is