Skip to content

Commit 0b2c8c8

Browse files
jimczijimczi
authored
Fix composite aggregation when after term is missing in the shard (#27936)
This change fixes a bug when a keyword term in the `after` key is not present in the shard. In this case the global ord of the document values are compared with the insertion point of the `after` keyword and values that are equal to the insertion point should be considered "after" the top value.
1 parent f6b9d3f commit 0b2c8c8

File tree

3 files changed

+111
-9
lines changed

3 files changed

+111
-9
lines changed

‎core/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSource.java‎

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,8 @@ static CompositeValuesSource<ValuesSource.Bytes.WithOrdinals, BytesRef> wrapGlob
129129
private static class GlobalOrdinalValuesSource extends CompositeValuesSource<ValuesSource.Bytes.WithOrdinals, BytesRef> {
130130
private final long[] values;
131131
private SortedSetDocValues lookup;
132-
private Long topValueLong;
132+
private Long topValueGlobalOrd;
133+
private boolean isTopValueInsertionPoint;
133134

134135
GlobalOrdinalValuesSource(ValuesSource.Bytes.WithOrdinals vs, int size, int reverseMul) {
135136
super(vs, size, reverseMul);
@@ -153,7 +154,14 @@ int compare(int from, int to) {
153154

154155
@Override
155156
int compareTop(int slot) {
156-
return Long.compare(values[slot], topValueLong) * reverseMul;
157+
int cmp = Long.compare(values[slot], topValueGlobalOrd);
158+
if (cmp == 0 && isTopValueInsertionPoint) {
159+
// the top value is missing in this shard, the comparison is against
160+
// the insertion point of the top value so equality means that the value
161+
// is "after" the insertion point.
162+
return reverseMul;
163+
}
164+
return cmp * reverseMul;
157165
}
158166

159167
@Override
@@ -177,11 +185,12 @@ Collector getLeafCollector(LeafReaderContext context, Collector next) throws IOE
177185
final SortedSetDocValues dvs = vs.globalOrdinalsValues(context);
178186
if (lookup == null) {
179187
lookup = dvs;
180-
if (topValue != null && topValueLong == null) {
181-
topValueLong = lookup.lookupTerm(topValue);
182-
if (topValueLong < 0) {
188+
if (topValue != null && topValueGlobalOrd == null) {
189+
topValueGlobalOrd = lookup.lookupTerm(topValue);
190+
if (topValueGlobalOrd < 0) {
183191
// convert negative insert position
184-
topValueLong = -topValueLong - 2;
192+
topValueGlobalOrd = -topValueGlobalOrd - 1;
193+
isTopValueInsertionPoint = true;
185194
}
186195
}
187196
}
@@ -202,7 +211,6 @@ Collector getLeafCollector(LeafReaderContext context, Collector next) throws IOE
202211
*/
203212
private static class BinaryValuesSource extends CompositeValuesSource<ValuesSource.Bytes, BytesRef> {
204213
private final BytesRef[] values;
205-
private BytesRef topValue;
206214

207215
BinaryValuesSource(ValuesSource.Bytes vs, int size, int reverseMul) {
208216
super(vs, size, reverseMul);
@@ -265,7 +273,6 @@ Collector getLeafCollector(LeafReaderContext context, Collector next) throws IOE
265273
*/
266274
private static class LongValuesSource extends CompositeValuesSource<ValuesSource.Numeric, Long> {
267275
private final long[] values;
268-
private long topValue;
269276

270277
LongValuesSource(ValuesSource.Numeric vs, int size, int reverseMul) {
271278
super(vs, size, reverseMul);
@@ -326,7 +333,6 @@ Collector getLeafCollector(LeafReaderContext context, Collector next) throws IOE
326333
*/
327334
private static class DoubleValuesSource extends CompositeValuesSource<ValuesSource.Numeric, Double> {
328335
private final double[] values;
329-
private double topValue;
330336

331337
DoubleValuesSource(ValuesSource.Numeric vs, int size, int reverseMul) {
332338
super(vs, size, reverseMul);

‎core/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,72 @@ public void testWithKeyword() throws Exception {
148148
);
149149
}
150150

151+
public void testWithKeywordMissingAfter() throws Exception {
152+
final List<Map<String, List<Object>>> dataset = new ArrayList<>();
153+
dataset.addAll(
154+
Arrays.asList(
155+
createDocument("keyword", "foo"),
156+
createDocument("keyword", "bar"),
157+
createDocument("keyword", "foo"),
158+
createDocument("keyword", "zoo"),
159+
createDocument("keyword", "bar"),
160+
createDocument("keyword", "delta")
161+
)
162+
);
163+
final Sort sort = new Sort(new SortedSetSortField("keyword", false));
164+
testSearchCase(new MatchAllDocsQuery(), sort, dataset,
165+
() -> {
166+
TermsValuesSourceBuilder terms = new TermsValuesSourceBuilder("keyword")
167+
.field("keyword");
168+
return new CompositeAggregationBuilder("name", Collections.singletonList(terms));
169+
}, (result) -> {
170+
assertEquals(4, result.getBuckets().size());
171+
assertEquals("{keyword=bar}", result.getBuckets().get(0).getKeyAsString());
172+
assertEquals(2L, result.getBuckets().get(0).getDocCount());
173+
assertEquals("{keyword=delta}", result.getBuckets().get(1).getKeyAsString());
174+
assertEquals(1L, result.getBuckets().get(1).getDocCount());
175+
assertEquals("{keyword=foo}", result.getBuckets().get(2).getKeyAsString());
176+
assertEquals(2L, result.getBuckets().get(2).getDocCount());
177+
assertEquals("{keyword=zoo}", result.getBuckets().get(3).getKeyAsString());
178+
assertEquals(1L, result.getBuckets().get(3).getDocCount());
179+
}
180+
);
181+
182+
testSearchCase(new MatchAllDocsQuery(), sort, dataset,
183+
() -> {
184+
TermsValuesSourceBuilder terms = new TermsValuesSourceBuilder("keyword")
185+
.field("keyword");
186+
return new CompositeAggregationBuilder("name", Collections.singletonList(terms))
187+
.aggregateAfter(Collections.singletonMap("keyword", "car"));
188+
}, (result) -> {
189+
assertEquals(3, result.getBuckets().size());
190+
assertEquals("{keyword=delta}", result.getBuckets().get(0).getKeyAsString());
191+
assertEquals(1L, result.getBuckets().get(0).getDocCount());
192+
assertEquals("{keyword=foo}", result.getBuckets().get(1).getKeyAsString());
193+
assertEquals(2L, result.getBuckets().get(1).getDocCount());
194+
assertEquals("{keyword=zoo}", result.getBuckets().get(2).getKeyAsString());
195+
assertEquals(1L, result.getBuckets().get(2).getDocCount());
196+
}
197+
);
198+
199+
testSearchCase(new MatchAllDocsQuery(), null, dataset,
200+
() -> {
201+
TermsValuesSourceBuilder terms = new TermsValuesSourceBuilder("keyword")
202+
.field("keyword").order(SortOrder.DESC);
203+
return new CompositeAggregationBuilder("name", Collections.singletonList(terms))
204+
.aggregateAfter(Collections.singletonMap("keyword", "mar"));
205+
}, (result) -> {
206+
assertEquals(3, result.getBuckets().size());
207+
assertEquals("{keyword=foo}", result.getBuckets().get(0).getKeyAsString());
208+
assertEquals(2L, result.getBuckets().get(0).getDocCount());
209+
assertEquals("{keyword=delta}", result.getBuckets().get(1).getKeyAsString());
210+
assertEquals(1L, result.getBuckets().get(1).getDocCount());
211+
assertEquals("{keyword=bar}", result.getBuckets().get(2).getKeyAsString());
212+
assertEquals(2L, result.getBuckets().get(2).getDocCount());
213+
}
214+
);
215+
}
216+
151217
public void testWithKeywordDesc() throws Exception {
152218
final List<Map<String, List<Object>>> dataset = new ArrayList<>();
153219
dataset.addAll(

‎rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/230_composite.yml‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,36 @@ setup:
163163
- match: { aggregations.test.buckets.1.key.kw: "bar" }
164164
- match: { aggregations.test.buckets.1.doc_count: 1 }
165165

166+
---
167+
"Aggregate After Missing":
168+
- skip:
169+
version: " - 6.99.99"
170+
reason: bug fixed in 7.0.0
171+
172+
173+
- do:
174+
search:
175+
index: test
176+
body:
177+
aggregations:
178+
test:
179+
composite:
180+
sources: [
181+
{
182+
"kw": {
183+
"terms": {
184+
"field": "keyword"
185+
}
186+
}
187+
}
188+
]
189+
after: { "kw": "delta" }
190+
191+
- match: {hits.total: 4}
192+
- length: { aggregations.test.buckets: 1 }
193+
- match: { aggregations.test.buckets.0.key.kw: "foo" }
194+
- match: { aggregations.test.buckets.0.doc_count: 2 }
195+
166196
---
167197
"Invalid Composite aggregation":
168198
- skip:

0 commit comments

Comments
 (0)