Skip to content

Commit 957b473

Browse files
nik9000nik9000
authored
Allow serial_diff under min_doc_count aggs (backport of #86401) (#86947)
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 b7a6b02 commit 957b473

File tree

16 files changed

+269
-51
lines changed

16 files changed

+269
-51
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: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import java.util.Map;
2424
import java.util.Optional;
2525
import java.util.Set;
26+
import java.util.function.Consumer;
2627

2728
/**
2829
* A factory that knows how to create an {@link Aggregator} of a specific type.
@@ -189,4 +190,38 @@ public static final class CommonFields extends ParseField.CommonFields {
189190
public String toString() {
190191
return Strings.toString(this);
191192
}
193+
194+
/**
195+
* Return true if any of the child aggregations is a time-series aggregation that requires an in-order execution
196+
*/
197+
public boolean isInSortOrderExecutionRequired() {
198+
for (AggregationBuilder builder : factoriesBuilder.getAggregatorFactories()) {
199+
if (builder.isInSortOrderExecutionRequired()) {
200+
return true;
201+
}
202+
}
203+
return false;
204+
}
205+
206+
/**
207+
* Called by aggregations whose parents must be sequentially ordered.
208+
* @param type the type of the aggregation being validated
209+
* @param name the name of the aggregation being validated
210+
* @param addValidationError callback to add validation errors
211+
*/
212+
protected void validateSequentiallyOrdered(String type, String name, Consumer<String> addValidationError) {
213+
addValidationError.accept(
214+
type + " aggregation [" + name + "] must have a histogram, date_histogram or auto_date_histogram as parent"
215+
);
216+
}
217+
218+
/**
219+
* Called by aggregations whose parents must be sequentially ordered without any gaps.
220+
* @param type the type of the aggregation being validated
221+
* @param name the name of the aggregation being validated
222+
* @param addValidationError callback to add validation errors
223+
*/
224+
protected void validateSequentiallyOrderedWithoutGaps(String type, String name, Consumer<String> addValidationError) {
225+
validateSequentiallyOrdered(type, name, addValidationError);
226+
}
192227
}

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

Lines changed: 20 additions & 20 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,23 +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) {
165-
HistogramAggregationBuilder histoParent = (HistogramAggregationBuilder) parent;
166-
if (histoParent.minDocCount() != 0) {
167-
addValidationError("parent histogram of " + type + " aggregation [" + name + "] must have min_doc_count of 0");
168-
}
169-
} else if (parent instanceof DateHistogramAggregationBuilder) {
170-
DateHistogramAggregationBuilder histoParent = (DateHistogramAggregationBuilder) parent;
171-
if (histoParent.minDocCount() != 0) {
172-
addValidationError("parent histogram of " + type + " aggregation [" + name + "] must have min_doc_count of 0");
173-
}
174-
} else if (parent instanceof AutoDateHistogramAggregationBuilder) {
175-
// Nothing to check
176-
} else {
177-
addValidationError(
178-
type + " aggregation [" + name + "] must have a histogram, date_histogram or auto_date_histogram as parent"
179-
);
180-
}
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);
181176
}
182177
}
183178

@@ -218,6 +213,11 @@ public void addBucketPathValidationError(String error) {
218213
*/
219214
public abstract void validateParentAggSequentiallyOrdered(String type, String name);
220215

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+
221221
/**
222222
* The validation exception, if there is one. It'll be {@code null}
223223
* 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
@@ -35,6 +35,7 @@
3535
import java.util.HashMap;
3636
import java.util.Map;
3737
import java.util.Objects;
38+
import java.util.function.Consumer;
3839

3940
public class AutoDateHistogramAggregationBuilder extends ValuesSourceAggregationBuilder<AutoDateHistogramAggregationBuilder> {
4041

@@ -354,4 +355,12 @@ public String toString() {
354355
return "RoundingInfo[" + rounding + " " + Arrays.toString(innerIntervals) + "]";
355356
}
356357
}
358+
359+
@Override
360+
protected void validateSequentiallyOrdered(String type, String name, Consumer<String> addValidationError) {}
361+
362+
@Override
363+
protected void validateSequentiallyOrderedWithoutGaps(String type, String name, Consumer<String> addValidationError) {
364+
// auto_date_histogram never has gaps
365+
}
357366
}

‎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
@@ -36,6 +36,7 @@
3636
import java.util.List;
3737
import java.util.Map;
3838
import java.util.Objects;
39+
import java.util.function.Consumer;
3940

4041
import static java.util.Collections.unmodifiableMap;
4142

@@ -532,4 +533,14 @@ public boolean equals(Object obj) {
532533
&& Objects.equals(extendedBounds, other.extendedBounds)
533534
&& Objects.equals(hardBounds, other.hardBounds);
534535
}
536+
537+
@Override
538+
protected void validateSequentiallyOrdered(String type, String name, Consumer<String> addValidationError) {}
539+
540+
@Override
541+
protected void validateSequentiallyOrderedWithoutGaps(String type, String name, Consumer<String> addValidationError) {
542+
if (minDocCount != 0) {
543+
addValidationError.accept("parent histogram of " + type + " aggregation [" + name + "] must have min_doc_count of 0");
544+
}
545+
}
535546
}

‎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
@@ -433,4 +434,14 @@ public boolean equals(Object obj) {
433434
&& Objects.equals(extendedBounds, other.extendedBounds)
434435
&& Objects.equals(hardBounds, other.hardBounds);
435436
}
437+
438+
@Override
439+
protected void validateSequentiallyOrdered(String type, String name, Consumer<String> addValidationError) {}
440+
441+
@Override
442+
protected void validateSequentiallyOrderedWithoutGaps(String type, String name, Consumer<String> addValidationError) {
443+
if (minDocCount != 0) {
444+
addValidationError.accept("parent histogram of " + type + " aggregation [" + name + "] must have min_doc_count of 0");
445+
}
446+
}
436447
}

‎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
@@ -96,7 +96,7 @@ protected void validate(ValidationContext context) {
9696
if (bucketsPaths.length != 1) {
9797
context.addBucketPathValidationError("must contain a single entry for aggregation [" + name + "]");
9898
}
99-
context.validateParentAggSequentiallyOrdered(NAME, name);
99+
context.validateParentAggSequentiallyOrderedWithoutSkips(NAME, name);
100100
}
101101

102102
@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
@@ -144,7 +144,7 @@ protected void validate(ValidationContext context) {
144144
);
145145
}
146146

147-
context.validateParentAggSequentiallyOrdered(NAME, name);
147+
context.validateParentAggSequentiallyOrderedWithoutSkips(NAME, name);
148148
}
149149

150150
@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
@@ -173,7 +173,7 @@ protected void validate(ValidationContext context) {
173173
if (window <= 0) {
174174
context.addValidationError("[" + WINDOW.getPreferredName() + "] must be a positive, non-zero integer.");
175175
}
176-
context.validateParentAggSequentiallyOrdered(NAME, name);
176+
context.validateParentAggSequentiallyOrderedWithoutSkips(NAME, name);
177177
}
178178

179179
@Override

0 commit comments

Comments
 (0)