Skip to content

Commit 86f955c

Browse files
austintleeaustintlee
authored
Implement buildEmptyAggregations for MultiTermsAggregator (opensearch-project#7089) (opensearch-project#7318)
* Implement buildEmptyAggregations for MultiTermsAggregator (opensearch-project#7089) Signed-off-by: Austin Lee <austin.t.lee@gmail.com> * Address Spotless check issue Signed-off-by: Austin Lee <austin.t.lee@gmail.com> * Add a unit test for MultiTermsAggregator.buildEmptyAggregations (opensearch-project#7089) Signed-off-by: Austin Lee <austin.t.lee@gmail.com> * Update changelog Update version check and reason Signed-off-by: Austin Lee <austin.t.lee@gmail.com> --------- Signed-off-by: Austin Lee <austin.t.lee@gmail.com>
1 parent 976048d commit 86f955c

File tree

4 files changed

+132
-1
lines changed

4 files changed

+132
-1
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
151151
- Enforce 512 byte document ID limit in bulk updates ([#8039](https://github.com/opensearch-project/OpenSearch/pull/8039))
152152
- With only GlobalAggregation in request causes unnecessary wrapping with MultiCollector ([#8125](https://github.com/opensearch-project/OpenSearch/pull/8125))
153153
- Fix mapping char_filter when mapping a hashtag ([#7591](https://github.com/opensearch-project/OpenSearch/pull/7591))
154+
- Fix NPE in multiterms aggregations involving empty buckets ([#7318](https://github.com/opensearch-project/OpenSearch/pull/7318))
154155

155156
### Security
156157

‎rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/370_multi_terms.yml

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -712,3 +712,51 @@ setup:
712712
- match: { aggregations.m_terms.buckets.0.key: ["a", 1] }
713713
- match: { aggregations.m_terms.buckets.0.key_as_string: "a|1" }
714714
- match: { aggregations.m_terms.buckets.0.doc_count: 4 }
715+
716+
---
717+
"aggregate over multi-terms test":
718+
- skip:
719+
version: "- 2.9.99"
720+
reason: "multi_terms aggregation was introduced in 2.1.0, NPE bug checked by this test case will manifest in any version < 3.0"
721+
722+
- do:
723+
bulk:
724+
index: test_1
725+
refresh: true
726+
body:
727+
- '{"index": {}}'
728+
- '{"str": "a", "ip": "127.0.0.1", "date": "2022-03-23"}'
729+
- '{"index": {}}'
730+
- '{"str": "a", "ip": "127.0.0.1", "date": "2022-03-25"}'
731+
- '{"index": {}}'
732+
- '{"str": "b", "ip": "127.0.0.1", "date": "2022-03-23"}'
733+
- '{"index": {}}'
734+
- '{"str": "b", "ip": "127.0.0.1", "date": "2022-03-25"}'
735+
736+
- do:
737+
search:
738+
index: test_1
739+
size: 0
740+
body:
741+
aggs:
742+
histo:
743+
date_histogram:
744+
field: date
745+
calendar_interval: day
746+
aggs:
747+
m_terms:
748+
multi_terms:
749+
terms:
750+
- field: str
751+
- field: ip
752+
753+
- match: { hits.total.value: 4 }
754+
- length: { aggregations.histo.buckets: 3 }
755+
- match: { aggregations.histo.buckets.0.key_as_string: "2022-03-23T00:00:00.000Z" }
756+
- match: { aggregations.histo.buckets.0.m_terms.buckets.0.key: ["a", "127.0.0.1"] }
757+
- match: { aggregations.histo.buckets.0.m_terms.buckets.1.key: ["b", "127.0.0.1"] }
758+
- match: { aggregations.histo.buckets.1.key_as_string: "2022-03-24T00:00:00.000Z" }
759+
- length: { aggregations.histo.buckets.1.m_terms.buckets: 0 }
760+
- match: { aggregations.histo.buckets.2.key_as_string: "2022-03-25T00:00:00.000Z" }
761+
- match: { aggregations.histo.buckets.2.m_terms.buckets.0.key: [ "a", "127.0.0.1" ] }
762+
- match: { aggregations.histo.buckets.2.m_terms.buckets.1.key: [ "b", "127.0.0.1" ] }

‎server/src/main/java/org/opensearch/search/aggregations/bucket/terms/MultiTermsAggregator.java

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -196,7 +196,20 @@ InternalMultiTerms buildResult(long owningBucketOrd, long otherDocCount, Interna
196196

197197
@Override
198198
public InternalAggregation buildEmptyAggregation() {
199-
return null;
199+
return new InternalMultiTerms(
200+
name,
201+
order,
202+
order,
203+
bucketCountThresholds.getRequiredSize(),
204+
bucketCountThresholds.getMinDocCount(),
205+
metadata(),
206+
bucketCountThresholds.getShardSize(),
207+
showTermDocCountError,
208+
0,
209+
0,
210+
formats,
211+
Collections.emptyList()
212+
);
200213
}
201214

202215
@Override

‎server/src/test/java/org/opensearch/search/aggregations/bucket/terms/MultiTermsAggregatorTests.java

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,36 +28,51 @@
2828
import org.opensearch.common.network.InetAddresses;
2929
import org.opensearch.common.settings.Settings;
3030
import org.opensearch.common.time.DateFormatter;
31+
import org.opensearch.common.util.BigArrays;
32+
import org.opensearch.common.util.MockPageCacheRecycler;
33+
import org.opensearch.index.IndexService;
34+
import org.opensearch.index.cache.IndexCache;
3135
import org.opensearch.index.mapper.BooleanFieldMapper;
3236
import org.opensearch.index.mapper.DateFieldMapper;
3337
import org.opensearch.index.mapper.GeoPointFieldMapper;
3438
import org.opensearch.index.mapper.IpFieldMapper;
3539
import org.opensearch.index.mapper.KeywordFieldMapper;
3640
import org.opensearch.index.mapper.MappedFieldType;
3741
import org.opensearch.index.mapper.NumberFieldMapper;
42+
import org.opensearch.index.query.QueryShardContext;
43+
import org.opensearch.index.shard.IndexShard;
44+
import org.opensearch.indices.breaker.NoneCircuitBreakerService;
3845
import org.opensearch.script.MockScriptEngine;
3946
import org.opensearch.script.Script;
4047
import org.opensearch.script.ScriptEngine;
4148
import org.opensearch.script.ScriptModule;
4249
import org.opensearch.script.ScriptService;
4350
import org.opensearch.script.ScriptType;
51+
import org.opensearch.search.DocValueFormat;
4452
import org.opensearch.search.aggregations.AggregationBuilder;
53+
import org.opensearch.search.aggregations.Aggregator;
54+
import org.opensearch.search.aggregations.AggregatorFactories;
4555
import org.opensearch.search.aggregations.AggregatorTestCase;
4656
import org.opensearch.search.aggregations.BucketOrder;
57+
import org.opensearch.search.aggregations.CardinalityUpperBound;
58+
import org.opensearch.search.aggregations.InternalAggregation;
4759
import org.opensearch.search.aggregations.metrics.InternalMax;
4860
import org.opensearch.search.aggregations.metrics.MaxAggregationBuilder;
4961
import org.opensearch.search.aggregations.support.CoreValuesSourceType;
5062
import org.opensearch.search.aggregations.support.MultiTermsValuesSourceConfig;
5163
import org.opensearch.search.aggregations.support.ValueType;
5264
import org.opensearch.search.aggregations.support.ValuesSourceType;
65+
import org.opensearch.search.internal.SearchContext;
5366
import org.opensearch.search.lookup.LeafDocLookup;
67+
import org.opensearch.test.TestSearchContext;
5468

5569
import java.io.IOException;
5670
import java.util.ArrayList;
5771
import java.util.Collections;
5872
import java.util.HashMap;
5973
import java.util.List;
6074
import java.util.Map;
75+
import java.util.UUID;
6176
import java.util.function.Consumer;
6277
import java.util.function.Function;
6378

@@ -68,8 +83,12 @@
6883
import static java.util.stream.Collectors.toList;
6984
import static org.hamcrest.Matchers.closeTo;
7085
import static org.hamcrest.Matchers.contains;
86+
import static org.hamcrest.Matchers.empty;
7187
import static org.hamcrest.Matchers.equalTo;
7288
import static org.hamcrest.Matchers.hasSize;
89+
import static org.hamcrest.Matchers.instanceOf;
90+
import static org.mockito.Mockito.mock;
91+
import static org.mockito.Mockito.when;
7392

7493
public class MultiTermsAggregatorTests extends AggregatorTestCase {
7594
private static final String FIELD_NAME = "field";
@@ -852,6 +871,56 @@ public void testIncludeExclude() throws IOException {
852871
);
853872
}
854873

874+
public void testEmptyAggregations() throws IOException {
875+
QueryShardContext queryShardContext = mock(QueryShardContext.class);
876+
IndexShard indexShard = mock(IndexShard.class);
877+
BigArrays bigArrays = new BigArrays(new MockPageCacheRecycler(Settings.EMPTY), new NoneCircuitBreakerService(), "");
878+
IndexService indexService = mock(IndexService.class);
879+
when(indexService.getShardOrNull(0)).thenReturn(indexShard);
880+
IndexCache cache = mock(IndexCache.class);
881+
when(cache.bitsetFilterCache()).thenReturn(null);
882+
when(indexService.cache()).thenReturn(cache);
883+
SearchContext context = new TestSearchContext(bigArrays, indexService);
884+
when(indexService.newQueryShardContext(0, null, () -> 0L, null)).thenReturn(queryShardContext);
885+
AggregatorFactories factories = AggregatorFactories.EMPTY;
886+
boolean showTermDocCountError = true;
887+
MultiTermsAggregator.InternalValuesSource internalValuesSources = mock(MultiTermsAggregator.InternalValuesSource.class);
888+
DocValueFormat format = mock(DocValueFormat.class);
889+
BucketOrder order = mock(BucketOrder.class);
890+
Aggregator.SubAggCollectionMode collectMode = Aggregator.SubAggCollectionMode.BREADTH_FIRST;
891+
TermsAggregator.BucketCountThresholds bucketCountThresholds = mock(TermsAggregator.BucketCountThresholds.class);
892+
Aggregator parent = mock(Aggregator.class);
893+
CardinalityUpperBound cardinality = CardinalityUpperBound.ONE;
894+
Map<String, Object> metadata = new HashMap<>();
895+
String k1 = UUID.randomUUID().toString();
896+
String v1 = UUID.randomUUID().toString();
897+
metadata.put(k1, v1);
898+
899+
MultiTermsAggregator mAgg = new MultiTermsAggregator(
900+
AGG_NAME,
901+
factories,
902+
showTermDocCountError,
903+
List.of(internalValuesSources),
904+
List.of(format),
905+
order,
906+
collectMode,
907+
bucketCountThresholds,
908+
context,
909+
parent,
910+
cardinality,
911+
metadata
912+
);
913+
InternalAggregation emptyAgg = mAgg.buildEmptyAggregation();
914+
915+
MatcherAssert.assertThat(emptyAgg.getName(), equalTo(AGG_NAME));
916+
MatcherAssert.assertThat(emptyAgg, instanceOf(InternalMultiTerms.class));
917+
918+
InternalMultiTerms mt = (InternalMultiTerms) emptyAgg;
919+
MatcherAssert.assertThat(mt.getMetadata().keySet(), contains(k1));
920+
MatcherAssert.assertThat(mt.getMetadata().get(k1), equalTo(v1));
921+
MatcherAssert.assertThat(mt.getBuckets(), empty());
922+
}
923+
855924
private void testAggregation(
856925
Query query,
857926
List<MultiTermsValuesSourceConfig> terms,

0 commit comments

Comments
 (0)