Modified the implementation of ReadFrom to support roaring 64 to directly read roaring 32 data. #290
Conversation
|
This pull request introduces 1 alert when merging ed067d5 into 3700649 - view on LGTM.com new alerts:
|
|
Good feature. +1 |
| @@ -67,11 +67,11 @@ func (rb *Bitmap) WriteToMsgpack(stream io.Writer) (int64, error) { | |||
| // The format is compatible with other RoaringBitmap | |||
| // implementations (Java, C) and is documented here: | |||
| // https://github.com/RoaringBitmap/RoaringFormatSpec | |||
| func (rb *Bitmap) ReadFrom(reader io.Reader) (p int64, err error) { | |||
| func (rb *Bitmap) ReadFrom(reader io.Reader, cookieHeader ...byte) (p int64, err error) { | |||
There was a problem hiding this comment.
Could you document the new parameter cookieHeader ...byte?
|
This pull request introduces 1 alert when merging 7498b11 into 3700649 - view on LGTM.com new alerts:
|
|
@guymolinari Would you review it? I just looked at it superficially. It might be useful to have someone think it through. |
|
Will do.
…On Mon, Nov 2, 2020, 5:36 PM Daniel Lemire ***@***.***> wrote:
@guymolinari <https://github.com/guymolinari> Would you review it? I just
looked at it superficially. It might be useful to have someone think it
through.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#290 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADZZAUQUUPQJWUSNBRWF3KLSN5NBXANCNFSM4THIBQRA>
.
|
|
I have no concerns with the general idea here, but it does seem to ugly-up some methods in ways I'd typically rather avoid. For instance, From what I can read, it basically is injecting some values from |
|
All good thoughts. Stepping back, I would suggest that we also consider
compatibility with the other Roaring library implementations. This is a
stated capability in that the wire protocol is specifically supposed to be
cross platform. I have concerns about the existing 64 bit implementation
not being compatible with the C/C++ 64 bit implementation. I think we
should have specific test cases verifying that we can seamlessly
interoperate.
Sorry for broadening the scope on this folks but I really think we should
do this the right way.
Guy
…On Mon, Nov 2, 2020 at 11:33 PM Jackso Newhouse ***@***.***> wrote:
I have no concerns with the general idea here, but it does seem to ugly-up
some methods in ways I'd typically rather avoid. For instance, func (ra
*roaringArray) readFrom(stream byteInput, cookieHeader ...byte) (int64,
error) is a much uglier method signature than func (ra *roaringArray)
readFrom(stream byteInput) (int64, error).
From what I can read, it basically is injecting some values from
cookieHeader that would normally be read from the byteInput. How possible
would it be to instead factor out all of the reading below the changes, and
then have two separate pathways. Something like func (ra *roaringArray)
readWithhHeaders(stream byteInput, format formatType) where formatType is
a type encompassing the possible headers. That way we could avoid having
logic in 32 bit roaring that is just there to support 64 bit roaring.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#290 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADZZAUV5NJYAMR7THUYPIYLSN6W3JANCNFSM4THIBQRA>
.
|
|
Broadening the scope should be done with care in the sense that we should not ask of a single PR what it cannot provide. 64-bit is a bit of a mess in the sense that there is no cross-language compatibility (at all). You have one implementation in C++, two implementations in Java (both in the RoaringBitmap library), you have one in roaring. People have said good things about the more recent Java 64-bit implementation and it is definitively being used in production. What one would need to do is to sit down, look and them, and propose a standard format that everyone could agree to. It is not hard as in "curing cancer", but it requires a deliberate effort. |
|
I can test this out as is then. Sometimes good is the enemy of perfect.
…On Tue, Nov 3, 2020, 11:51 AM Daniel Lemire ***@***.***> wrote:
Broadening the scope should be done with care in the sense that we should
not ask of a single PR what it cannot provide.
64-bit is a bit of a mess in the sense that there is *no* cross-language
compatibility (at all). You have one implementation in C++, *two*
implementations in Java (both in the RoaringBitmap library), you have one
in roaring. People have said good things about the more recent Java 64-bit
implementation and it is definitively being used in production.
What one would need to do is to sit down, look and them, and propose a
standard format that everyone could agree to. It is not hard as in "curing
cancer", but it requires a deliberate effort.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#290 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADZZAUQ4QAFG5V3GHOTLFBDSOBNLZANCNFSM4THIBQRA>
.
|
|
@guymolinari Another thoughts regarding this PR? I think we need to either accept it, or make some concrete recommendations to @davidchen-cn I think that @jacksonrnewhouse made some good points about how this PR does make things "uglier" at times. But I think we are a bit too vague about the fix... |
|
@guymolinari , hello. do you have any other comments for this PR? |
|
I would just merge it. We could refactor later if needed.
…On Sun, Dec 13, 2020, 11:33 PM Alex Sharov ***@***.***> wrote:
@guymolinari <https://github.com/guymolinari> , hello. do you have any
other comments for this PR?
Theoretically can move all "container" types from 32 and 64 packages to
"internal" package, but it maybe a big move.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#290 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADZZAUTNB4SXS47BMETF2UDSUW5S5ANCNFSM4THIBQRA>
.
|
|
@davidchen-cn Would you sync with our main branch? Sorry about this, but it seems that there were changes and the two versions now differ. |
|
@davidchen-cn Ping! Would you sync your fork? |
|
@lemire Sorry for reply late. I will sync it soon. |
|
Merging. To be revised later. |
Hi reviewer, I think this can help roaring32 users to smoothly migrate to roaring64.