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

Jump to bottom

Support for aggregation names with dots in first element path of a pipeline aggregation #77481

Merged
merged 4 commits into from
Sep 9, 2021
Merged

Support for aggregation names with dots in first element path of a pipeline aggregation #77481

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5aaa0ef
Support for aggregation names with dots in first element path of a pi…
iverase Sep 9, 2021
6dec35b
checkstyle
iverase Sep 9, 2021
ee08d3a
iter
iverase Sep 9, 2021
7b44697
spotless
iverase Sep 9, 2021
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
97 changes: 56 additions & 41 deletions .../internalClusterTest/java/org/elasticsearch/search/aggregations/pipeline/AvgBucketIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ public class AvgBucketIT extends ESIntegTestCase {
static int numValueBuckets;
static long[] valueCounts;

static String histoName;
static String termsName;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have only do it for one test but maybe we should do it for all of them for consistency? Maybe a follow up PR?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think that'd be good to do in a follow up PR.

@Override
public void setupSuiteScopeCluster() throws Exception {
assertAcked(client().admin().indices().prepareCreate("idx").setMapping("tag", "type=keyword").get());
Expand Down Expand Up @@ -86,21 +89,29 @@ public void setupSuiteScopeCluster() throws Exception {
}
indexRandom(true, builders);
ensureSearchable();
histoName = randomName();
termsName = randomName();
}

private static String randomName() {
return randomBoolean()
? randomAlphaOfLengthBetween(3, 12)
: randomAlphaOfLengthBetween(3, 6) + "." + randomAlphaOfLengthBetween(3, 6);
Copy link
Member

Choose a reason for hiding this comment

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

We should test non-ascii characters too, probably in a follow up PR. Or just open a ticket for it, could be a good first issue for someone.

}

public void testDocCountTopLevel() throws Exception {
SearchResponse response = client().prepareSearch("idx")
.addAggregation(
histogram("histo").field(SINGLE_VALUED_FIELD_NAME).interval(interval).extendedBounds(minRandomValue, maxRandomValue)
histogram(histoName).field(SINGLE_VALUED_FIELD_NAME).interval(interval).extendedBounds(minRandomValue, maxRandomValue)
)
.addAggregation(avgBucket("avg_bucket", "histo>_count"))
.addAggregation(avgBucket("avg_bucket", histoName + ">_count"))
.get();

assertSearchResponse(response);

Histogram histo = response.getAggregations().get("histo");
Histogram histo = response.getAggregations().get(histoName);
assertThat(histo, notNullValue());
assertThat(histo.getName(), equalTo("histo"));
assertThat(histo.getName(), equalTo(histoName));
List<? extends Bucket> buckets = histo.getBuckets();
assertThat(buckets.size(), equalTo(numValueBuckets));

Expand All @@ -125,20 +136,22 @@ public void testDocCountTopLevel() throws Exception {
public void testDocCountAsSubAgg() throws Exception {
SearchResponse response = client().prepareSearch("idx")
.addAggregation(
terms("terms").field("tag")
terms(termsName).field("tag")
.order(BucketOrder.key(true))
.subAggregation(
histogram("histo").field(SINGLE_VALUED_FIELD_NAME).interval(interval).extendedBounds(minRandomValue, maxRandomValue)
histogram(histoName).field(SINGLE_VALUED_FIELD_NAME)
.interval(interval)
.extendedBounds(minRandomValue, maxRandomValue)
)
.subAggregation(avgBucket("avg_bucket", "histo>_count"))
.subAggregation(avgBucket("avg_bucket", histoName + ">_count"))
)
.get();

assertSearchResponse(response);

Terms terms = response.getAggregations().get("terms");
Terms terms = response.getAggregations().get(termsName);
assertThat(terms, notNullValue());
assertThat(terms.getName(), equalTo("terms"));
assertThat(terms.getName(), equalTo(termsName));
List<? extends Terms.Bucket> termsBuckets = terms.getBuckets();
assertThat(termsBuckets.size(), equalTo(interval));

Expand All @@ -147,9 +160,9 @@ public void testDocCountAsSubAgg() throws Exception {
assertThat(termsBucket, notNullValue());
assertThat((String) termsBucket.getKey(), equalTo("tag" + (i % interval)));

Histogram histo = termsBucket.getAggregations().get("histo");
Histogram histo = termsBucket.getAggregations().get(histoName);
assertThat(histo, notNullValue());
assertThat(histo.getName(), equalTo("histo"));
assertThat(histo.getName(), equalTo(histoName));
List<? extends Bucket> buckets = histo.getBuckets();

double sum = 0;
Expand All @@ -172,15 +185,15 @@ public void testDocCountAsSubAgg() throws Exception {

public void testMetricTopLevel() throws Exception {
SearchResponse response = client().prepareSearch("idx")
.addAggregation(terms("terms").field("tag").subAggregation(sum("sum").field(SINGLE_VALUED_FIELD_NAME)))
.addAggregation(avgBucket("avg_bucket", "terms>sum"))
.addAggregation(terms(termsName).field("tag").subAggregation(sum("sum").field(SINGLE_VALUED_FIELD_NAME)))
.addAggregation(avgBucket("avg_bucket", termsName + ">sum"))
.get();

assertSearchResponse(response);

Terms terms = response.getAggregations().get("terms");
Terms terms = response.getAggregations().get(termsName);
assertThat(terms, notNullValue());
assertThat(terms.getName(), equalTo("terms"));
assertThat(terms.getName(), equalTo(termsName));
List<? extends Terms.Bucket> buckets = terms.getBuckets();
assertThat(buckets.size(), equalTo(interval));

Expand All @@ -207,23 +220,23 @@ public void testMetricTopLevel() throws Exception {
public void testMetricAsSubAgg() throws Exception {
SearchResponse response = client().prepareSearch("idx")
.addAggregation(
terms("terms").field("tag")
terms(termsName).field("tag")
.order(BucketOrder.key(true))
.subAggregation(
histogram("histo").field(SINGLE_VALUED_FIELD_NAME)
histogram(histoName).field(SINGLE_VALUED_FIELD_NAME)
.interval(interval)
.extendedBounds(minRandomValue, maxRandomValue)
.subAggregation(sum("sum").field(SINGLE_VALUED_FIELD_NAME))
)
.subAggregation(avgBucket("avg_bucket", "histo>sum"))
.subAggregation(avgBucket("avg_bucket", histoName + ">sum"))
)
.get();

assertSearchResponse(response);

Terms terms = response.getAggregations().get("terms");
Terms terms = response.getAggregations().get(termsName);
assertThat(terms, notNullValue());
assertThat(terms.getName(), equalTo("terms"));
assertThat(terms.getName(), equalTo(termsName));
List<? extends Terms.Bucket> termsBuckets = terms.getBuckets();
assertThat(termsBuckets.size(), equalTo(interval));

Expand All @@ -232,9 +245,9 @@ public void testMetricAsSubAgg() throws Exception {
assertThat(termsBucket, notNullValue());
assertThat((String) termsBucket.getKey(), equalTo("tag" + (i % interval)));

Histogram histo = termsBucket.getAggregations().get("histo");
Histogram histo = termsBucket.getAggregations().get(histoName);
assertThat(histo, notNullValue());
assertThat(histo.getName(), equalTo("histo"));
assertThat(histo.getName(), equalTo(histoName));
List<? extends Bucket> buckets = histo.getBuckets();

double bucketSum = 0;
Expand Down Expand Up @@ -262,23 +275,23 @@ public void testMetricAsSubAgg() throws Exception {
public void testMetricAsSubAggWithInsertZeros() throws Exception {
SearchResponse response = client().prepareSearch("idx")
.addAggregation(
terms("terms").field("tag")
terms(termsName).field("tag")
.order(BucketOrder.key(true))
.subAggregation(
histogram("histo").field(SINGLE_VALUED_FIELD_NAME)
histogram(histoName).field(SINGLE_VALUED_FIELD_NAME)
.interval(interval)
.extendedBounds(minRandomValue, maxRandomValue)
.subAggregation(sum("sum").field(SINGLE_VALUED_FIELD_NAME))
)
.subAggregation(avgBucket("avg_bucket", "histo>sum").gapPolicy(GapPolicy.INSERT_ZEROS))
.subAggregation(avgBucket("avg_bucket", histoName + ">sum").gapPolicy(GapPolicy.INSERT_ZEROS))
)
.get();

assertSearchResponse(response);

Terms terms = response.getAggregations().get("terms");
Terms terms = response.getAggregations().get(termsName);
assertThat(terms, notNullValue());
assertThat(terms.getName(), equalTo("terms"));
assertThat(terms.getName(), equalTo(termsName));
List<? extends Terms.Bucket> termsBuckets = terms.getBuckets();
assertThat(termsBuckets.size(), equalTo(interval));

Expand All @@ -287,9 +300,9 @@ public void testMetricAsSubAggWithInsertZeros() throws Exception {
assertThat(termsBucket, notNullValue());
assertThat((String) termsBucket.getKey(), equalTo("tag" + (i % interval)));

Histogram histo = termsBucket.getAggregations().get("histo");
Histogram histo = termsBucket.getAggregations().get(histoName);
assertThat(histo, notNullValue());
assertThat(histo.getName(), equalTo("histo"));
assertThat(histo.getName(), equalTo(histoName));
List<? extends Bucket> buckets = histo.getBuckets();

double bucketSum = 0;
Expand All @@ -316,18 +329,18 @@ public void testMetricAsSubAggWithInsertZeros() throws Exception {
public void testNoBuckets() throws Exception {
SearchResponse response = client().prepareSearch("idx")
.addAggregation(
terms("terms").field("tag")
terms(termsName).field("tag")
.includeExclude(new IncludeExclude(null, "tag.*"))
.subAggregation(sum("sum").field(SINGLE_VALUED_FIELD_NAME))
)
.addAggregation(avgBucket("avg_bucket", "terms>sum"))
.addAggregation(avgBucket("avg_bucket", termsName + ">sum"))
.get();

assertSearchResponse(response);

Terms terms = response.getAggregations().get("terms");
Terms terms = response.getAggregations().get(termsName);
assertThat(terms, notNullValue());
assertThat(terms.getName(), equalTo("terms"));
assertThat(terms.getName(), equalTo(termsName));
List<? extends Terms.Bucket> buckets = terms.getBuckets();
assertThat(buckets.size(), equalTo(0));

Expand All @@ -340,21 +353,23 @@ public void testNoBuckets() throws Exception {
public void testNested() throws Exception {
SearchResponse response = client().prepareSearch("idx")
.addAggregation(
terms("terms").field("tag")
terms(termsName).field("tag")
.order(BucketOrder.key(true))
.subAggregation(
histogram("histo").field(SINGLE_VALUED_FIELD_NAME).interval(interval).extendedBounds(minRandomValue, maxRandomValue)
histogram(histoName).field(SINGLE_VALUED_FIELD_NAME)
.interval(interval)
.extendedBounds(minRandomValue, maxRandomValue)
)
.subAggregation(avgBucket("avg_histo_bucket", "histo>_count"))
.subAggregation(avgBucket("avg_histo_bucket", histoName + ">_count"))
)
.addAggregation(avgBucket("avg_terms_bucket", "terms>avg_histo_bucket"))
.addAggregation(avgBucket("avg_terms_bucket", termsName + ">avg_histo_bucket"))
.get();

assertSearchResponse(response);

Terms terms = response.getAggregations().get("terms");
Terms terms = response.getAggregations().get(termsName);
assertThat(terms, notNullValue());
assertThat(terms.getName(), equalTo("terms"));
assertThat(terms.getName(), equalTo(termsName));
List<? extends Terms.Bucket> termsBuckets = terms.getBuckets();
assertThat(termsBuckets.size(), equalTo(interval));

Expand All @@ -365,9 +380,9 @@ public void testNested() throws Exception {
assertThat(termsBucket, notNullValue());
assertThat((String) termsBucket.getKey(), equalTo("tag" + (i % interval)));

Histogram histo = termsBucket.getAggregations().get("histo");
Histogram histo = termsBucket.getAggregations().get(histoName);
assertThat(histo, notNullValue());
assertThat(histo.getName(), equalTo("histo"));
assertThat(histo.getName(), equalTo(histoName));
List<? extends Bucket> buckets = histo.getBuckets();

double aggHistoSum = 0;
Expand Down
8 changes: 3 additions & 5 deletions ...g/elasticsearch/search/aggregations/pipeline/BucketMetricsPipelineAggregationBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import org.elasticsearch.search.DocValueFormat;
import org.elasticsearch.search.aggregations.AggregationBuilder;
import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy;
import org.elasticsearch.search.aggregations.support.AggregationPath;

import java.io.IOException;
import java.util.Map;
Expand Down Expand Up @@ -103,11 +104,8 @@ protected void validate(ValidationContext context) {
context.addBucketPathValidationError("must contain a single entry for aggregation [" + name + "]");
return;
}
// Need to find the first agg name in the buckets path to check its a
// multi bucket agg: aggs are split with '>' and can optionally have a
// metric name after them by using '.' so need to split on both to get
// just the agg name
final String firstAgg = bucketsPaths[0].split("[>\\.]")[0];
// find the first agg name in the buckets path to check its a multi bucket agg
final String firstAgg = AggregationPath.parse(bucketsPaths[0]).getPathElementsAsStringList().get(0);
Optional<AggregationBuilder> aggBuilder = context.getSiblingAggregations()
.stream()
.filter(builder -> builder.getName().equals(firstAgg))
Expand Down