Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow serial_diff under min_doc_count aggs #86401

Merged
merged 5 commits into from
May 19, 2022
Merged

Allow serial_diff under min_doc_count aggs #86401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9e13fd7
Allow serial_diff under min_doc_count aggs
nik9000 May 3, 2022
782540c
Update docs/changelog/86401.yaml
nik9000 May 3, 2022
0157364
Fix test
nik9000 May 3, 2022
2290dd7
Merge branch 'master' into serial_diff_gaps_ok
nik9000 May 18, 2022
b7d76ad
Clean
nik9000 May 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/changelog/86401.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 86401
summary: Allow `serial_diff` under `min_doc_count` aggs
area: Aggregations
type: bug
issues: []
163 changes: 163 additions & 0 deletions ...spec/src/yamlRestTest/resources/rest-api-spec/test/search.aggregation/500_serial_diff.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
basic:
- do:
bulk:
index: test
refresh: true
body:
- { "index": { } }
- { "@timestamp": "2022-01-01T00:00:00", "v": 1 }
- { "index": { } }
- { "@timestamp": "2022-01-01T01:00:00", "v": 2 }
- { "index": { } }
- { "@timestamp": "2022-01-01T02:00:00", "v": 1 }

- do:
search:
body:
size: 0
aggs:
"@timestamp":
date_histogram:
field: "@timestamp"
fixed_interval: 1h
aggs:
v: {avg: {field: v}}
d: {serial_diff: {buckets_path: v}}
- match: { hits.total.value: 3 }
- length: { aggregations.@timestamp.buckets: 3 }
- match: { aggregations.@timestamp.buckets.0.key_as_string: 2022-01-01T00:00:00.000Z }
- match: { aggregations.@timestamp.buckets.1.key_as_string: 2022-01-01T01:00:00.000Z }
- match: { aggregations.@timestamp.buckets.2.key_as_string: 2022-01-01T02:00:00.000Z }
- match: { aggregations.@timestamp.buckets.0.v.value: 1 }
- match: { aggregations.@timestamp.buckets.1.v.value: 2 }
- match: { aggregations.@timestamp.buckets.2.v.value: 1 }
- is_false: aggregations.@timestamp.buckets.0.d
- match: { aggregations.@timestamp.buckets.1.d.value: 1 }
- match: { aggregations.@timestamp.buckets.2.d.value: -1 }

---
lag:
- do:
bulk:
index: test
refresh: true
body:
- { "index": { } }
- { "@timestamp": "2022-01-01T00:00:00", "v": 1 }
- { "index": { } }
- { "@timestamp": "2022-01-01T01:00:00", "v": 2 }
- { "index": { } }
- { "@timestamp": "2022-01-01T02:00:00", "v": 3 }
- { "index": { } }
- { "@timestamp": "2022-01-01T03:00:00", "v": 1 }

- do:
search:
body:
size: 0
aggs:
"@timestamp":
date_histogram:
field: "@timestamp"
fixed_interval: 1h
aggs:
v: { avg: { field: v } }
d: { serial_diff: { buckets_path: v, lag: 2 } }
- match: { hits.total.value: 4 }
- length: { aggregations.@timestamp.buckets: 4 }
- match: { aggregations.@timestamp.buckets.0.key_as_string: 2022-01-01T00:00:00.000Z }
- match: { aggregations.@timestamp.buckets.1.key_as_string: 2022-01-01T01:00:00.000Z }
- match: { aggregations.@timestamp.buckets.2.key_as_string: 2022-01-01T02:00:00.000Z }
- match: { aggregations.@timestamp.buckets.3.key_as_string: 2022-01-01T03:00:00.000Z }
- match: { aggregations.@timestamp.buckets.0.v.value: 1 }
- match: { aggregations.@timestamp.buckets.1.v.value: 2 }
- match: { aggregations.@timestamp.buckets.2.v.value: 3 }
- match: { aggregations.@timestamp.buckets.3.v.value: 1 }
- is_false: aggregations.@timestamp.buckets.0.d
- is_false: aggregations.@timestamp.buckets.1.d
- match: { aggregations.@timestamp.buckets.2.d.value: 2 }
- match: { aggregations.@timestamp.buckets.3.d.value: -1 }

