Skip to content

Commit 77c77f2

Browse files
authored
Rare terms aggregation false **positive** fix (#126884)
I found that `rare_terms` aggregation can return **false positive** results (some of the returned results are not real). I traced it down and find out it's a bug in `CuckooFilter`'s merge methord. ``` if (isSetMode == false && other.isSetMode) { other.hashes.forEach(this::add); } ``` should be ``` if (isSetMode == false && other.isSetMode) { other.hashes.forEach(this::addHash); } ```
1 parent 0f6b0b9 commit 77c77f2

File tree

3 files changed

+34
-1
lines changed

3 files changed

+34
-1
lines changed

docs/changelog/126884.yaml

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 126884
2+
summary: Rare terms aggregation false **positive** fix
3+
area: Aggregations
4+
type: bug
5+
issues: []

server/src/main/java/org/elasticsearch/common/util/SetBackedScalingCuckooFilter.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -341,7 +341,7 @@ public void merge(SetBackedScalingCuckooFilter other) {
341341
} else if (isSetMode == false && other.isSetMode) {
342342
// Rather than converting the other to a cuckoo first, we can just
343343
// replay the values directly into our filter.
344-
other.hashes.forEach(this::add);
344+
other.hashes.forEach(this::addHash);
345345
} else {
346346
// Both are in cuckoo mode, merge raw fingerprints
347347

server/src/test/java/org/elasticsearch/common/util/SetBackedScalingCuckooFilterTests.java

+28
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,34 @@ public void testConvertTwice() {
132132
);
133133
}
134134

135+
public void testMergeBigSmall() {
136+
int threshold = 1000;
137+
138+
// Setup the first filter
139+
SetBackedScalingCuckooFilter filter = new SetBackedScalingCuckooFilter(threshold, Randomness.get(), 0.01);
140+
int counter = 0;
141+
Set<Long> values = new HashSet<>();
142+
while (counter < threshold + 1) {
143+
long value = randomLong();
144+
filter.add(value);
145+
boolean newValue = values.add(value);
146+
if (newValue) {
147+
counter += 1;
148+
}
149+
}
150+
151+
SetBackedScalingCuckooFilter filter2 = new SetBackedScalingCuckooFilter(threshold, Randomness.get(), 0.01);
152+
long value = randomLong();
153+
while (filter.mightContain(value)) {
154+
value = randomLong();
155+
}
156+
157+
filter2.add(value);
158+
159+
filter.merge(filter2);
160+
assertTrue(filter.mightContain(value));
161+
}
162+
135163
public void testMergeSmall() {
136164
int threshold = 1000;
137165

0 commit comments

Comments
 (0)