Skip to content

Commit 1ff0ce4

Browse files
nik9000nik9000
authored
Allow serial_diff under min_doc_count aggs (#86401)
Before 6.6 we allowed the `serial_diff` agg in lots of places, including under `date_histogram` aggregations with `min_doc_count: 1`. This allowed you to take the difference of two adjacent buckets, skipping any that don't have any value. So if you have a value at 10am, no value at 11am, and another at noon the `serial_diff` will diff the 10am and noon values. In 6.6 we disabled support for this. We'd like it back.
1 parent ab5ff6f commit 1ff0ce4

File tree

16 files changed

+257
-49
lines changed

16 files changed

+257
-49
lines changed

‎docs/changelog/86401.yaml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 86401
2+
summary: Allow `serial_diff` under `min_doc_count` aggs
3+
area: Aggregations
4+
type: bug
5+
issues: []

‎rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.aggregation/500_serial_diff.yml‎

Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,163 @@
1+
basic:
2+
- do:
3+
bulk:
4+
index: test
5+
refresh: true
6+
body:
7+
- { "index": { } }
8+
- { "@timestamp": "2022-01-01T00:00:00", "v": 1 }
9+
- { "index": { } }
10+
- { "@timestamp": "2022-01-01T01:00:00", "v": 2 }
11+
- { "index": { } }
12+
- { "@timestamp": "2022-01-01T02:00:00", "v": 1 }
13+
14+
- do:
15+
search:
16+
body:
17+
size: 0
18+
aggs:
19+
"@timestamp":
20+
date_histogram:
21+
field: "@timestamp"
22+
fixed_interval: 1h
23+
aggs:
24+
v: {avg: {field: v}}
25+
d: {serial_diff: {buckets_path: v}}
26+
- match: { hits.total.value: 3 }
27+
- length: { [email protected]: 3 }
28+
- match: { [email protected]_as_string: 2022-01-01T00:00:00.000Z }
29+
- match: { [email protected]_as_string: 2022-01-01T01:00:00.000Z }
30+
- match: { [email protected]_as_string: 2022-01-01T02:00:00.000Z }
31+
- match: { [email protected]: 1 }
32+
- match: { [email protected]: 2 }
33+
- match: { [email protected]: 1 }
34+
- is_false: [email protected]
35+
- match: { [email protected]: 1 }
36+
- match: { [email protected]: -1 }
37+
38+
---
39+
lag:
40+
- do:
41+
bulk:
42+
index: test
43+
refresh: true
44+
body:
45+
- { "index": { } }
46+
- { "@timestamp": "2022-01-01T00:00:00", "v": 1 }
47+
- { "index": { } }
48+
- { "@timestamp": "2022-01-01T01:00:00", "v": 2 }
49+
- { "index": { } }
50+
- { "@timestamp": "2022-01-01T02:00:00", "v": 3 }
51+
- { "index": { } }
52+
- { "@timestamp": "2022-01-01T03:00:00", "v": 1 }
53+
54+
- do:
55+
search:
56+
body:
57+
size: 0
58+
aggs:
59+
"@timestamp":
60+
date_histogram:
61+
field: "@timestamp"
62+
fixed_interval: 1h
63+
aggs:
64+
v: { avg: { field: v } }
65+
d: { serial_diff: { buckets_path: v, lag: 2 } }
66+
- match: { hits.total.value: 4 }
67+
- length: { [email protected]: 4 }
68+
- match: { [email protected]_as_string: 2022-01-01T00:00:00.000Z }
69+
- match: { [email protected]_as_string: 2022-01-01T01:00:00.000Z }
70+
- match: { [email protected]_as_string: 2022-01-01T02:00:00.000Z }
71+
- match: { [email protected]_as_string: 2022-01-01T03:00:00.000Z }
72+
- match: { [email protected]: 1 }
73+
- match: { [email protected]: 2 }
74+
- match: { [email protected]: 3 }
75+
- match: { [email protected]: 1 }
76+
- is_false: [email protected]
77+
- is_false: [email protected]
78+
- match: { [email protected]: 2 }
79+
- match: { [email protected]: -1 }
80+
81+
---
82+
parent has gap:
83+
- do:
84+
bulk:
85+
index: test
86+
refresh: true
87+
body:
88+
- { "index": { } }
89+
- { "@timestamp": "2022-01-01T00:00:00", "v": 1 }
90+
- { "index": { } }
91+
- { "@timestamp": "2022-01-01T01:00:00", "v": 2 }
92+
- { "index": { } }
93+
- { "@timestamp": "2022-01-01T03:00:00", "v": 1 }
94+
95+
- do:
96+
search:
97+
body:
98+
size: 0
99+
aggs:
100+
"@timestamp":
101+
date_histogram:
102+
field: "@timestamp"
103+
fixed_interval: 1h
104+
aggs:
105+
v: {avg: {field: v}}
106+
d: {serial_diff: {buckets_path: v}}
107+
- match: { hits.total.value: 3 }
108+
- length: { [email protected]: 4 }
109+
- match: { [email protected]_as_string: 2022-01-01T00:00:00.000Z }
110+
- match: { [email protected]_as_string: 2022-01-01T01:00:00.000Z }
111+
- match: { [email protected]_as_string: 2022-01-01T02:00:00.000Z }
112+
- match: { [email protected]_as_string: 2022-01-01T03:00:00.000Z }
113+
- match: { [email protected]: 1 }
114+
- match: { [email protected]: 2 }
115+
- is_false: [email protected]
116+
- match: { [email protected]: 1 }
117+
- is_false: [email protected]
118+
- match: { [email protected]: 1 }
119+
- is_false: [email protected]
120+
- is_false: [email protected]
121+
122+
---
123+
parent has min_doc_count:
124+
- skip:
125+
version: " - 8.2.99"
126+
reason: allowed in 8.3.0
127+
128+
- do:
129+
bulk:
130+
index: test
131+
refresh: true
132+
body:
133+
- { "index": { } }
134+
- { "@timestamp": "2022-01-01T00:00:00", "v": 1 }
135+
- { "index": { } }
136+
- { "@timestamp": "2022-01-01T01:00:00", "v": 2 }
137+
- { "index": { } }
138+
- { "@timestamp": "2022-01-01T03:00:00", "v": 1 }
139+
140+
- do:
141+
search:
142+
body:
143+
size: 0
144+
aggs:
145+
"@timestamp":
146+
date_histogram:
147+
field: "@timestamp"
148+
fixed_interval: 1h
149+
min_doc_count: 1
150+
aggs:
151+
v: {avg: {field: v}}
152+
d: {serial_diff: {buckets_path: v}}
153+
- match: { hits.total.value: 3 }
154+
- length: { [email protected]: 3 }
155+
- match: { [email protected]_as_string: 2022-01-01T00:00:00.000Z }
156+
- match: { [email protected]_as_string: 2022-01-01T01:00:00.000Z }
157+
- match: { [email protected]_as_string: 2022-01-01T03:00:00.000Z }
158+
- match: { [email protected]: 1 }
159+
- match: { [email protected]: 2 }
160+
- match: { [email protected]: 1 }
161+
- is_false: [email protected]
162+
- match: { [email protected]: 1 }
163+
- match: { [email protected]: -1 }

‎server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilder.java‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import java.util.Map;
2525
import java.util.Optional;
2626
import java.util.Set;
27+
import java.util.function.Consumer;
2728

2829
/**
2930
* A factory that knows how to create an {@link Aggregator} of a specific type.
@@ -215,4 +216,26 @@ public boolean isInSortOrderExecutionRequired() {
215216
}
216217
return false;
217218
}
219+
220+
/**
221+
* Called by aggregations whose parents must be sequentially ordered.
222+
* @param type the type of the aggregation being validated
223+
* @param name the name of the aggregation being validated
224+
* @param addValidationError callback to add validation errors
225+
*/
226+
protected void validateSequentiallyOrdered(String type, String name, Consumer<String> addValidationError) {
227+
addValidationError.accept(
228+
type + " aggregation [" + name + "] must have a histogram, date_histogram or auto_date_histogram as parent"
229+
);
230+
}
231+
232+
/**
233+
* Called by aggregations whose parents must be sequentially ordered without any gaps.
234+
* @param type the type of the aggregation being validated
235+
* @param name the name of the aggregation being validated
236+
* @param addValidationError callback to add validation errors
237+
*/
238+
protected void validateSequentiallyOrderedWithoutGaps(String type, String name, Consumer<String> addValidationError) {
239+
validateSequentiallyOrdered(type, name, addValidationError);
240+
}
218241
}