---
parent has gap:
- do:
bulk:
index: test
refresh: true
body:
- { "index": { } }
- { "@timestamp": "2022-01-01T00:00:00", "v": 1 }
- { "index": { } }
- { "@timestamp": "2022-01-01T01:00:00", "v": 2 }
- { "index": { } }
- { "@timestamp": "2022-01-01T03:00:00", "v": 1 }

- do:
search:
body:
size: 0
aggs:
"@timestamp":
date_histogram:
field: "@timestamp"
fixed_interval: 1h
aggs:
v: {avg: {field: v}}
d: {serial_diff: {buckets_path: v}}
- match: { hits.total.value: 3 }
- length: { aggregations.@timestamp.buckets: 4 }
- match: { aggregations.@timestamp.buckets.0.key_as_string: 2022-01-01T00:00:00.000Z }
- match: { aggregations.@timestamp.buckets.1.key_as_string: 2022-01-01T01:00:00.000Z }
- match: { aggregations.@timestamp.buckets.2.key_as_string: 2022-01-01T02:00:00.000Z }
- match: { aggregations.@timestamp.buckets.3.key_as_string: 2022-01-01T03:00:00.000Z }
- match: { aggregations.@timestamp.buckets.0.v.value: 1 }
- match: { aggregations.@timestamp.buckets.1.v.value: 2 }
- is_false: aggregations.@timestamp.buckets.2.v.value
- match: { aggregations.@timestamp.buckets.3.v.value: 1 }
- is_false: aggregations.@timestamp.buckets.0.d
- match: { aggregations.@timestamp.buckets.1.d.value: 1 }
- is_false: aggregations.@timestamp.buckets.2.d
- is_false: aggregations.@timestamp.buckets.3.d

---
parent has min_doc_count:
- skip:
version: " - 8.2.99"
reason: allowed in 8.3.0

- do:
bulk:
index: test
refresh: true
body:
- { "index": { } }
- { "@timestamp": "2022-01-01T00:00:00", "v": 1 }
- { "index": { } }
- { "@timestamp": "2022-01-01T01:00:00", "v": 2 }
- { "index": { } }
- { "@timestamp": "2022-01-01T03:00:00", "v": 1 }

- do:
search:
body:
size: 0
aggs:
"@timestamp":
date_histogram:
field: "@timestamp"
fixed_interval: 1h
min_doc_count: 1
aggs:
v: {avg: {field: v}}
d: {serial_diff: {buckets_path: v}}
- match: { hits.total.value: 3 }
- length: { aggregations.@timestamp.buckets: 3 }
- match: { aggregations.@timestamp.buckets.0.key_as_string: 2022-01-01T00:00:00.000Z }
- match: { aggregations.@timestamp.buckets.1.key_as_string: 2022-01-01T01:00:00.000Z }
- match: { aggregations.@timestamp.buckets.2.key_as_string: 2022-01-01T03:00:00.000Z }
- match: { aggregations.@timestamp.buckets.0.v.value: 1 }
- match: { aggregations.@timestamp.buckets.1.v.value: 2 }
- match: { aggregations.@timestamp.buckets.2.v.value: 1 }
- is_false: aggregations.@timestamp.buckets.0.d
- match: { aggregations.@timestamp.buckets.1.d.value: 1 }
- match: { aggregations.@timestamp.buckets.2.d.value: -1 }
23 changes: 23 additions & 0 deletions server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.function.Consumer;

/**
* A factory that knows how to create an {@link Aggregator} of a specific type.
Expand Down Expand Up @@ -215,4 +216,26 @@ public boolean isInSortOrderExecutionRequired() {
}
return false;
}

/**
* Called by aggregations whose parents must be sequentially ordered.
* @param type the type of the aggregation being validated
* @param name the name of the aggregation being validated
* @param addValidationError callback to add validation errors
*/
protected void validateSequentiallyOrdered(String type, String name, Consumer<String> addValidationError) {
addValidationError.accept(
type + " aggregation [" + name + "] must have a histogram, date_histogram or auto_date_histogram as parent"
);
}

