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

depend on new Bitmap interface #11

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

depend on new Bitmap interface #11

wants to merge 2 commits into from

Conversation

mschoch
Copy link
Contributor

@mschoch mschoch commented Feb 17, 2021

this removes dependency on roaring bitmaps

this removes dependency on roaring bitmaps
@mschoch
Copy link
Contributor Author

mschoch commented Feb 17, 2021

OK, so this is the start of a multi-part proposal. The other major parts can be found here:

Zapx changes (would need to be cherry-picked to all other branches as well):
blevesearch/zapx#65

Bleve changes (confined to scorch):
blevesearch/bleve#1557

Why?

  1. The intent of the scorch_segment_api is to allow for innovation and plugging in new implementations. But, the hard dependency on roaring bitmaps meant all implementations had to use that library. By switching to this interface, we focus on the required behaviors, and not the implementation choice.
  2. Having a 3rd-party dependency exposed as part of the API was problematic, as changes in that library can affect our own API promises, and trigger unnecessary semantic version numbering changes.

Questions

Some of the Bitmap methods take other Bitmaps as method parameters, are they expected to work with other implementations? No, as of today we allow for implementations to follow the same rule we use with Segment merging. You are only required to work with your own implementation, and you are expected to panic if this is violated. In summary, all segments in an index must use the same Segment implementation, and all segments must use the same Bitmap implementation.

Status

There are some rough spots in this worth discussing further, but I wanted to float this and get an initial reaction.

Copy link
Member

@abhinavdangeti abhinavdangeti left a comment

Choose a reason for hiding this comment

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

Looks great to me, especially the go.mod.

@mschoch
Copy link
Contributor Author

mschoch commented Feb 18, 2021

@abhinavdangeti I generally agree. To me, the API package and bleve side are almost obvious improvements. The possible downside is some perf hit from type assertions in zapx code, and in one case a new allocation (slice of different type).

Also, I would be interested in feedback on how I rolled the top-level roaring.XYZ methods which return new objects, into the API based on existing objects. I felt it was kind of OK, as it avoids having to go back to the segment plugin to find another method. By kind-of-ok, what I mean is that the 2 ones with 2 arguments, making the method receiver the first argument was natural. However, the HeapOr takes variable args, and so converting it to use the method receiver as one of the arguments was more confusing...

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.

2 participants