Skip to content

Commit 49f1cec

Browse files
jtibshiranijtibshirani
committed
Ensure that ip_range aggregations always return bucket keys. (#30701)
(cherry picked from commit 638a719)
1 parent 36d940f commit 49f1cec

File tree

6 files changed

+80
-71
lines changed

6 files changed

+80
-71
lines changed

‎docs/reference/aggregations/bucket/iprange-aggregation.asciidoc‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,10 +37,12 @@ Response:
3737
"ip_ranges": {
3838
"buckets" : [
3939
{
40+
"key": "*-10.0.0.5",
4041
"to": "10.0.0.5",
4142
"doc_count": 10
4243
},
4344
{
45+
"key": "10.0.0.5-*",
4446
"from": "10.0.0.5",
4547
"doc_count": 260
4648
}

‎rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/40_range.yml‎

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -147,25 +147,18 @@ setup:
147147

148148
- length: { aggregations.ip_range.buckets: 3 }
149149

150-
# ip_range does not automatically add keys to buckets, see #21045
151-
# - match: { aggregations.ip_range.buckets.0.key: "*-192.168.0.0" }
152-
153150
- is_false: aggregations.ip_range.buckets.0.from
154151

155152
- match: { aggregations.ip_range.buckets.0.to: "192.168.0.0" }
156153

157154
- match: { aggregations.ip_range.buckets.0.doc_count: 1 }
158155

159-
# - match: { aggregations.ip_range.buckets.1.key: "192.168.0.0-192.169.0.0" }
160-
161156
- match: { aggregations.ip_range.buckets.1.from: "192.168.0.0" }
162157

163158
- match: { aggregations.ip_range.buckets.1.to: "192.169.0.0" }
164159

165160
- match: { aggregations.ip_range.buckets.1.doc_count: 2 }
166161

167-
# - match: { aggregations.ip_range.buckets.2.key: "192.169.0.0-*" }
168-
169162
- match: { aggregations.ip_range.buckets.2.from: "192.169.0.0" }
170163

171164
- is_false: aggregations.ip_range.buckets.2.to
@@ -180,24 +173,18 @@ setup:
180173

181174
- length: { aggregations.ip_range.buckets: 3 }
182175

183-
# - match: { aggregations.ip_range.buckets.0.key: "*-192.168.0.0" }
184-
185176
- is_false: aggregations.ip_range.buckets.0.from
186177

187178
- match: { aggregations.ip_range.buckets.0.to: "192.168.0.0" }
188179

189180
- match: { aggregations.ip_range.buckets.0.doc_count: 1 }
190181

191-
# - match: { aggregations.ip_range.buckets.1.key: "192.168.0.0-192.169.0.0" }
192-
193182
- match: { aggregations.ip_range.buckets.1.from: "192.168.0.0" }
194183

195184
- match: { aggregations.ip_range.buckets.1.to: "192.169.0.0" }
196185

197186
- match: { aggregations.ip_range.buckets.1.doc_count: 2 }
198187

199-
# - match: { aggregations.ip_range.buckets.2.key: "192.169.0.0-*" }
200-
201188
- match: { aggregations.ip_range.buckets.2.from: "192.169.0.0" }
202189

203190
- is_false: aggregations.ip_range.buckets.2.to
@@ -226,6 +213,21 @@ setup:
226213

227214
- match: { aggregations.ip_range.buckets.1.doc_count: 2 }
228215

216+
---
217+
"IP Range Key Generation":
218+
- skip:
219+
version: " - 6.3.99"
220+
reason: "Before 6.4.0, ip_range did not always generate bucket keys (see #21045)."
221+
222+
- do:
223+
search:
224+
body: { "size" : 0, "aggs" : { "ip_range" : { "ip_range" : { "field" : "ip", "ranges": [ { "to": "192.168.0.0" }, { "from": "192.168.0.0", "to": "192.169.0.0" }, { "from": "192.169.0.0" } ] } } } }
225+
226+
- length: { aggregations.ip_range.buckets: 3 }
227+
- match: { aggregations.ip_range.buckets.0.key: "*-192.168.0.0" }
228+
- match: { aggregations.ip_range.buckets.1.key: "192.168.0.0-192.169.0.0" }
229+
- match: { aggregations.ip_range.buckets.2.key: "192.169.0.0-*" }
230+
229231
---
230232
"Date range":
231233
- skip:

‎server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRange.java‎

Lines changed: 28 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
package org.elasticsearch.search.aggregations.bucket.range;
2121

2222
import org.apache.lucene.util.BytesRef;
23+
import org.elasticsearch.Version;
2324
import org.elasticsearch.common.io.stream.StreamInput;
2425
import org.elasticsearch.common.io.stream.StreamOutput;
2526
import org.elasticsearch.common.xcontent.XContentBuilder;
@@ -57,35 +58,41 @@ public Bucket(DocValueFormat format, boolean keyed, String key, BytesRef from, B
5758
long docCount, InternalAggregations aggregations) {
5859
this.format = format;
5960
this.keyed = keyed;
60-
this.key = key;
61+
this.key = key != null ? key : generateKey(from, to, format);
6162
this.from = from;
6263
this.to = to;
6364
this.docCount = docCount;
6465
this.aggregations = aggregations;
6566
}
6667

67-
// for serialization
68-
private Bucket(StreamInput in, DocValueFormat format, boolean keyed) throws IOException {
69-
this.format = format;
70-
this.keyed = keyed;
71-
key = in.readOptionalString();
72-
if (in.readBoolean()) {
73-
from = in.readBytesRef();
74-
} else {
75-
from = null;
76-
}
77-
if (in.readBoolean()) {
78-
to = in.readBytesRef();
79-
} else {
80-
to = null;
81-
}
82-
docCount = in.readLong();
83-
aggregations = InternalAggregations.readAggregations(in);
68+
private static String generateKey(BytesRef from, BytesRef to, DocValueFormat format) {
69+
StringBuilder builder = new StringBuilder()
70+
.append(from == null ? "*" : format.format(from))
71+
.append("-")
72+
.append(to == null ? "*" : format.format(to));
73+
return builder.toString();
74+
}
75+
76+
private static Bucket createFromStream(StreamInput in, DocValueFormat format, boolean keyed) throws IOException {
77+
String key = in.getVersion().onOrAfter(Version.V_6_4_0)
78+
? in.readString()
79+
: in.readOptionalString();
80+
81+
BytesRef from = in.readBoolean() ? in.readBytesRef() : null;
82+
BytesRef to = in.readBoolean() ? in.readBytesRef() : null;
83+
long docCount = in.readLong();
84+
InternalAggregations aggregations = InternalAggregations.readAggregations(in);
85+
86+
return new Bucket(format, keyed, key, from, to, docCount, aggregations);
8487
}
8588

8689
@Override
8790
public void writeTo(StreamOutput out) throws IOException {
88-
out.writeOptionalString(key);
91+
if (out.getVersion().onOrAfter(Version.V_6_4_0)) {
92+
out.writeString(key);
93+
} else {
94+
out.writeOptionalString(key);
95+
}
8996
out.writeBoolean(from != null);
9097
if (from != null) {
9198
out.writeBytesRef(from);
@@ -122,19 +129,10 @@ public Aggregations getAggregations() {
122129
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
123130
String key = this.key;
124131
if (keyed) {
125-
if (key == null) {
126-
StringBuilder keyBuilder = new StringBuilder();
127-
keyBuilder.append(from == null ? "*" : format.format(from));
128-
keyBuilder.append("-");
129-
keyBuilder.append(to == null ? "*" : format.format(to));
130-
key = keyBuilder.toString();
131-
}
132132
builder.startObject(key);
133133
} else {
134134
builder.startObject();
135-
if (key != null) {
136-
builder.field(CommonFields.KEY.getPreferredName(), key);
137-
}
135+
builder.field(CommonFields.KEY.getPreferredName(), key);
138136
}
139137
if (from != null) {
140138
builder.field(CommonFields.FROM.getPreferredName(), getFrom());
@@ -208,10 +206,9 @@ public InternalBinaryRange(StreamInput in) throws IOException {
208206
super(in);
209207
format = in.readNamedWriteable(DocValueFormat.class);
210208
keyed = in.readBoolean();
211-
buckets = in.readList(stream -> new Bucket(stream, format, keyed));
209+
buckets = in.readList(stream -> Bucket.createFromStream(stream, format, keyed));
212210
}
213211

214-
215212
@Override
216213
protected void doWriteTo(StreamOutput out) throws IOException {
217214
out.writeNamedWriteable(format);

‎server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/ParsedBinaryRange.java‎

Lines changed: 5 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -98,18 +98,16 @@ public String getToAsString() {
9898
@Override
9999
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
100100
if (isKeyed()) {
101-
builder.startObject(key != null ? key : rangeKey(from, to));
101+
builder.startObject(key);
102102
} else {
103103
builder.startObject();
104-
if (key != null) {
105-
builder.field(CommonFields.KEY.getPreferredName(), key);
106-
}
104+
builder.field(CommonFields.KEY.getPreferredName(), key);
107105
}
108106
if (from != null) {
109-
builder.field(CommonFields.FROM.getPreferredName(), getFrom());
107+
builder.field(CommonFields.FROM.getPreferredName(), from);
110108
}
111109
if (to != null) {
112-
builder.field(CommonFields.TO.getPreferredName(), getTo());
110+
builder.field(CommonFields.TO.getPreferredName(), to);
113111
}
114112
builder.field(CommonFields.DOC_COUNT.getPreferredName(), getDocCount());
115113
getAggregations().toXContentInternal(builder, params);
@@ -123,10 +121,9 @@ static ParsedBucket fromXContent(final XContentParser parser, final boolean keye
123121
XContentParser.Token token = parser.currentToken();
124122
String currentFieldName = parser.currentName();
125123

126-
String rangeKey = null;
127124
if (keyed) {
128125
ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser::getTokenLocation);
129-
rangeKey = currentFieldName;
126+
bucket.key = currentFieldName;
130127
ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser::getTokenLocation);
131128
}
132129

@@ -150,19 +147,7 @@ static ParsedBucket fromXContent(final XContentParser parser, final boolean keye
150147
}
151148
}
152149
bucket.setAggregations(new Aggregations(aggregations));
153-
154-
if (keyed) {
155-
if (rangeKey(bucket.from, bucket.to).equals(rangeKey)) {
156-
bucket.key = null;
157-
} else {
158-
bucket.key = rangeKey;
159-
}
160-
}
161150
return bucket;
162151
}
163-
164-
private static String rangeKey(String from, String to) {
165-
return (from == null ? "*" : from) + '-' + (to == null ? "*" : to);
166-
}
167152
}
168153
}

‎server/src/test/java/org/elasticsearch/search/aggregations/bucket/IpRangeIT.java‎

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,8 @@
1818
*/
1919
package org.elasticsearch.search.aggregations.bucket;
2020

21-
import java.util.Arrays;
22-
import java.util.Collection;
23-
import java.util.Collections;
24-
import java.util.Map;
25-
import java.util.function.Function;
26-
27-
import org.elasticsearch.action.search.SearchResponse;
2821
import org.elasticsearch.action.search.SearchPhaseExecutionException;
22+
import org.elasticsearch.action.search.SearchResponse;
2923
import org.elasticsearch.cluster.health.ClusterHealthStatus;
3024
import org.elasticsearch.plugins.Plugin;
3125
import org.elasticsearch.script.MockScriptPlugin;
@@ -35,6 +29,12 @@
3529
import org.elasticsearch.search.aggregations.bucket.range.Range;
3630
import org.elasticsearch.test.ESIntegTestCase;
3731

32+
import java.util.Arrays;
33+
import java.util.Collection;
34+
import java.util.Collections;
35+
import java.util.Map;
36+
import java.util.function.Function;
37+
3838
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
3939
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchResponse;
4040
import static org.hamcrest.Matchers.containsString;
@@ -91,16 +91,19 @@ public void testSingleValuedField() {
9191
Range.Bucket bucket1 = range.getBuckets().get(0);
9292
assertNull(bucket1.getFrom());
9393
assertEquals("192.168.1.0", bucket1.getTo());
94+
assertEquals("*-192.168.1.0", bucket1.getKey());
9495
assertEquals(0, bucket1.getDocCount());
9596

9697
Range.Bucket bucket2 = range.getBuckets().get(1);
9798
assertEquals("192.168.1.0", bucket2.getFrom());
9899
assertEquals("192.168.1.10", bucket2.getTo());
100+
assertEquals("192.168.1.0-192.168.1.10", bucket2.getKey());
99101
assertEquals(1, bucket2.getDocCount());
100102

101103
Range.Bucket bucket3 = range.getBuckets().get(2);
102104
assertEquals("192.168.1.10", bucket3.getFrom());
103105
assertNull(bucket3.getTo());
106+
assertEquals("192.168.1.10-*", bucket3.getKey());
104107
assertEquals(2, bucket3.getDocCount());
105108
}
106109

@@ -118,16 +121,19 @@ public void testMultiValuedField() {
118121
Range.Bucket bucket1 = range.getBuckets().get(0);
119122
assertNull(bucket1.getFrom());
120123
assertEquals("192.168.1.0", bucket1.getTo());
124+
assertEquals("*-192.168.1.0", bucket1.getKey());
121125
assertEquals(1, bucket1.getDocCount());
122126

123127
Range.Bucket bucket2 = range.getBuckets().get(1);
124128
assertEquals("192.168.1.0", bucket2.getFrom());
125129
assertEquals("192.168.1.10", bucket2.getTo());
130+
assertEquals("192.168.1.0-192.168.1.10", bucket2.getKey());
126131
assertEquals(1, bucket2.getDocCount());
127132

128133
Range.Bucket bucket3 = range.getBuckets().get(2);
129134
assertEquals("192.168.1.10", bucket3.getFrom());
130135
assertNull(bucket3.getTo());
136+
assertEquals("192.168.1.10-*", bucket3.getKey());
131137
assertEquals(2, bucket3.getDocCount());
132138
}
133139

@@ -169,16 +175,19 @@ public void testPartiallyUnmapped() {
169175
Range.Bucket bucket1 = range.getBuckets().get(0);
170176
assertNull(bucket1.getFrom());
171177
assertEquals("192.168.1.0", bucket1.getTo());
178+
assertEquals("*-192.168.1.0", bucket1.getKey());
172179
assertEquals(0, bucket1.getDocCount());
173180

174181
Range.Bucket bucket2 = range.getBuckets().get(1);
175182
assertEquals("192.168.1.0", bucket2.getFrom());
176183
assertEquals("192.168.1.10", bucket2.getTo());
184+
assertEquals("192.168.1.0-192.168.1.10", bucket2.getKey());
177185
assertEquals(1, bucket2.getDocCount());
178186

179187
Range.Bucket bucket3 = range.getBuckets().get(2);
180188
assertEquals("192.168.1.10", bucket3.getFrom());
181189
assertNull(bucket3.getTo());
190+
assertEquals("192.168.1.10-*", bucket3.getKey());
182191
assertEquals(2, bucket3.getDocCount());
183192
}
184193

@@ -196,16 +205,19 @@ public void testUnmapped() {
196205
Range.Bucket bucket1 = range.getBuckets().get(0);
197206
assertNull(bucket1.getFrom());
198207
assertEquals("192.168.1.0", bucket1.getTo());
208+
assertEquals("*-192.168.1.0", bucket1.getKey());
199209
assertEquals(0, bucket1.getDocCount());
200210

201211
Range.Bucket bucket2 = range.getBuckets().get(1);
202212
assertEquals("192.168.1.0", bucket2.getFrom());
203213
assertEquals("192.168.1.10", bucket2.getTo());
214+
assertEquals("192.168.1.0-192.168.1.10", bucket2.getKey());
204215
assertEquals(0, bucket2.getDocCount());
205216

206217
Range.Bucket bucket3 = range.getBuckets().get(2);
207218
assertEquals("192.168.1.10", bucket3.getFrom());
208219
assertNull(bucket3.getTo());
220+
assertEquals("192.168.1.10-*", bucket3.getKey());
209221
assertEquals(0, bucket3.getDocCount());
210222
}
211223

‎server/src/test/java/org/elasticsearch/search/aggregations/bucket/range/InternalBinaryRangeTests.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,4 +157,15 @@ protected InternalBinaryRange mutateInstance(InternalBinaryRange instance) {
157157
}
158158
return new InternalBinaryRange(name, format, keyed, buckets, pipelineAggregators, metaData);
159159
}
160+
161+
/**
162+
* Checks the invariant that bucket keys are always non-null, even if null keys
163+
* were originally provided.
164+
*/
165+
public void testKeyGeneration() {
166+
InternalBinaryRange range = createTestInstance();
167+
for (InternalBinaryRange.Bucket bucket : range.getBuckets()) {
168+
assertNotNull(bucket.getKey());
169+
}
170+
}
160171
}

0 commit comments

Comments
 (0)