Remove Lucene's PackedInt dependency from Cuckoo filter #74736
Conversation
elasticmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
romseygeek
left a comment
romseygeek
left a comment
There was a problem hiding this comment.
I think we're still relying on API code in lucene that won't be present in v9 so the mixed-cluster branches won't work here?
| public void readBytes(byte[] b, int offset, int len) throws IOException { | ||
| in.readBytes(b, offset, len); | ||
| if (in.getVersion().before(Version.V_8_0_0)) { | ||
| final PackedInts.Reader reader = PackedInts.getReader(new DataInput() { |
There was a problem hiding this comment.
PackedInts.getReader() doesn't exist in lucene 9, so is the plan to remove this further in a follow-up or do we need to rework it here?
There was a problem hiding this comment.
This will be removed once we backport the change to 7.x
| mutable.set(i, data.get(i)); | ||
| } | ||
| }); | ||
| mutable.save(new DataOutput() { |
There was a problem hiding this comment.
Similarly, save() is no longer in the lucene 9 API
There was a problem hiding this comment.
This will be removed once we backport the change to 7.x
|
@romseygeek true but this is the first step. Once we move this to 7.x, we can remove the dependency in master? |
romseygeek
left a comment
romseygeek
left a comment
There was a problem hiding this comment.
Ah, ok, so the dance goes something like: backport this to 7x, adjust the version checks to be before/after 7.15, remove the BWC branch from master entirely. LGTM then, thanks @iverase!
|
Yes, that's the dance |
|
|
||
| public void testSingleValuedString() throws Exception { | ||
| final Settings.Builder settings = Settings.builder() | ||
| .put(IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING.getKey(), 2) |
There was a problem hiding this comment.
I think its worth leaving a comment about why you chose 2 here and 12000 - 17000 and 5 below. I know they were fairly carefully chosen to get us to trigger the cuckoo filter. Its probably worth writing that out so future me won't blindly screw this up.
Oh!!!!!!!!!! Can we add to the profile results whether or not we used the cuckoo filter? Like a count of the number of times we used it? That way we can assert in this test that we hit it. Without that assertion this test could bit rot fairly easily.
There was a problem hiding this comment.
Add a comment about the choice of the number of docs. Not sure about adding such implementations details into the profiler. I guess it will be nice if the agg reported the precision of the result if possible, then you will know if the egg never went into cuckoo filter if the result is fully precise.
There was a problem hiding this comment.
Not sure about adding such implementations details into the profiler. I guess it will be nice if the agg reported the precision of the result if possible, then you will know if the egg never went into cuckoo filter if the result is fully precise.
That's kind of the point of the profiler - if you've ever wondered "did it do x or y" - you can just make it tell you. But, yeah, maybe it'd be nice to add a precise_results boolean or something then there isn't really a need for the profiler here at all. Either of those can wait for a follow up - but i think something to make sure the test can assert that it hit the non-precise case is important soon-ish.
qa/mixed-cluster/src/test/java/org/elasticsearch/backwards/RareTermsIT.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/common/util/CuckooFilter.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/common/util/CuckooFilter.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/common/util/CuckooFilter.java
Outdated
Show resolved
Hide resolved
|
open #74817 @elasticmachine run elasticsearch-ci/part-2 |
|
@elasticmachine run elasticsearch-ci/part-2 |
|
@elasticmachine test this please |
|
@elasticmachine test this please |
|
@elasticmachine run elasticsearch-ci/part-2 |
|
@elasticmachine update branch |
After backporting #74736, remove the BwC serialisation for 8.0.
5 participants
Currently, Cuckoo filters use Lucene structures to hold in memory arrays of packed ints. This is problematic when we move to Lucene 9.0 as those classes have been heavily modified, In addition we use Lucene Input/output API to serialise / deserialise the data structure which will break as well as Lucene is changing the API endianness.
Therefore, this PR forks the Lucene data structure into a class called PackedArray. The main change is that this class will be using Elasticsearch streams to serialise / deserialise, therefore we remove the dependency in Lucene's endianness. WE are still using lucene PackedInts to handle backwards compatibility.
We added a ESRestTestCase that can trigger the creation of Cuckoo filters and run in a mixed cluster in order to check backwards compatibility.
relates to #74057