/**
* Called by aggregations whose parents must be sequentially ordered without any gaps.
* @param type the type of the aggregation being validated
* @param name the name of the aggregation being validated
* @param addValidationError callback to add validation errors
*/
protected void validateSequentiallyOrderedWithoutGaps(String type, String name, Consumer<String> addValidationError) {
validateSequentiallyOrdered(type, name, addValidationError);
}
}
38 changes: 20 additions & 18 deletions server/src/main/java/org/elasticsearch/search/aggregations/PipelineAggregationBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,6 @@
import org.elasticsearch.index.query.QueryRewriteContext;
import org.elasticsearch.index.query.Rewriteable;
import org.elasticsearch.search.aggregations.AggregatorFactories.Builder;
import org.elasticsearch.search.aggregations.bucket.histogram.AutoDateHistogramAggregationBuilder;
import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramAggregationBuilder;
import org.elasticsearch.search.aggregations.bucket.histogram.HistogramAggregationBuilder;
import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;
import org.elasticsearch.xcontent.ToXContentFragment;

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

@Override
public void validateParentAggSequentiallyOrdered(String type, String name) {
noParentCantBeOrdered(type, name);
}

@Override
public void validateParentAggSequentiallyOrderedWithoutSkips(String type, String name) {
noParentCantBeOrdered(type, name);
}

