Skip to content

Commit 9b6d8e8

Browse files
not-napoleonnot-napoleon
authored
[7.17] Fix a bug with flattened fields in terms aggregations (elastic#87392) (elastic#87506)
* Fix a bug with flattened fields in terms aggregations (elastic#87392) The root cause here was that missing did not correctly delegate `supportsGlobalOrdinalsMappnig` to the wrapped values source, instead falling back to the default. I've added the delegation, and made the base method abstract so this doesn't happen again. * BWC version dance, part one * Fix flattened object format
1 parent 51b1f1d commit 9b6d8e8

File tree

6 files changed

+164
-4
lines changed

6 files changed

+164
-4
lines changed

‎docs/changelog/87392.yaml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 87392
2+
summary: Fix a bug with flattened fields in terms aggregations
3+
area: Aggregations
4+
type: bug
5+
issues: []

‎rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.aggregation/20_terms.yml‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@ setup:
1010
properties:
1111
str:
1212
type: keyword
13+
str_missing:
14+
type: keyword
1315
ip:
1416
type: ip
1517
boolean:
@@ -44,7 +46,6 @@ setup:
4446
- do:
4547
cluster.health:
4648
wait_for_status: green
47-
4849
---
4950
"Basic test":
5051
- do:

‎rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.aggregation/24_terms_flattened_field.yml‎

Lines changed: 139 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,139 @@
1+
setup:
2+
- do:
3+
indices.create:
4+
index: test_1
5+
body:
6+
settings:
7+
number_of_shards: 1
8+
number_of_replicas: 0
9+
mappings:
10+
properties:
11+
flattened:
12+
type: flattened
13+
14+
- do:
15+
cluster.health:
16+
wait_for_status: green
17+
18+
- do:
19+
bulk:
20+
refresh: true
21+
index: test_1
22+
body:
23+
- '{"index": {}}'
24+
- '{"flattened": {"primary": "foo", "secondary": "bar"}}'
25+
- '{"index": {}}'
26+
- '{"flattened": {"primary": "foo", "secondary": "zwomp"}}'
27+
- '{"index": {}}'
28+
- '{"flattened": {"primary": "baz", "secondary": "bar"}}'
29+
- '{"index": {}}'
30+
- '{"flattened": {"primary": "quux"}}'
31+
---
32+
"Key exists, no missing values":
33+
- skip:
34+
version: " - 7.17.4, 8.0.0 - 8.3.99"
35+
reason: "fixed in 8.4"
36+
37+
- do:
38+
search:
39+
rest_total_hits_as_int: true
40+
body:
41+
size: 0
42+
aggs:
43+
primary_terms:
44+
terms:
45+
field: "flattened.primary"
46+
47+
48+
49+
- match: { hits.total: 4 }
50+
- length: { aggregations.primary_terms.buckets: 3 }
51+
- match: { aggregations.primary_terms.buckets.0.key: "foo" }
52+
- match: { aggregations.primary_terms.buckets.0.doc_count: 2 }
53+
- match: { aggregations.primary_terms.buckets.1.key: "baz" }
54+
- match: { aggregations.primary_terms.buckets.1.doc_count: 1 }
55+
- match: { aggregations.primary_terms.buckets.2.key: "quux" }
56+
- match: { aggregations.primary_terms.buckets.2.doc_count: 1 }
57+
---
58+
"Key exists, missing values, missing specified":
59+
- skip:
60+
version: " - 7.17.4, 8.0.0 - 8.3.99"
61+
reason: "fixed in 8.4"
62+
- do:
63+
search:
64+
rest_total_hits_as_int: true
65+
body:
66+
size: 0
67+
aggs:
68+
primary_terms:
69+
terms:
70+
field: "flattened.secondary"
71+
missing: "missing_value"
72+
73+
74+
- match: { hits.total: 4 }
75+
- length: { aggregations.primary_terms.buckets: 3 }
76+
- match: { aggregations.primary_terms.buckets.0.key: "bar" }
77+
- match: { aggregations.primary_terms.buckets.0.doc_count: 2 }
78+
- match: { aggregations.primary_terms.buckets.1.key: "missing_value" }
79+
- match: { aggregations.primary_terms.buckets.1.doc_count: 1 }
80+
- match: { aggregations.primary_terms.buckets.2.key: "zwomp" }
81+
- match: { aggregations.primary_terms.buckets.2.doc_count: 1 }
82+
---
83+
"Key exists, missing values, missing not specified":
84+
- skip:
85+
version: " - 7.17.4, 8.0.0 - 8.3.99"
86+
reason: "fixed in 8.4"
87+
- do:
88+
search:
89+
rest_total_hits_as_int: true
90+
body:
91+
size: 0
92+
aggs:
93+
primary_terms:
94+
terms:
95+
field: "flattened.secondary"
96+
97+
- match: { hits.total: 4 }
98+
- length: { aggregations.primary_terms.buckets: 2 }
99+
- match: { aggregations.primary_terms.buckets.0.key: "bar" }
100+
- match: { aggregations.primary_terms.buckets.0.doc_count: 2 }
101+
- match: { aggregations.primary_terms.buckets.1.key: "zwomp" }
102+
- match: { aggregations.primary_terms.buckets.1.doc_count: 1 }
103+
---
104+
"Key does not exist, missing specified":
105+
- skip:
106+
version: " - 7.17.4, 8.0.0 - 8.3.99"
107+
reason: "fixed in 8.4"
108+
- do:
109+
search:
110+
rest_total_hits_as_int: true
111+
body:
112+
size: 0
113+
aggs:
114+
primary_terms:
115+
terms:
116+
field: "flattened.bogus"
117+
missing: "missing_val"
118+
119+
- match: { hits.total: 4 }
120+
- length: { aggregations.primary_terms.buckets: 1 }
121+
- match: { aggregations.primary_terms.buckets.0.key: "missing_val" }
122+
- match: { aggregations.primary_terms.buckets.0.doc_count: 4 }
123+
---
124+
"Key does not exist, missing not specified":
125+
- skip:
126+
version: " - 7.17.4, 8.0.0 - 8.3.99"
127+
reason: "fixed in 8.4"
128+
- do:
129+
search:
130+
rest_total_hits_as_int: true
131+
body:
132+
size: 0
133+
aggs:
134+
primary_terms:
135+
terms:
136+
field: "flattened.bogus"
137+
138+
- match: { hits.total: 4 }
139+
- length: { aggregations.primary_terms.buckets: 0 }

‎server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorFactory.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@
88

99
package org.elasticsearch.search.aggregations.bucket.terms;
1010

11+
import org.apache.logging.log4j.LogManager;
12+
import org.apache.logging.log4j.Logger;
1113
import org.apache.lucene.index.DocValues;
1214
import org.apache.lucene.index.IndexReader;
1315
import org.apache.lucene.index.SortedSetDocValues;
@@ -44,6 +46,8 @@
4446
public class TermsAggregatorFactory extends ValuesSourceAggregatorFactory {
4547
static Boolean REMAP_GLOBAL_ORDS, COLLECT_SEGMENT_ORDS;
4648

49+
private static final Logger logger = LogManager.getLogger(TermsAggregatorFactory.class);
50+
4751
static void registerAggregators(ValuesSourceRegistry.Builder builder) {
4852
builder.register(
4953
TermsAggregationBuilder.REGISTRY_KEY,
@@ -127,6 +131,7 @@ public Aggregator build(
127131
}
128132

129133
// TODO: [Zach] we might want refactor and remove ExecutionMode#create(), moving that logic outside the enum
134+
logger.debug("Creating bytes terms aggregator with execution mode [{}]", execution);
130135
return execution.create(
131136
name,
132137
factories,
@@ -443,6 +448,7 @@ Aggregator create(
443448
* any more times doing filter-by-filter then we would
444449
* doing regular collection.
445450
*/
451+
logger.debug("Using adapted fiter-by-filter implementation");
446452
return adapted;
447453
}
448454
}
@@ -464,6 +470,7 @@ Aggregator create(
464470
* - the maximum global ordinal is less than 2048 (LOW_CARDINALITY has additional memory usage,
465471
* which directly linked to maxOrd, so we need to limit).
466472
*/
473+
logger.debug("Using low cardinality global ordinals implementation");
467474
return new GlobalOrdinalsStringTermsAggregator.LowCardinality(
468475
name,
469476
factories,
@@ -507,6 +514,7 @@ Aggregator create(
507514
remapGlobalOrds = false;
508515
}
509516
}
517+
logger.debug("Using standard global ordinals implementation. remap is [{}]", remapGlobalOrds);
510518
return new GlobalOrdinalsStringTermsAggregator(
511519
name,
512520
factories,

‎server/src/main/java/org/elasticsearch/search/aggregations/support/MissingValues.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -226,6 +226,11 @@ public LongUnaryOperator globalOrdinalsMapping(LeafReaderContext context) throws
226226
);
227227
}
228228

229+
@Override
230+
public boolean supportsGlobalOrdinalsMapping() {
231+
return valuesSource.supportsGlobalOrdinalsMapping();
232+
}
233+
229234
@Override
230235
public String toString() {
231236
return "anon ValuesSource.Bytes.WithOrdinals of [" + super.toString() + "]";

‎server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,10 @@ public LongUnaryOperator globalOrdinalsMapping(LeafReaderContext context) throws
152152
return LongUnaryOperator.identity();
153153
}
154154

155+
@Override
156+
public boolean supportsGlobalOrdinalsMapping() {
157+
return true;
158+
}
155159
};
156160

157161
@Override
@@ -222,9 +226,7 @@ public DocValueBits docsWithValue(LeafReaderContext context) throws IOException
222226
* by returning the underlying {@link OrdinalMap}. If this method returns false, then calling
223227
* {@link #globalOrdinalsMapping} will result in an {@link UnsupportedOperationException}.
224228
*/
225-
public boolean supportsGlobalOrdinalsMapping() {
226-
return true;
227-
}
229+
public abstract boolean supportsGlobalOrdinalsMapping();
228230

229231
@Override
230232
public boolean hasOrdinals() {

0 commit comments

Comments
 (0)