Skip to content

Commit 47719f8

Browse files
authored
Properly size empty filters (#71864)
This saves a few ArrayList realocations by fixing an order of operations bug issue in the "empty" handling arm for `filters`. I've also added a test so I can debug that branch.
1 parent 326a543 commit 47719f8

File tree

2 files changed

+31
-1
lines changed

2 files changed

+31
-1
lines changed

server/src/main/java/org/elasticsearch/search/aggregations/bucket/filter/FiltersAggregator.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -251,7 +251,7 @@ public InternalAggregation[] buildAggregations(long[] owningBucketOrds) throws I
251251
@Override
252252
public InternalAggregation buildEmptyAggregation() {
253253
InternalAggregations subAggs = buildEmptySubAggregations();
254-
List<InternalFilters.InternalBucket> buckets = new ArrayList<>(filters.size() + otherBucketKey == null ? 0 : 1);
254+
List<InternalFilters.InternalBucket> buckets = new ArrayList<>(filters.size() + (otherBucketKey == null ? 0 : 1));
255255
for (QueryToFilterAdapter<?> filter : filters) {
256256
InternalFilters.InternalBucket bucket = new InternalFilters.InternalBucket(filter.key().toString(), 0, subAggs, keyed);
257257
buckets.add(bucket);

server/src/test/java/org/elasticsearch/search/aggregations/bucket/filter/FiltersAggregatorTests.java

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@
8181
import static org.hamcrest.Matchers.hasSize;
8282
import static org.hamcrest.Matchers.instanceOf;
8383
import static org.hamcrest.Matchers.lessThan;
84+
import static org.hamcrest.Matchers.nullValue;
8485
import static org.mockito.Mockito.mock;
8586

8687
public class FiltersAggregatorTests extends AggregatorTestCase {
@@ -115,6 +116,35 @@ public void testEmpty() throws Exception {
115116
directory.close();
116117
}
117118

119+
public void testBuildEmpty() throws IOException {
120+
int numFilters = randomIntBetween(1, 10);
121+
QueryBuilder[] filters = new QueryBuilder[numFilters];
122+
for (int i = 0; i < filters.length; i++) {
123+
filters[i] = QueryBuilders.termQuery("field", randomAlphaOfLength(5));
124+
}
125+
FiltersAggregationBuilder builder = new FiltersAggregationBuilder("test", filters);
126+
boolean askForOtherBucket = true;
127+
if (askForOtherBucket) {
128+
builder.otherBucket(true).otherBucketKey("other");
129+
}
130+
withAggregator(
131+
builder,
132+
new MatchAllDocsQuery(),
133+
iw -> {},
134+
(searcher, aggregator) -> {
135+
InternalFilters result = (InternalFilters) aggregator.buildEmptyAggregation();
136+
for (int i = 0; i < filters.length; i++) {
137+
assertThat(result.getBucketByKey(String.valueOf(i)).getDocCount(), equalTo(0L));
138+
}
139+
if (askForOtherBucket) {
140+
assertThat(result.getBucketByKey("other").getDocCount(), equalTo(0L));
141+
} else {
142+
assertThat(result.getBucketByKey("other"), nullValue());
143+
}
144+
}
145+
);
146+
}
147+
118148
public void testNoFilters() throws IOException {
119149
testCase(new FiltersAggregationBuilder("test", new KeyedFilter[0]), new MatchAllDocsQuery(), iw -> {
120150
iw.addDocument(List.of());

0 commit comments

Comments
 (0)