private void noParentCantBeOrdered(String type, String name) {
addValidationError(
type
+ " aggregation ["
Expand Down Expand Up @@ -161,21 +167,12 @@ public void validateHasParent(String type, String name) {

@Override
public void validateParentAggSequentiallyOrdered(String type, String name) {
if (parent instanceof HistogramAggregationBuilder histoParent) {
if (histoParent.minDocCount() != 0) {
addValidationError("parent histogram of " + type + " aggregation [" + name + "] must have min_doc_count of 0");
}
} else if (parent instanceof DateHistogramAggregationBuilder histoParent) {
if (histoParent.minDocCount() != 0) {
addValidationError("parent histogram of " + type + " aggregation [" + name + "] must have min_doc_count of 0");
}
} else if (parent instanceof AutoDateHistogramAggregationBuilder) {
// Nothing to check
} else {
addValidationError(
type + " aggregation [" + name + "] must have a histogram, date_histogram or auto_date_histogram as parent"
);
}
parent.validateSequentiallyOrdered(type, name, this::addValidationError);
}

@Override
public void validateParentAggSequentiallyOrderedWithoutSkips(String type, String name) {
parent.validateSequentiallyOrderedWithoutGaps(type, name, this::addValidationError);
}
}

Expand Down Expand Up @@ -216,6 +213,11 @@ public void addBucketPathValidationError(String error) {
*/
public abstract void validateParentAggSequentiallyOrdered(String type, String name);

/**
* Validates that the parent is sequentially ordered and doesn't have any gps.
*/
public abstract void validateParentAggSequentiallyOrderedWithoutSkips(String type, String name);

/**
* The validation exception, if there is one. It'll be {@code null}
* if the context wasn't provided with any exception on creation
Expand Down
9 changes: 9 additions & 0 deletions ...asticsearch/search/aggregations/bucket/histogram/AutoDateHistogramAggregationBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import java.util.Arrays;
import java.util.Map;
import java.util.Objects;
import java.util.function.Consumer;

import static java.util.Map.entry;

Expand Down Expand Up @@ -352,4 +353,12 @@ public String toString() {
return "RoundingInfo[" + rounding + " " + Arrays.toString(innerIntervals) + "]";
}
}

@Override
protected void validateSequentiallyOrdered(String type, String name, Consumer<String> addValidationError) {}

@Override
protected void validateSequentiallyOrderedWithoutGaps(String type, String name, Consumer<String> addValidationError) {
// auto_date_histogram never has gaps
}
}
11 changes: 11 additions & 0 deletions ...g/elasticsearch/search/aggregations/bucket/histogram/DateHistogramAggregationBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.function.Consumer;

import static java.util.Map.entry;

Expand Down Expand Up @@ -501,4 +502,14 @@ public boolean equals(Object obj) {
public Version getMinimalSupportedVersion() {
return Version.V_EMPTY;
}

@Override
protected void validateSequentiallyOrdered(String type, String name, Consumer<String> addValidationError) {}

@Override
protected void validateSequentiallyOrderedWithoutGaps(String type, String name, Consumer<String> addValidationError) {
if (minDocCount != 0) {
addValidationError.accept("parent histogram of " + type + " aggregation [" + name + "] must have min_doc_count of 0");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to think about extended/hard bounds here? I'm pretty sure the answer is "no", but I want to say it out loud.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We never have in the past. I think it's ok because those'll just make the result wider without adding any gaps.

}
}
}
11 changes: 11 additions & 0 deletions ...a/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.function.Consumer;

/**
* A builder for histograms on numeric fields. This builder can operate on either base numeric fields, or numeric range fields. IP range
Expand Down Expand Up @@ -443,4 +444,14 @@ public boolean equals(Object obj) {
public Version getMinimalSupportedVersion() {
return Version.V_EMPTY;
}

@Override
protected void validateSequentiallyOrdered(String type, String name, Consumer<String> addValidationError) {}

@Override
protected void validateSequentiallyOrderedWithoutGaps(String type, String name, Consumer<String> addValidationError) {
if (minDocCount != 0) {
addValidationError.accept("parent histogram of " + type + " aggregation [" + name + "] must have min_doc_count of 0");
}
}
}
2 changes: 1 addition & 1 deletion ...g/elasticsearch/search/aggregations/pipeline/CumulativeSumPipelineAggregationBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ protected void validate(ValidationContext context) {
if (bucketsPaths.length != 1) {
context.addBucketPathValidationError("must contain a single entry for aggregation [" + name + "]");
}
context.validateParentAggSequentiallyOrdered(NAME, name);
context.validateParentAggSequentiallyOrderedWithoutSkips(NAME, name);
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion .../org/elasticsearch/search/aggregations/pipeline/DerivativePipelineAggregationBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ protected void validate(ValidationContext context) {
);
}

context.validateParentAggSequentiallyOrdered(NAME, name);
context.validateParentAggSequentiallyOrderedWithoutSkips(NAME, name);
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion .../java/org/elasticsearch/search/aggregations/pipeline/MovFnPipelineAggregationBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ protected void validate(ValidationContext context) {
if (window <= 0) {
context.addValidationError("[" + WINDOW.getPreferredName() + "] must be a positive, non-zero integer.");
}
context.validateParentAggSequentiallyOrdered(NAME, name);
context.validateParentAggSequentiallyOrderedWithoutSkips(NAME, name);
}

@Override
Expand Down
7 changes: 2 additions & 5 deletions server/src/test/java/org/elasticsearch/search/aggregations/pipeline/CumulativeSumTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,13 @@

import org.elasticsearch.search.aggregations.AggregationBuilder;
import org.elasticsearch.search.aggregations.BasePipelineAggregationTestCase;
import org.elasticsearch.search.aggregations.bucket.terms.TermsAggregationBuilder;

import java.io.IOException;

import static java.util.Collections.emptyList;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.nullValue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

public class CumulativeSumTests extends BasePipelineAggregationTestCase<CumulativeSumPipelineAggregationBuilder> {
@Override
Expand All @@ -42,9 +41,7 @@ public void testValidate() throws IOException {
}

public void testInvalidParent() throws IOException {
AggregationBuilder parent = mock(AggregationBuilder.class);
when(parent.getName()).thenReturn("name");

AggregationBuilder parent = new TermsAggregationBuilder("name");
assertThat(
validate(parent, new CumulativeSumPipelineAggregationBuilder("name", "invalid_agg>metric")),
equalTo(
Expand Down
Loading