Skip to content

Commit 24b0cc9

Browse files
msfrohmsfroh
authored
Allow composite aggregation under filter or reverse_nested parent (opensearch-project#11499)
* Allow composite aggregation under filter parent Composite aggregations are able to run under a filter aggregation with no change required (other than not throwing an exception). Also cleaned up FilterAggregatorFactory a little. Signed-off-by: Michael Froh <[email protected]> * Add changelog entry Signed-off-by: Michael Froh <[email protected]> * Add support for reverse nested agg too Signed-off-by: Michael Froh <[email protected]> * Add unit test coverage Signed-off-by: Michael Froh <[email protected]> * Skip new tests in pre-3.0 mixed cluster Signed-off-by: Michael Froh <[email protected]> --------- Signed-off-by: Michael Froh <[email protected]>
1 parent f55b9e0 commit 24b0cc9

File tree

6 files changed

+196
-15
lines changed

6 files changed

+196
-15
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
208208
- Change error message when per shard document limit is breached ([#11312](https://github.com/opensearch-project/OpenSearch/pull/11312))
209209
- Improve boolean parsing performance ([#11308](https://github.com/opensearch-project/OpenSearch/pull/11308))
210210
- Interpret byte array as primitive using VarHandles ([#11362](https://github.com/opensearch-project/OpenSearch/pull/11362))
211+
- Allow composite aggregation to run under a parent filter aggregation ([#11499](https://github.com/opensearch-project/OpenSearch/pull/11499))
211212
- Automatically add scheme to discovery.ec2.endpoint ([#11512](https://github.com/opensearch-project/OpenSearch/pull/11512))
212213
- Restore support for Java 8 for RestClient ([#11562](https://github.com/opensearch-project/OpenSearch/pull/11562))
213214
- Add deleted doc count in _cat/shards ([#11678](https://github.com/opensearch-project/OpenSearch/pull/11678))

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

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -509,6 +509,134 @@ setup:
509509
- match: { aggregations.1.2.buckets.1.key.nested: 1000 }
510510
- match: { aggregations.1.2.buckets.1.doc_count: 1 }
511511

512+
---
513+
"Composite aggregation with filtered nested parent":
514+
- skip:
515+
version: " - 2.99.99"
516+
reason: fixed in 3.0.0
517+
- do:
518+
search:
519+
rest_total_hits_as_int: true
520+
index: test
521+
body:
522+
aggregations:
523+
1:
524+
nested:
525+
path: nested
526+
aggs:
527+
2:
528+
filter:
529+
range:
530+
nested.nested_long:
531+
gt: 0
532+
lt: 100
533+
aggs:
534+
3:
535+
composite:
536+
sources: [
537+
"nested": {
538+
"terms": {
539+
"field": "nested.nested_long"
540+
}
541+
}
542+
]
543+
544+
- match: {hits.total: 6}
545+
- length: { aggregations.1.2.3.buckets: 2 }
546+
- match: { aggregations.1.2.3.buckets.0.key.nested: 10 }
547+
- match: { aggregations.1.2.3.buckets.0.doc_count: 2 }
548+
- match: { aggregations.1.2.3.buckets.1.key.nested: 20 }
549+
- match: { aggregations.1.2.3.buckets.1.doc_count: 2 }
550+
- do:
551+
search:
552+
rest_total_hits_as_int: true
553+
index: test
554+
body:
555+
aggregations:
556+
1:
557+
nested:
558+
path: nested
559+
aggs:
560+
2:
561+
filter:
562+
range:
563+
nested.nested_long:
564+
gt: 0
565+
lt: 100
566+
aggs:
567+
3:
568+
composite:
569+
after: { "nested": 10 }
570+
sources: [
571+
"nested": {
572+
"terms": {
573+
"field": "nested.nested_long"
574+
}
575+
}
576+
]
577+
- match: {hits.total: 6}
578+
- length: { aggregations.1.2.3.buckets: 1 }
579+
- match: { aggregations.1.2.3.buckets.0.key.nested: 20 }
580+
- match: { aggregations.1.2.3.buckets.0.doc_count: 2 }
581+
582+
---
583+
"Composite aggregation with filtered reverse nested parent":
584+
- skip:
585+
version: " - 2.99.99"
586+
reason: fixed in 3.0.0
587+
- do:
588+
search:
589+
rest_total_hits_as_int: true
590+
index: test
591+
body:
592+
aggregations:
593+
1:
594+
nested:
595+
path: nested
596+
aggs:
597+
2:
598+
filter:
599+
range:
600+
nested.nested_long:
601+
gt: 0
602+
lt: 20
603+
aggs:
604+
3:
605+
reverse_nested: {}
606+
aggs:
607+
4:
608+
composite:
609+
sources: [
610+
{
611+
"long": {
612+
"terms": {
613+
"field": "long"
614+
}
615+
}
616+
},
617+
{
618+
"kw": {
619+
"terms": {
620+
"field": "keyword"
621+
}
622+
}
623+
}
624+
]
625+
- match: {hits.total: 6}
626+
- length: { aggregations.1.2.3.4.buckets: 4 }
627+
- match: { aggregations.1.2.3.4.buckets.0.key.long: 0 }
628+
- match: { aggregations.1.2.3.4.buckets.0.key.kw: "bar" }
629+
- match: { aggregations.1.2.3.4.buckets.0.doc_count: 1 }
630+
- match: { aggregations.1.2.3.4.buckets.1.key.long: 10 }
631+
- match: { aggregations.1.2.3.4.buckets.1.key.kw: "foo" }
632+
- match: { aggregations.1.2.3.4.buckets.1.doc_count: 1 }
633+
- match: { aggregations.1.2.3.4.buckets.2.key.long: 20 }
634+
- match: { aggregations.1.2.3.4.buckets.2.key.kw: "foo" }
635+
- match: { aggregations.1.2.3.4.buckets.2.doc_count: 1 }
636+
- match: { aggregations.1.2.3.4.buckets.3.key.long: 100 }
637+
- match: { aggregations.1.2.3.4.buckets.3.key.kw: "bar" }
638+
- match: { aggregations.1.2.3.4.buckets.3.doc_count: 1 }
639+
512640
---
513641
"Composite aggregation with unmapped field":
514642
- skip:

‎server/src/main/java/org/opensearch/search/aggregations/bucket/composite/CompositeAggregationBuilder.java‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,9 @@
4444
import org.opensearch.search.aggregations.AggregationBuilder;
4545
import org.opensearch.search.aggregations.AggregatorFactories;
4646
import org.opensearch.search.aggregations.AggregatorFactory;
47+
import org.opensearch.search.aggregations.bucket.filter.FilterAggregatorFactory;
4748
import org.opensearch.search.aggregations.bucket.nested.NestedAggregatorFactory;
49+
import org.opensearch.search.aggregations.bucket.nested.ReverseNestedAggregatorFactory;
4850
import org.opensearch.search.aggregations.support.ValuesSourceRegistry;
4951

5052
import java.io.IOException;
@@ -240,14 +242,16 @@ public BucketCardinality bucketCardinality() {
240242
* this aggregator or the instance of the parent's factory that is incompatible with
241243
* the composite aggregation.
242244
*/
243-
private AggregatorFactory checkParentIsNullOrNested(AggregatorFactory factory) {
245+
private static AggregatorFactory checkParentIsSafe(AggregatorFactory factory) {
244246
if (factory == null) {
245247
return null;
246-
} else if (factory instanceof NestedAggregatorFactory) {
247-
return checkParentIsNullOrNested(factory.getParent());
248-
} else {
249-
return factory;
250-
}
248+
} else if (factory instanceof NestedAggregatorFactory
249+
|| factory instanceof FilterAggregatorFactory
250+
|| factory instanceof ReverseNestedAggregatorFactory) {
251+
return checkParentIsSafe(factory.getParent());
252+
} else {
253+
return factory;
254+
}
251255
}
252256

253257
private static void validateSources(List<CompositeValuesSourceBuilder<?>> sources) {
@@ -278,7 +282,7 @@ protected AggregatorFactory doBuild(
278282
AggregatorFactory parent,
279283
AggregatorFactories.Builder subfactoriesBuilder
280284
) throws IOException {
281-
AggregatorFactory invalid = checkParentIsNullOrNested(parent);
285+
AggregatorFactory invalid = checkParentIsSafe(parent);
282286
if (invalid != null) {
283287
throw new IllegalArgumentException(
284288
"[composite] aggregation cannot be used with a parent aggregation of"

‎server/src/main/java/org/opensearch/search/aggregations/bucket/filter/FilterAggregatorFactory.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@
5656
public class FilterAggregatorFactory extends AggregatorFactory {
5757

5858
private Weight weight;
59-
private Query filter;
59+
private final Query filter;
6060

6161
public FilterAggregatorFactory(
6262
String name,
@@ -85,7 +85,7 @@ public Weight getWeight() {
8585
try {
8686
weight = contextSearcher.createWeight(contextSearcher.rewrite(filter), ScoreMode.COMPLETE_NO_SCORES, 1f);
8787
} catch (IOException e) {
88-
throw new AggregationInitializationException("Failed to initialse filter", e);
88+
throw new AggregationInitializationException("Failed to initialise filter", e);
8989
}
9090
}
9191
return weight;
@@ -98,7 +98,7 @@ public Aggregator createInternal(
9898
CardinalityUpperBound cardinality,
9999
Map<String, Object> metadata
100100
) throws IOException {
101-
return new FilterAggregator(name, () -> this.getWeight(), factories, searchContext, parent, cardinality, metadata);
101+
return new FilterAggregator(name, this::getWeight, factories, searchContext, parent, cardinality, metadata);
102102
}
103103

104104
@Override

‎server/src/test/java/org/opensearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,10 @@
3838
import org.apache.lucene.search.MatchAllDocsQuery;
3939
import org.apache.lucene.search.TermQuery;
4040
import org.opensearch.OpenSearchParseException;
41+
import org.opensearch.index.query.MatchAllQueryBuilder;
42+
import org.opensearch.search.aggregations.AggregationBuilders;
4143
import org.opensearch.search.aggregations.Aggregator;
44+
import org.opensearch.search.aggregations.bucket.filter.FilterAggregationBuilder;
4245
import org.opensearch.search.aggregations.bucket.histogram.DateHistogramInterval;
4346
import org.opensearch.search.aggregations.bucket.missing.MissingOrder;
4447
import org.opensearch.search.aggregations.bucket.terms.StringTerms;
@@ -2460,4 +2463,41 @@ public void testIndexSortWithDuplicate() throws Exception {
24602463
);
24612464
}
24622465
}
2466+
2467+
public void testUnderFilterAggregator() throws IOException {
2468+
executeTestCase(false, false, new MatchAllDocsQuery(), Collections.emptyList(), () -> {
2469+
FilterAggregationBuilder filterAggregatorBuilder = new FilterAggregationBuilder(
2470+
"filter_mcmilterface",
2471+
new MatchAllQueryBuilder()
2472+
);
2473+
filterAggregatorBuilder.subAggregation(
2474+
new CompositeAggregationBuilder(
2475+
"compo",
2476+
Collections.singletonList(new TermsValuesSourceBuilder("keyword").field("keyword"))
2477+
)
2478+
);
2479+
return filterAggregatorBuilder;
2480+
}, (ic) -> {});
2481+
}
2482+
2483+
public void testUnderBucketAggregator() throws IOException {
2484+
try {
2485+
executeTestCase(false, false, new MatchAllDocsQuery(), Collections.emptyList(), () -> {
2486+
TermsAggregationBuilder termsAggregationBuilder = AggregationBuilders.terms("terms").field("keyword");
2487+
termsAggregationBuilder.subAggregation(
2488+
new CompositeAggregationBuilder(
2489+
"compo",
2490+
Collections.singletonList(new TermsValuesSourceBuilder("keyword").field("keyword"))
2491+
)
2492+
);
2493+
return termsAggregationBuilder;
2494+
}, (ic) -> {});
2495+
fail("Should have thrown an IllegalArgumentException");
2496+
} catch (IllegalArgumentException iae) {
2497+
assertTrue(
2498+
iae.getMessage()
2499+
.contains("[composite] aggregation cannot be used with a parent aggregation of type: [TermsAggregatorFactory]")
2500+
);
2501+
}
2502+
}
24632503
}

‎test/framework/src/main/java/org/opensearch/search/aggregations/composite/BaseCompositeAggregatorTestCase.java‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,9 @@
4646
import org.opensearch.index.mapper.MappedFieldType;
4747
import org.opensearch.index.mapper.MapperService;
4848
import org.opensearch.index.mapper.NumberFieldMapper;
49+
import org.opensearch.search.aggregations.AggregationBuilder;
4950
import org.opensearch.search.aggregations.AggregatorTestCase;
51+
import org.opensearch.search.aggregations.InternalAggregation;
5052
import org.opensearch.search.aggregations.bucket.composite.CompositeAggregationBuilder;
5153
import org.opensearch.search.aggregations.bucket.composite.CompositeValuesSourceBuilder;
5254
import org.opensearch.search.aggregations.bucket.composite.InternalComposite;
@@ -139,12 +141,16 @@ protected void executeTestCase(
139141
boolean useIndexSort,
140142
Query query,
141143
List<Map<String, List<Object>>> dataset,
142-
Supplier<CompositeAggregationBuilder> create,
144+
Supplier<? extends AggregationBuilder> create,
143145
Consumer<InternalComposite> verify
144146
) throws IOException {
145147
Map<String, MappedFieldType> types = FIELD_TYPES.stream().collect(Collectors.toMap(MappedFieldType::name, Function.identity()));
146-
CompositeAggregationBuilder aggregationBuilder = create.get();
147-
Sort indexSort = useIndexSort ? buildIndexSort(aggregationBuilder.sources(), types) : null;
148+
AggregationBuilder aggregationBuilder = create.get();
149+
Sort indexSort = null;
150+
if (aggregationBuilder instanceof CompositeAggregationBuilder && useIndexSort) {
151+
CompositeAggregationBuilder cab = (CompositeAggregationBuilder) aggregationBuilder;
152+
indexSort = buildIndexSort(cab.sources(), types);
153+
}
148154
IndexSettings indexSettings = createIndexSettings(indexSort);
149155
try (Directory directory = newDirectory()) {
150156
IndexWriterConfig config = newIndexWriterConfig(random(), new MockAnalyzer(random()));
@@ -180,14 +186,16 @@ protected void executeTestCase(
180186
}
181187
try (IndexReader indexReader = DirectoryReader.open(directory)) {
182188
IndexSearcher indexSearcher = new IndexSearcher(indexReader);
183-
InternalComposite composite = searchAndReduce(
189+
InternalAggregation aggregation = searchAndReduce(
184190
indexSettings,
185191
indexSearcher,
186192
query,
187193
aggregationBuilder,
188194
FIELD_TYPES.toArray(new MappedFieldType[0])
189195
);
190-
verify.accept(composite);
196+
if (aggregation instanceof InternalComposite) {
197+
verify.accept((InternalComposite) aggregation);
198+
}
191199
}
192200
}
193201
}

0 commit comments

Comments
 (0)