‎server/src/main/java/org/elasticsearch/search/aggregations/PipelineAggregationBuilder.java‎

Lines changed: 20 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,6 @@
1414
import org.elasticsearch.index.query.QueryRewriteContext;
1515
import org.elasticsearch.index.query.Rewriteable;
1616
import org.elasticsearch.search.aggregations.AggregatorFactories.Builder;
17-
import org.elasticsearch.search.aggregations.bucket.histogram.AutoDateHistogramAggregationBuilder;
18-
import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramAggregationBuilder;
19-
import org.elasticsearch.search.aggregations.bucket.histogram.HistogramAggregationBuilder;
2017
import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;
2118
import org.elasticsearch.xcontent.ToXContentFragment;
2219

@@ -127,6 +124,15 @@ public void validateHasParent(String type, String name) {
127124

128125
@Override
129126
public void validateParentAggSequentiallyOrdered(String type, String name) {
127+
noParentCantBeOrdered(type, name);
128+
}
129+
130+
@Override
131+
public void validateParentAggSequentiallyOrderedWithoutSkips(String type, String name) {
132+
noParentCantBeOrdered(type, name);
133+
}
134+
135+
private void noParentCantBeOrdered(String type, String name) {
130136
addValidationError(
131137
type
132138
+ " aggregation ["
@@ -161,21 +167,12 @@ public void validateHasParent(String type, String name) {
161167

162168
@Override
163169
public void validateParentAggSequentiallyOrdered(String type, String name) {
164-
if (parent instanceof HistogramAggregationBuilder histoParent) {
165-
if (histoParent.minDocCount() != 0) {
166-
addValidationError("parent histogram of " + type + " aggregation [" + name + "] must have min_doc_count of 0");
167-
}
168-
} else if (parent instanceof DateHistogramAggregationBuilder histoParent) {
169-
if (histoParent.minDocCount() != 0) {
170-
addValidationError("parent histogram of " + type + " aggregation [" + name + "] must have min_doc_count of 0");
171-
}
172-
} else if (parent instanceof AutoDateHistogramAggregationBuilder) {
173-
// Nothing to check
174-
} else {
175-
addValidationError(
176-
type + " aggregation [" + name + "] must have a histogram, date_histogram or auto_date_histogram as parent"
177-
);
178-
}
170+
parent.validateSequentiallyOrdered(type, name, this::addValidationError);
171+
}
172+
173+
@Override
174+
public void validateParentAggSequentiallyOrderedWithoutSkips(String type, String name) {
175+
parent.validateSequentiallyOrderedWithoutGaps(type, name, this::addValidationError);
179176
}
180177
}
181178

@@ -216,6 +213,11 @@ public void addBucketPathValidationError(String error) {
216213
*/
217214
public abstract void validateParentAggSequentiallyOrdered(String type, String name);
218215

216+
/**
217+
* Validates that the parent is sequentially ordered and doesn't have any gps.
218+
*/
219+
public abstract void validateParentAggSequentiallyOrderedWithoutSkips(String type, String name);
220+
219221
/**
220222
* The validation exception, if there is one. It'll be {@code null}
221223
* if the context wasn't provided with any exception on creation

‎server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregationBuilder.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import java.util.Arrays;
3535
import java.util.Map;
3636
import java.util.Objects;
37+
import java.util.function.Consumer;
3738

3839
import static java.util.Map.entry;
3940

@@ -352,4 +353,12 @@ public String toString() {
352353
return "RoundingInfo[" + rounding + " " + Arrays.toString(innerIntervals) + "]";
353354
}
354355
}
356+
357+
@Override
358+
protected void validateSequentiallyOrdered(String type, String name, Consumer<String> addValidationError) {}
359+
360+
@Override
361+
protected void validateSequentiallyOrderedWithoutGaps(String type, String name, Consumer<String> addValidationError) {
362+
// auto_date_histogram never has gaps
363+
}
355364
}

‎server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramAggregationBuilder.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import java.util.List;
3636
import java.util.Map;
3737
import java.util.Objects;
38+
import java.util.function.Consumer;
3839

3940
import static java.util.Map.entry;
4041

@@ -501,4 +502,14 @@ public boolean equals(Object obj) {
501502
public Version getMinimalSupportedVersion() {
502503
return Version.V_EMPTY;
503504
}
505+
506+
@Override
507+
protected void validateSequentiallyOrdered(String type, String name, Consumer<String> addValidationError) {}
508+
509+
@Override
510+
protected void validateSequentiallyOrderedWithoutGaps(String type, String name, Consumer<String> addValidationError) {
511+
if (minDocCount != 0) {
512+
addValidationError.accept("parent histogram of " + type + " aggregation [" + name + "] must have min_doc_count of 0");
513+
}
514+
}
504515
}

‎server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
import java.util.List;
3333
import java.util.Map;
3434
import java.util.Objects;
35+
import java.util.function.Consumer;
3536

3637
/**
3738
* A builder for histograms on numeric fields. This builder can operate on either base numeric fields, or numeric range fields. IP range
@@ -443,4 +444,14 @@ public boolean equals(Object obj) {
443444
public Version getMinimalSupportedVersion() {
444445
return Version.V_EMPTY;
445446
}
447+
448+
@Override
449+
protected void validateSequentiallyOrdered(String type, String name, Consumer<String> addValidationError) {}
450+
451+
@Override
452+
protected void validateSequentiallyOrderedWithoutGaps(String type, String name, Consumer<String> addValidationError) {
453+
if (minDocCount != 0) {
454+
addValidationError.accept("parent histogram of " + type + " aggregation [" + name + "] must have min_doc_count of 0");
455+
}
456+
}
446457
}

‎server/src/main/java/org/elasticsearch/search/aggregations/pipeline/CumulativeSumPipelineAggregationBuilder.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ protected void validate(ValidationContext context) {
9797
if (bucketsPaths.length != 1) {
9898
context.addBucketPathValidationError("must contain a single entry for aggregation [" + name + "]");
9999
}
100-
context.validateParentAggSequentiallyOrdered(NAME, name);
100+
context.validateParentAggSequentiallyOrderedWithoutSkips(NAME, name);
101101
}
102102

103103
@Override

‎server/src/main/java/org/elasticsearch/search/aggregations/pipeline/DerivativePipelineAggregationBuilder.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ protected void validate(ValidationContext context) {
145145
);
146146
}
147147

148-
context.validateParentAggSequentiallyOrdered(NAME, name);
148+
context.validateParentAggSequentiallyOrderedWithoutSkips(NAME, name);
149149
}
150150

151151
@Override

‎server/src/main/java/org/elasticsearch/search/aggregations/pipeline/MovFnPipelineAggregationBuilder.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ protected void validate(ValidationContext context) {
167167
if (window <= 0) {
168168
context.addValidationError("[" + WINDOW.getPreferredName() + "] must be a positive, non-zero integer.");
169169
}
170-
context.validateParentAggSequentiallyOrdered(NAME, name);
170+
context.validateParentAggSequentiallyOrderedWithoutSkips(NAME, name);
171171
}
172172

173173
@Override

0 commit comments

Comments
 (0)