Skip to content

Commit 6bef1e1

Browse files
kkewweikkewwei
authored
Fix infinite loop in nested agg (#15931)
Signed-off-by: kkewwei <[email protected]>
1 parent 80ff07e commit 6bef1e1

File tree

5 files changed

+174
-12
lines changed

5 files changed

+174
-12
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
3030
- Fix wildcard query containing escaped character ([#15737](https://github.com/opensearch-project/OpenSearch/pull/15737))
3131
- Fix case-insensitive query on wildcard field ([#15882](https://github.com/opensearch-project/OpenSearch/pull/15882))
3232
- Add validation for the search backpressure cancellation settings ([#15501](https://github.com/opensearch-project/OpenSearch/pull/15501))
33+
- Fix infinite loop in nested agg ([#15931](https://github.com/opensearch-project/OpenSearch/pull/15931))
3334

3435
### Security
3536

‎rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/410_nested_aggs.yml

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
---
2+
# The test setup includes:
3+
# - Create nested mapping for test_nested_agg_index index
4+
# - Index two example documents
5+
# - nested agg
6+
7+
setup:
8+
- do:
9+
indices.create:
10+
index: test_nested_agg_index
11+
body:
12+
mappings:
13+
properties:
14+
a:
15+
type: nested
16+
properties:
17+
b1:
18+
type: keyword
19+
b2:
20+
type: nested
21+
properties:
22+
c:
23+
type: nested
24+
properties:
25+
d:
26+
type: keyword
27+
28+
- do:
29+
bulk:
30+
refresh: true
31+
body: |
32+
{"index": {"_index": "test_nested_agg_index", "_id": "0"}}
33+
{"a": { "b1": "b11", "b2": { "c": { "d": "d1" } }}}
34+
{"index": {"_index": "test_nested_agg_index", "_id": "1"}}
35+
{"a": { "b1": "b12", "b2": { "c": { "d": "d2" } }}}
36+
37+
---
38+
# Delete Index when connection is teardown
39+
teardown:
40+
- do:
41+
indices.delete:
42+
index: test_nested_agg_index
43+
44+
---
45+
"Supported queries":
46+
- skip:
47+
version: " - 2.17.99"
48+
reason: "fixed in 2.18.0"
49+
50+
# Verify Document Count
51+
- do:
52+
search:
53+
body: {
54+
query: {
55+
match_all: { }
56+
}
57+
}
58+
59+
- length: { hits.hits: 2 }
60+
61+
# Verify nested aggregation
62+
- do:
63+
search:
64+
body: {
65+
aggs: {
66+
nested_agg: {
67+
nested: {
68+
path: "a"
69+
},
70+
aggs: {
71+
a_b1: {
72+
terms: {
73+
field: "a.b1"
74+
},
75+
aggs: {
76+
"c": {
77+
nested: {
78+
path: "a.b2.c"
79+
},
80+
aggs: {
81+
"d": {
82+
terms: {
83+
field: "a.b2.c.d"
84+
}
85+
}
86+
}
87+
}
88+
}
89+
}
90+
}
91+
}
92+
}
93+
}
94+
95+
- length: { hits.hits: 2 }
96+
- match: { aggregations.nested_agg.doc_count: 2 }
97+
- length: { aggregations.nested_agg.a_b1.buckets: 2 }
98+
99+
- match: { aggregations.nested_agg.a_b1.buckets.0.key: "b11" }
100+
- match: { aggregations.nested_agg.a_b1.buckets.0.doc_count: 1 }
101+
- match: { aggregations.nested_agg.a_b1.buckets.0.c.doc_count: 1 }
102+
- length: { aggregations.nested_agg.a_b1.buckets.0.c.d.buckets: "1" }
103+
- match: { aggregations.nested_agg.a_b1.buckets.0.c.d.buckets.0.key: "d1" }
104+
- match: { aggregations.nested_agg.a_b1.buckets.0.c.d.buckets.0.doc_count: 1 }
105+
106+
- match: { aggregations.nested_agg.a_b1.buckets.1.key: "b12" }
107+
- match: { aggregations.nested_agg.a_b1.buckets.1.doc_count: 1 }
108+
- match: { aggregations.nested_agg.a_b1.buckets.1.c.doc_count: 1 }
109+
- length: { aggregations.nested_agg.a_b1.buckets.1.c.d.buckets: "1" }
110+
- match: { aggregations.nested_agg.a_b1.buckets.1.c.d.buckets.0.key: "d2" }
111+
- match: { aggregations.nested_agg.a_b1.buckets.1.c.d.buckets.0.doc_count: 1 }

‎server/src/main/java/org/opensearch/index/mapper/ObjectMapper.java

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,19 @@ public void setIncludeInParent(boolean value) {
171171
public void setIncludeInRoot(boolean value) {
172172
includeInRoot = new Explicit<>(value, true);
173173
}
174+
175+
public static boolean isParent(ObjectMapper parentObjectMapper, ObjectMapper childObjectMapper, MapperService mapperService) {
176+
if (parentObjectMapper == null || childObjectMapper == null) {
177+
return false;
178+
}
179+
180+
ObjectMapper parent = childObjectMapper.getParentObjectMapper(mapperService);
181+
while (parent != null && parent != parentObjectMapper) {
182+
childObjectMapper = parent;
183+
parent = childObjectMapper.getParentObjectMapper(mapperService);
184+
}
185+
return parentObjectMapper == parent;
186+
}
174187
}
175188

176189
/**

‎server/src/main/java/org/opensearch/search/aggregations/bucket/nested/NestedAggregator.java

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,6 @@
4646
import org.opensearch.common.collect.Tuple;
4747
import org.opensearch.common.lucene.search.Queries;
4848
import org.opensearch.core.ParseField;
49-
import org.opensearch.index.mapper.MapperService;
5049
import org.opensearch.index.mapper.ObjectMapper;
5150
import org.opensearch.search.aggregations.Aggregator;
5251
import org.opensearch.search.aggregations.AggregatorFactories;
@@ -63,6 +62,8 @@
6362
import java.util.List;
6463
import java.util.Map;
6564

65+
import static org.opensearch.index.mapper.ObjectMapper.Nested.isParent;
66+
6667
/**
6768
* Aggregate all docs that match a nested path
6869
*
@@ -98,17 +99,6 @@ public class NestedAggregator extends BucketsAggregator implements SingleBucketA
9899
this.collectsFromSingleBucket = cardinality.map(estimate -> estimate < 2);
99100
}
100101

101-
private boolean isParent(ObjectMapper parentObjectMapper, ObjectMapper childObjectMapper, MapperService mapperService) {
102-
if (parentObjectMapper == null) {
103-
return false;
104-
}
105-
ObjectMapper parent;
106-
do {
107-
parent = childObjectMapper.getParentObjectMapper(mapperService);
108-
} while (parent != null && parent != parentObjectMapper);
109-
return parentObjectMapper == parent;
110-
}
111-
112102
@Override
113103
public LeafBucketCollector getLeafCollector(final LeafReaderContext ctx, final LeafBucketCollector sub) throws IOException {
114104
IndexReaderContext topLevelContext = ReaderUtil.getTopLevelContext(ctx);

‎server/src/test/java/org/opensearch/index/mapper/ObjectMapperTests.java

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,12 @@
5151

5252
import java.io.IOException;
5353
import java.util.Collection;
54+
import java.util.Collections;
55+
56+
import org.mockito.Mockito;
5457

5558
import static org.opensearch.common.util.FeatureFlags.STAR_TREE_INDEX;
59+
import static org.opensearch.index.mapper.ObjectMapper.Nested.isParent;
5660
import static org.hamcrest.Matchers.containsString;
5761

5862
public class ObjectMapperTests extends OpenSearchSingleNodeTestCase {
@@ -568,6 +572,49 @@ public void testCompositeFields() throws Exception {
568572
FeatureFlags.initializeFeatureFlags(Settings.EMPTY);
569573
}
570574

575+
public void testNestedIsParent() throws Exception {
576+
String mapping = XContentFactory.jsonBuilder()
577+
.startObject()
578+
.startObject("properties")
579+
.startObject("a")
580+
.field("type", "nested")
581+
.startObject("properties")
582+
.field("b1", Collections.singletonMap("type", "keyword"))
583+
.startObject("b2")
584+
.field("type", "nested")
585+
.startObject("properties")
586+
.startObject("c")
587+
.field("type", "nested")
588+
.startObject("properties")
589+
.field("d", Collections.singletonMap("type", "keyword"))
590+
.endObject()
591+
.endObject()
592+
.endObject()
593+
.endObject()
594+
.endObject()
595+
.endObject()
596+
.endObject()
597+
.endObject()
598+
.toString();
599+
600+
DocumentMapper documentMapper = createIndex("test").mapperService()
601+
.documentMapperParser()
602+
.parse("_doc", new CompressedXContent(mapping));
603+
604+
MapperService mapperService = Mockito.mock(MapperService.class);
605+
Mockito.when(mapperService.getObjectMapper(("a"))).thenReturn(documentMapper.objectMappers().get("a"));
606+
Mockito.when(mapperService.getObjectMapper(("a.b2"))).thenReturn(documentMapper.objectMappers().get("a.b2"));
607+
Mockito.when(mapperService.getObjectMapper(("a.b2.c"))).thenReturn(documentMapper.objectMappers().get("a.b2.c"));
608+
609+
assertTrue(isParent(documentMapper.objectMappers().get("a"), documentMapper.objectMappers().get("a.b2.c"), mapperService));
610+
assertTrue(isParent(documentMapper.objectMappers().get("a"), documentMapper.objectMappers().get("a.b2"), mapperService));
611+
assertTrue(isParent(documentMapper.objectMappers().get("a.b2"), documentMapper.objectMappers().get("a.b2.c"), mapperService));
612+
613+
assertFalse(isParent(documentMapper.objectMappers().get("a.b2.c"), documentMapper.objectMappers().get("a"), mapperService));
614+
assertFalse(isParent(documentMapper.objectMappers().get("a.b2"), documentMapper.objectMappers().get("a"), mapperService));
615+
assertFalse(isParent(documentMapper.objectMappers().get("a.b2.c"), documentMapper.objectMappers().get("a.b2"), mapperService));
616+
}
617+
571618
@Override
572619
protected Collection<Class<? extends Plugin>> getPlugins() {
573620
return pluginList(InternalSettingsPlugin.class);

0 commit comments

Comments
 (0)